mbox series

[0/2] proc: proc_setattr for /proc/$PID/net

Message ID 20230624-proc-net-setattr-v1-0-73176812adee@weissschuh.net
Headers show
Series proc: proc_setattr for /proc/$PID/net | expand

Message

Thomas Weißschuh June 24, 2023, 10:30 a.m. UTC 
/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,

Comments

Zhangjin Wu June 30, 2023, 2:06 p.m. UTC | #1 
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>
Thomas Weißschuh July 9, 2023, 5:10 p.m. UTC | #2 
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
Thomas Weißschuh July 9, 2023, 5:57 p.m. UTC | #3 
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
Thomas Weißschuh July 9, 2023, 6:22 p.m. UTC | #4 
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
Thomas Weißschuh July 10, 2023, 7:36 a.m. UTC | #5 
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_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.

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