/proc/$PID/net currently allows the setting of file attributes, in contrast to other /proc/$PID/ files and directories.
This would break the nolibc testsuite so the first patch in the series removes the offending testcase. The "fix" for nolibc-test is intentionally kept trivial as the series will most likely go through the filesystem tree and if conflicts arise, it is obvious on how to resolve them.
Technically this can lead to breakage of nolibc-test if an old nolibc-test is used with a newer kernel containing the fix.
Note:
Except for /proc itself this is the only "struct inode_operations" in fs/proc/ that is missing an implementation of setattr().
Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- Thomas Weißschuh (2): selftests/nolibc: drop test chmod_net proc: use generic setattr() for /proc/$PID/net
fs/proc/proc_net.c | 1 + tools/testing/selftests/nolibc/nolibc-test.c | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) --- base-commit: a92b7d26c743b9dc06d520f863d624e94978a1d9 change-id: 20230624-proc-net-setattr-8f0a6b8eb2f5
Best regards,
The test relies on /proc/$PID/net to allow chmod() operations. It is the only file or directory in /proc/$PID/ to allow this and a bug. That bug will be fixed in the next patch in the series and therefore the test would start failing.
Link: https://lore.kernel.org/lkml/d0d111ef-edae-4760-83fb-36db84278da1@t-8ch.de/ Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- tools/testing/selftests/nolibc/nolibc-test.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c index 21bacc928bf7..7e649814a241 100644 --- a/tools/testing/selftests/nolibc/nolibc-test.c +++ b/tools/testing/selftests/nolibc/nolibc-test.c @@ -512,7 +512,6 @@ int run_syscall(int min, int max) CASE_TEST(chdir_root); EXPECT_SYSZR(1, chdir("/")); break; CASE_TEST(chdir_dot); EXPECT_SYSZR(1, chdir(".")); break; CASE_TEST(chdir_blah); EXPECT_SYSER(1, chdir("/blah"), -1, ENOENT); break; - CASE_TEST(chmod_net); EXPECT_SYSZR(proc, chmod("/proc/self/net", 0555)); break; CASE_TEST(chmod_self); EXPECT_SYSER(proc, chmod("/proc/self", 0555), -1, EPERM); break; CASE_TEST(chown_self); EXPECT_SYSER(proc, chown("/proc/self", 0, 0), -1, EPERM); break; CASE_TEST(chroot_root); EXPECT_SYSZR(euid0, chroot("/")); break;
All other files in /proc/$PID/ use proc_setattr().
Not using it allows the usage of chmod() on /proc/$PID/net, even on other processes owned by the same user. The same would probably also be true for other attributes to be changed.
As this technically represents an ABI change it is not marked for stable so any unlikely regressions are caught during a full release cycle.
Fixes: e9720acd728a ("[NET]: Make /proc/net a symlink on /proc/self/net (v3)") Signed-off-by: Thomas Weißschuh linux@weissschuh.net --- fs/proc/proc_net.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c index a0c0419872e3..78f9e6b469c0 100644 --- a/fs/proc/proc_net.c +++ b/fs/proc/proc_net.c @@ -321,6 +321,7 @@ static int proc_tgid_net_getattr(struct mnt_idmap *idmap, const struct inode_operations proc_net_inode_operations = { .lookup = proc_tgid_net_lookup, .getattr = proc_tgid_net_getattr, + .setattr = proc_setattr, };
static int proc_tgid_net_readdir(struct file *file, struct dir_context *ctx)
On Sat, Jun 24, 2023 at 12:30:47PM +0200, Thomas Weißschuh wrote:
All other files in /proc/$PID/ use proc_setattr().
Not using it allows the usage of chmod() on /proc/$PID/net, even on other processes owned by the same user. The same would probably also be true for other attributes to be changed.
As this technically represents an ABI change it is not marked for stable so any unlikely regressions are caught during a full release cycle.
Fixes: e9720acd728a ("[NET]: Make /proc/net a symlink on /proc/self/net (v3)") Signed-off-by: Thomas Weißschuh linux@weissschuh.net
fs/proc/proc_net.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/fs/proc/proc_net.c b/fs/proc/proc_net.c index a0c0419872e3..78f9e6b469c0 100644 --- a/fs/proc/proc_net.c +++ b/fs/proc/proc_net.c @@ -321,6 +321,7 @@ static int proc_tgid_net_getattr(struct mnt_idmap *idmap, const struct inode_operations proc_net_inode_operations = { .lookup = proc_tgid_net_lookup, .getattr = proc_tgid_net_getattr,
- .setattr = proc_setattr,
}; static int proc_tgid_net_readdir(struct file *file, struct dir_context *ctx)
So your concern really is specifically about /proc/$pid/net itself as that's owned by the user and thus the user itself can chmod it and thus also restrict access for other processess running with the same uid:
chmod 0000 /proc/1234/net ls -al /proc/self/net ls: cannot open directory '/proc/self/net/': Permission denied
Yeah, it's not a huge deal but it's arguably a bug especially since the original commit from 2006 that introduced proc_setattr() was clear that it should apply to anything beneath /proc/<pid>/ owned by the user.
So I agree and we should probably try and have the same behavior for /proc/$pid/net as well. We can see if that breaks something.
Hi, Thomas
Just applied your patchset on v6.4, and then:
- revert the 1st patch: 'selftests/nolibc: drop test chmod_net' manually
- do the 'run' test of nolibc on arm/vexpress-a9
The 'chmod_net' test of tools/testing/selftests/nolibc/nolibc-test.c really failed as expected (and therefore, should be removed):
11 chdir_root = 0 [OK] 12 chdir_dot = 0 [OK] 13 chdir_blah = -1 ENOENT [OK] 14 chmod_net = -1 EPERM [FAIL] 15 chmod_self = -1 EPERM [OK] 16 chmod_tmpdir = 0 [OK] 17 chown_self = -1 EPERM [OK]
So, If this test result is enough for this patch, here is my:
Tested-by: Zhangjin Wu falcon@tinylab.org
Best regards, Zhangjin
/proc/$PID/net currently allows the setting of file attributes, in contrast to other /proc/$PID/ files and directories.
This would break the nolibc testsuite so the first patch in the series removes the offending testcase. The "fix" for nolibc-test is intentionally kept trivial as the series will most likely go through the filesystem tree and if conflicts arise, it is obvious on how to resolve them.
Technically this can lead to breakage of nolibc-test if an old nolibc-test is used with a newer kernel containing the fix.
Note:
Except for /proc itself this is the only "struct inode_operations" in fs/proc/ that is missing an implementation of setattr().
Signed-off-by: Thomas Weißschuh linux@weissschuh.net
Thomas Weißschuh (2): selftests/nolibc: drop test chmod_net proc: use generic setattr() for /proc/$PID/net
fs/proc/proc_net.c | 1 + tools/testing/selftests/nolibc/nolibc-test.c | 1 - 2 files changed, 1 insertion(+), 1 deletion(-)
base-commit: a92b7d26c743b9dc06d520f863d624e94978a1d9 change-id: 20230624-proc-net-setattr-8f0a6b8eb2f5
Best regards,
Thomas Weißschuh linux@weissschuh.net
Hi,
On Fri, Jun 30, 2023 at 10:06:09PM +0800, Zhangjin Wu wrote:
Hi, Thomas
Just applied your patchset on v6.4, and then:
revert the 1st patch: 'selftests/nolibc: drop test chmod_net' manually
do the 'run' test of nolibc on arm/vexpress-a9
The 'chmod_net' test of tools/testing/selftests/nolibc/nolibc-test.c really failed as expected (and therefore, should be removed):
11 chdir_root = 0 [OK] 12 chdir_dot = 0 [OK] 13 chdir_blah = -1 ENOENT [OK] 14 chmod_net = -1 EPERM [FAIL] 15 chmod_self = -1 EPERM [OK] 16 chmod_tmpdir = 0 [OK] 17 chown_self = -1 EPERM [OK]So, If this test result is enough for this patch, here is my:
Tested-by: Zhangjin Wu falcon@tinylab.org
Now queued, thanks! Willy
Hi Willy,
On 2023-07-09 11:29:47+0200, Willy Tarreau wrote:
On Fri, Jun 30, 2023 at 10:06:09PM +0800, Zhangjin Wu wrote:
[..]
Now queued, thanks! Willy
Don't we need an Ack from the fs maintainers for the patch to fs/proc/proc_net.c ?
Personally I expected this series to go in via the fs tree because of that patch.
Thomas
Hi Thomas,
On Sun, Jul 09, 2023 at 07:10:58PM +0200, Thomas Weißschuh wrote:
Hi Willy,
On 2023-07-09 11:29:47+0200, Willy Tarreau wrote:
On Fri, Jun 30, 2023 at 10:06:09PM +0800, Zhangjin Wu wrote:
[..]
Now queued, thanks! Willy
Don't we need an Ack from the fs maintainers for the patch to fs/proc/proc_net.c ?
Personally I expected this series to go in via the fs tree because of that patch.
Gasp! You're totally right, I confused it with a test only changing the nolibc-test file, as the chmod_net test appeared as a dependency! Let me drop it from the series and push again.
Thanks a lot for notifying me! Willy
Hi Willy,
On 2023-07-09 19:27:53+0200, Willy Tarreau wrote:
On Sun, Jul 09, 2023 at 07:10:58PM +0200, Thomas Weißschuh wrote:
On 2023-07-09 11:29:47+0200, Willy Tarreau wrote:
On Fri, Jun 30, 2023 at 10:06:09PM +0800, Zhangjin Wu wrote:
[..]
Now queued, thanks! Willy
Don't we need an Ack from the fs maintainers for the patch to fs/proc/proc_net.c ?
Personally I expected this series to go in via the fs tree because of that patch.
Gasp! You're totally right, I confused it with a test only changing the nolibc-test file, as the chmod_net test appeared as a dependency! Let me drop it from the series and push again.
I think if this patch now also goes in via both the nolibc/rcu trees and the fs tree it would not be great.
The best way forward would probably for you to rebase your tree on top of mainline after the fs tree has introduced both patches of the series into Linus' tree and then you can drop your copy of the test removal.
I want to keep both patches together because I expect the fs change to be backported and if it is backported on its own it will break nolibc-test in those trees.
But maybe I'm overthinking it, nobody is running nolibc-test on non-mainline kernels anyways and both patches can be split.
If they are to be kept together and go via fs an Ack on the nolibc-test patch is probably needed, too.
Thomas
On Sun, Jul 09, 2023 at 07:57:27PM +0200, Thomas Weißschuh wrote:
Hi Willy,
On 2023-07-09 19:27:53+0200, Willy Tarreau wrote:
On Sun, Jul 09, 2023 at 07:10:58PM +0200, Thomas Weißschuh wrote:
On 2023-07-09 11:29:47+0200, Willy Tarreau wrote:
On Fri, Jun 30, 2023 at 10:06:09PM +0800, Zhangjin Wu wrote:
[..]
Now queued, thanks! Willy
Don't we need an Ack from the fs maintainers for the patch to fs/proc/proc_net.c ?
Personally I expected this series to go in via the fs tree because of that patch.
Gasp! You're totally right, I confused it with a test only changing the nolibc-test file, as the chmod_net test appeared as a dependency! Let me drop it from the series and push again.
I think if this patch now also goes in via both the nolibc/rcu trees and the fs tree it would not be great.
The best way forward would probably for you to rebase your tree on top of mainline after the fs tree has introduced both patches of the series into Linus' tree and then you can drop your copy of the test removal.
Yeah I agree.
I want to keep both patches together because I expect the fs change to be backported and if it is backported on its own it will break nolibc-test in those trees.
OK but we can also fix the test regardless, and mark it for backport, no ?
But maybe I'm overthinking it, nobody is running nolibc-test on non-mainline kernels anyways and both patches can be split.
I agree that we shouldn't grant too much importance to this test ;-) I'm regularly seeing Sasha propose them for backports and am thinking "ok it cannot hurt but I'm not convinced anyone will notice the fix".
If they are to be kept together and go via fs an Ack on the nolibc-test patch is probably needed, too.
OK. Let's first see if someone from FS agrees on the change.
Thanks for the clarification, Willy
On 2023-07-09 20:04:32+0200, Willy Tarreau wrote:
On Sun, Jul 09, 2023 at 07:57:27PM +0200, Thomas Weißschuh wrote:
On 2023-07-09 19:27:53+0200, Willy Tarreau wrote:
On Sun, Jul 09, 2023 at 07:10:58PM +0200, Thomas Weißschuh wrote:
On 2023-07-09 11:29:47+0200, Willy Tarreau wrote:
On Fri, Jun 30, 2023 at 10:06:09PM +0800, Zhangjin Wu wrote:
[..]
Now queued, thanks! Willy
Don't we need an Ack from the fs maintainers for the patch to fs/proc/proc_net.c ?
Personally I expected this series to go in via the fs tree because of that patch.
Gasp! You're totally right, I confused it with a test only changing the nolibc-test file, as the chmod_net test appeared as a dependency! Let me drop it from the series and push again.
I think if this patch now also goes in via both the nolibc/rcu trees and the fs tree it would not be great.
The best way forward would probably for you to rebase your tree on top of mainline after the fs tree has introduced both patches of the series into Linus' tree and then you can drop your copy of the test removal.
Yeah I agree.
I want to keep both patches together because I expect the fs change to be backported and if it is backported on its own it will break nolibc-test in those trees.
OK but we can also fix the test regardless, and mark it for backport, no ?
That should work fine, too. Can you add the Fixes and Cc-stable tags in your tree and let the fs maintainers know? Or do you want me to split and resend the series?
But maybe I'm overthinking it, nobody is running nolibc-test on non-mainline kernels anyways and both patches can be split.
I agree that we shouldn't grant too much importance to this test ;-) I'm regularly seeing Sasha propose them for backports and am thinking "ok it cannot hurt but I'm not convinced anyone will notice the fix".
If they are to be kept together and go via fs an Ack on the nolibc-test patch is probably needed, too.
OK. Let's first see if someone from FS agrees on the change.
Sounds good.
Thomas
On Sun, Jul 09, 2023 at 08:22:31PM +0200, Thomas Weißschuh wrote:
On 2023-07-09 20:04:32+0200, Willy Tarreau wrote:
On Sun, Jul 09, 2023 at 07:57:27PM +0200, Thomas Weißschuh wrote:
On 2023-07-09 19:27:53+0200, Willy Tarreau wrote:
On Sun, Jul 09, 2023 at 07:10:58PM +0200, Thomas Weißschuh wrote:
On 2023-07-09 11:29:47+0200, Willy Tarreau wrote:
On Fri, Jun 30, 2023 at 10:06:09PM +0800, Zhangjin Wu wrote: > [..]
Now queued, thanks! Willy
Don't we need an Ack from the fs maintainers for the patch to fs/proc/proc_net.c ?
Personally I expected this series to go in via the fs tree because of that patch.
Gasp! You're totally right, I confused it with a test only changing the nolibc-test file, as the chmod_net test appeared as a dependency! Let me drop it from the series and push again.
I think if this patch now also goes in via both the nolibc/rcu trees and the fs tree it would not be great.
The best way forward would probably for you to rebase your tree on top of mainline after the fs tree has introduced both patches of the series into Linus' tree and then you can drop your copy of the test removal.
Yeah I agree.
I want to keep both patches together because I expect the fs change to be backported and if it is backported on its own it will break nolibc-test in those trees.
OK but we can also fix the test regardless, and mark it for backport, no ?
That should work fine, too. Can you add the Fixes and Cc-stable tags in your tree and let the fs maintainers know?
OK here's what it's like now, let me know if you'd prefer any change:
commit 8c2e51e174ed0f998b6bd90244324a4966a55efc Author: Thomas Weißschuh linux@weissschuh.net Date: Sat Jun 24 12:30:46 2023 +0200
selftests/nolibc: drop test chmod_net
The test relies on /proc/$PID/net to allow chmod() operations. It is the only file or directory in /proc/$PID/ to allow this and a bug. That bug will be fixed in the next patch in the series and therefore the test would start failing.
Link: https://lore.kernel.org/lkml/d0d111ef-edae-4760-83fb-36db84278da1@t-8ch.de/ Fixes: b4844fa0bdb4 ("selftests/nolibc: implement a few tests for various syscalls") Signed-off-by: Thomas Weißschuh linux@weissschuh.net Tested-by: Zhangjin Wu falcon@tinylab.org Signed-off-by: Willy Tarreau w@1wt.eu
Or do you want me to split and resend the series?
Not needed, thank you. Willy
On 2023-07-10 09:09:20+0200, Willy Tarreau wrote:
On Sun, Jul 09, 2023 at 08:22:31PM +0200, Thomas Weißschuh wrote:
On 2023-07-09 20:04:32+0200, Willy Tarreau wrote:
[..]
That should work fine, too. Can you add the Fixes and Cc-stable tags in your tree and let the fs maintainers know?
OK here's what it's like now, let me know if you'd prefer any change:
commit 8c2e51e174ed0f998b6bd90244324a4966a55efc Author: Thomas Weißschuh linux@weissschuh.net Date: Sat Jun 24 12:30:46 2023 +0200
selftests/nolibc: drop test chmod_netThe test relies on /proc/$PID/net to allow chmod() operations. It is the only file or directory in /proc/$PID/ to allow this and a bug. That bug will be fixed in the next patch in the series and therefore the test would start failing.
As the patch is now standalone the part "fixed in the next patch in the series" is not accurate anymore. Maybe only "When this bug gets fixed the test would start failing"?
Link: https://lore.kernel.org/lkml/d0d111ef-edae-4760-83fb-36db84278da1@t-8ch.de/ Fixes: b4844fa0bdb4 ("selftests/nolibc: implement a few tests for various syscalls")
+ Cc: stable@vger.kernel.org
The Fixes tag alone is not enough to trigger the formalized backport process. It may be picked up anyways through heuristics but that would only be luck.
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> Tested-by: Zhangjin Wu <falcon@tinylab.org> Signed-off-by: Willy Tarreau <w@1wt.eu>Or do you want me to split and resend the series?
Not needed, thank you.
Thanks!
Thomas
On Sun, Jul 09, 2023 at 07:10:58PM +0200, Thomas Weißschuh wrote:
Hi Willy,
On 2023-07-09 11:29:47+0200, Willy Tarreau wrote:
On Fri, Jun 30, 2023 at 10:06:09PM +0800, Zhangjin Wu wrote:
[..]
Now queued, thanks! Willy
Don't we need an Ack from the fs maintainers for the patch to fs/proc/proc_net.c ?
Personally I expected this series to go in via the fs tree because of that patch.
I don't necessarily see patches I'm not Cced on.
On 2023-07-13 13:51:39+0200, Christian Brauner wrote:
On Sun, Jul 09, 2023 at 07:10:58PM +0200, Thomas Weißschuh wrote:
Hi Willy,
On 2023-07-09 11:29:47+0200, Willy Tarreau wrote:
On Fri, Jun 30, 2023 at 10:06:09PM +0800, Zhangjin Wu wrote:
[..]
Now queued, thanks! Willy
Don't we need an Ack from the fs maintainers for the patch to fs/proc/proc_net.c ?
Personally I expected this series to go in via the fs tree because of that patch.
I don't necessarily see patches I'm not Cced on.
Currently you are not listed explicitly maintainer for "PROC FILESYSTEM" in MAINTAINERS. If those should also go to you directly could you add yourself in MAINTAINERS? Otherwise get_maintainers.pl will miss your address.
Thomas
On Sat, 24 Jun 2023 12:30:45 +0200, Thomas Weißschuh wrote:
/proc/$PID/net currently allows the setting of file attributes, in contrast to other /proc/$PID/ files and directories.
This would break the nolibc testsuite so the first patch in the series removes the offending testcase. The "fix" for nolibc-test is intentionally kept trivial as the series will most likely go through the filesystem tree and if conflicts arise, it is obvious on how to resolve them.
[...]
I've picked both for now. Let me know if I should do something else. In any case, this needs long soaking in -next.
---
Applied to the fs.proc.net.uapi branch of the vfs/vfs.git tree. Patches in the fs.proc.net.uapi branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: fs.proc.net.uapi
[1/2] selftests/nolibc: drop test chmod_net https://git.kernel.org/vfs/vfs/c/49319832de90 [2/2] proc: use generic setattr() for /proc/$PID/net https://git.kernel.org/vfs/vfs/c/18e66ae67673
linux-kselftest-mirror@lists.linaro.org
-
Christian Brauner -
Thomas Weißschuh -
Thomas Weißschuh -
Willy Tarreau -
Zhangjin Wu