We've aligned setgid behavior over multiple kernel releases. The details
can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
Consistent setgid stripping behavior is now encapsulated in the
setattr_should_drop_sgid() helper which is used by all filesystems that
strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
raised in e.g., chown_common() and is subject to the
setattr_should_drop_sgid() check to determine whether the setgid bit can
be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
will cause notify_change() to strip it even if the caller had the
necessary privileges to retain it. Ensure that nfsd only raises
ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
setgid bit.
Without this patch the setgid stripping tests in LTP will fail:
> As you can see, the problem is S_ISGID (0002000) was dropped on a
> non-group-executable file while chown was invoked by super-user, while
[...]
> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
[...]
> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
With this patch all tests pass.
Reported-by: Sherry Yang <[email protected]>
Signed-off-by: Christian Brauner <[email protected]>
---
ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s chown02
INFO: ltp-pan reported all tests PASS
LTP Version: 20230127-112-gf41e8a2fa
ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s fchown02
INFO: ltp-pan reported all tests PASS
LTP Version: 20230127-112-gf41e8a2fa
ubuntu@imp1-vm:~/src/git/xfstests$ sudo ./check -g perms
FSTYP -- nfs
PLATFORM -- Linux/x86_64 imp1-vm 6.3.0-nfs-setgid-3a3cfe624076 #20 SMP PREEMPT_DYNAMIC Tue May 2 12:35:51 UTC 2023
MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
MOUNT_OPTIONS -- -o vers=3,noacl 127.0.0.1:/nfsscratch /mnt/scratch
Passed all 41 tests
---
fs/nfsd/vfs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index bb9d47172162..c4ef24c5ffd0 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -388,7 +388,9 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
iap->ia_mode &= ~S_ISGID;
} else {
/* set ATTR_KILL_* bits and let VFS handle it */
- iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
+ iap->ia_valid |= ATTR_KILL_SUID;
+ iap->ia_valid |=
+ setattr_should_drop_sgid(&nop_mnt_idmap, inode);
}
}
}
--
2.34.1
> On May 2, 2023, at 9:36 AM, Christian Brauner <[email protected]> wrote:
>
> We've aligned setgid behavior over multiple kernel releases. The details
> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
> Consistent setgid stripping behavior is now encapsulated in the
> setattr_should_drop_sgid() helper which is used by all filesystems that
> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
> raised in e.g., chown_common() and is subject to the
> setattr_should_drop_sgid() check to determine whether the setgid bit can
> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
> will cause notify_change() to strip it even if the caller had the
> necessary privileges to retain it. Ensure that nfsd only raises
> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
> setgid bit.
>
> Without this patch the setgid stripping tests in LTP will fail:
>
>> As you can see, the problem is S_ISGID (0002000) was dropped on a
>> non-group-executable file while chown was invoked by super-user, while
>
> [...]
>
>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>
> [...]
>
>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>
> With this patch all tests pass.
>
> Reported-by: Sherry Yang <[email protected]>
> Signed-off-by: Christian Brauner <[email protected]>
There are some similar fstests failures this fix might address.
I've applied this patch to the nfsd-fixes tree for broader
testing. Thanks, Christian and Sherry!
> ---
> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s chown02
> INFO: ltp-pan reported all tests PASS
> LTP Version: 20230127-112-gf41e8a2fa
>
> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s fchown02
> INFO: ltp-pan reported all tests PASS
> LTP Version: 20230127-112-gf41e8a2fa
>
> ubuntu@imp1-vm:~/src/git/xfstests$ sudo ./check -g perms
> FSTYP -- nfs
> PLATFORM -- Linux/x86_64 imp1-vm 6.3.0-nfs-setgid-3a3cfe624076 #20 SMP PREEMPT_DYNAMIC Tue May 2 12:35:51 UTC 2023
> MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
> MOUNT_OPTIONS -- -o vers=3,noacl 127.0.0.1:/nfsscratch /mnt/scratch
> Passed all 41 tests
> ---
> fs/nfsd/vfs.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index bb9d47172162..c4ef24c5ffd0 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -388,7 +388,9 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
> iap->ia_mode &= ~S_ISGID;
> } else {
> /* set ATTR_KILL_* bits and let VFS handle it */
> - iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
> + iap->ia_valid |= ATTR_KILL_SUID;
> + iap->ia_valid |=
> + setattr_should_drop_sgid(&nop_mnt_idmap, inode);
> }
> }
> }
> --
> 2.34.1
>
--
Chuck Lever
> On May 2, 2023, at 9:49 AM, Chuck Lever III <[email protected]> wrote:
>
>>
>> On May 2, 2023, at 9:36 AM, Christian Brauner <[email protected]> wrote:
>>
>> We've aligned setgid behavior over multiple kernel releases. The details
>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
>> Consistent setgid stripping behavior is now encapsulated in the
>> setattr_should_drop_sgid() helper which is used by all filesystems that
>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
>> raised in e.g., chown_common() and is subject to the
>> setattr_should_drop_sgid() check to determine whether the setgid bit can
>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
>> will cause notify_change() to strip it even if the caller had the
>> necessary privileges to retain it. Ensure that nfsd only raises
>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
>> setgid bit.
>>
>> Without this patch the setgid stripping tests in LTP will fail:
>>
>>> As you can see, the problem is S_ISGID (0002000) was dropped on a
>>> non-group-executable file while chown was invoked by super-user, while
>>
>> [...]
>>
>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>
>> [...]
>>
>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>
>> With this patch all tests pass.
>>
>> Reported-by: Sherry Yang <[email protected]>
>> Signed-off-by: Christian Brauner <[email protected]>
>
> There are some similar fstests failures this fix might address.
>
> I've applied this patch to the nfsd-fixes tree for broader
> testing.
ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined!
Did I apply this patch to the wrong kernel?
> Thanks, Christian and Sherry!
>
>
>> ---
>> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s chown02
>> INFO: ltp-pan reported all tests PASS
>> LTP Version: 20230127-112-gf41e8a2fa
>>
>> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s fchown02
>> INFO: ltp-pan reported all tests PASS
>> LTP Version: 20230127-112-gf41e8a2fa
>>
>> ubuntu@imp1-vm:~/src/git/xfstests$ sudo ./check -g perms
>> FSTYP -- nfs
>> PLATFORM -- Linux/x86_64 imp1-vm 6.3.0-nfs-setgid-3a3cfe624076 #20 SMP PREEMPT_DYNAMIC Tue May 2 12:35:51 UTC 2023
>> MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
>> MOUNT_OPTIONS -- -o vers=3,noacl 127.0.0.1:/nfsscratch /mnt/scratch
>> Passed all 41 tests
>> ---
>> fs/nfsd/vfs.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index bb9d47172162..c4ef24c5ffd0 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -388,7 +388,9 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
>> iap->ia_mode &= ~S_ISGID;
>> } else {
>> /* set ATTR_KILL_* bits and let VFS handle it */
>> - iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
>> + iap->ia_valid |= ATTR_KILL_SUID;
>> + iap->ia_valid |=
>> + setattr_should_drop_sgid(&nop_mnt_idmap, inode);
>> }
>> }
>> }
>> --
>> 2.34.1
>>
>
> --
> Chuck Lever
--
Chuck Lever
On Tue, 2023-05-02 at 15:36 +0200, Christian Brauner wrote:
> We've aligned setgid behavior over multiple kernel releases. The details
> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
> Consistent setgid stripping behavior is now encapsulated in the
> setattr_should_drop_sgid() helper which is used by all filesystems that
> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
> raised in e.g., chown_common() and is subject to the
> setattr_should_drop_sgid() check to determine whether the setgid bit can
> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
> will cause notify_change() to strip it even if the caller had the
> necessary privileges to retain it. Ensure that nfsd only raises
> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
> setgid bit.
>
> Without this patch the setgid stripping tests in LTP will fail:
>
> > As you can see, the problem is S_ISGID (0002000) was dropped on a
> > non-group-executable file while chown was invoked by super-user, while
>
> [...]
>
> > fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>
> [...]
>
> > chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>
> With this patch all tests pass.
>
> Reported-by: Sherry Yang <[email protected]>
> Signed-off-by: Christian Brauner <[email protected]>
> ---
> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s chown02
> INFO: ltp-pan reported all tests PASS
> LTP Version: 20230127-112-gf41e8a2fa
>
> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s fchown02
> INFO: ltp-pan reported all tests PASS
> LTP Version: 20230127-112-gf41e8a2fa
>
> ubuntu@imp1-vm:~/src/git/xfstests$ sudo ./check -g perms
> FSTYP -- nfs
> PLATFORM -- Linux/x86_64 imp1-vm 6.3.0-nfs-setgid-3a3cfe624076 #20 SMP PREEMPT_DYNAMIC Tue May 2 12:35:51 UTC 2023
> MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
> MOUNT_OPTIONS -- -o vers=3,noacl 127.0.0.1:/nfsscratch /mnt/scratch
> Passed all 41 tests
> ---
> fs/nfsd/vfs.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index bb9d47172162..c4ef24c5ffd0 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -388,7 +388,9 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
> iap->ia_mode &= ~S_ISGID;
> } else {
> /* set ATTR_KILL_* bits and let VFS handle it */
> - iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
> + iap->ia_valid |= ATTR_KILL_SUID;
> + iap->ia_valid |=
> + setattr_should_drop_sgid(&nop_mnt_idmap, inode);
> }
> }
> }
Looks good to me. The thread should have assumed the user's creds by
this point.
Reviewed-by: Jeff Layton <[email protected]>
> On May 2, 2023, at 12:50 PM, Chuck Lever III <[email protected]> wrote:
>
>
>
>> On May 2, 2023, at 9:49 AM, Chuck Lever III <[email protected]> wrote:
>>
>>>
>>> On May 2, 2023, at 9:36 AM, Christian Brauner <[email protected]> wrote:
>>>
>>> We've aligned setgid behavior over multiple kernel releases. The details
>>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
>>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
>>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
>>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
>>> Consistent setgid stripping behavior is now encapsulated in the
>>> setattr_should_drop_sgid() helper which is used by all filesystems that
>>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
>>> raised in e.g., chown_common() and is subject to the
>>> setattr_should_drop_sgid() check to determine whether the setgid bit can
>>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
>>> will cause notify_change() to strip it even if the caller had the
>>> necessary privileges to retain it. Ensure that nfsd only raises
>>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
>>> setgid bit.
>>>
>>> Without this patch the setgid stripping tests in LTP will fail:
>>>
>>>> As you can see, the problem is S_ISGID (0002000) was dropped on a
>>>> non-group-executable file while chown was invoked by super-user, while
>>>
>>> [...]
>>>
>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>>
>>> [...]
>>>
>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>>
>>> With this patch all tests pass.
>>>
>>> Reported-by: Sherry Yang <[email protected]>
>>> Signed-off-by: Christian Brauner <[email protected]>
>>
>> There are some similar fstests failures this fix might address.
>>
>> I've applied this patch to the nfsd-fixes tree for broader
>> testing.
>
> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined!
>
> Did I apply this patch to the wrong kernel?
setattr_should_drop_sgid() is not available to callers built as
modules. It needs an EXPORT_SYMBOL or _GPL.
>> Thanks, Christian and Sherry!
>>
>>
>>> ---
>>> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s chown02
>>> INFO: ltp-pan reported all tests PASS
>>> LTP Version: 20230127-112-gf41e8a2fa
>>>
>>> ubuntu@imp1-vm:~/ltp-install$ sudo ./runltp -d /mnt -s fchown02
>>> INFO: ltp-pan reported all tests PASS
>>> LTP Version: 20230127-112-gf41e8a2fa
>>>
>>> ubuntu@imp1-vm:~/src/git/xfstests$ sudo ./check -g perms
>>> FSTYP -- nfs
>>> PLATFORM -- Linux/x86_64 imp1-vm 6.3.0-nfs-setgid-3a3cfe624076 #20 SMP PREEMPT_DYNAMIC Tue May 2 12:35:51 UTC 2023
>>> MKFS_OPTIONS -- 127.0.0.1:/nfsscratch
>>> MOUNT_OPTIONS -- -o vers=3,noacl 127.0.0.1:/nfsscratch /mnt/scratch
>>> Passed all 41 tests
>>> ---
>>> fs/nfsd/vfs.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>> index bb9d47172162..c4ef24c5ffd0 100644
>>> --- a/fs/nfsd/vfs.c
>>> +++ b/fs/nfsd/vfs.c
>>> @@ -388,7 +388,9 @@ nfsd_sanitize_attrs(struct inode *inode, struct iattr *iap)
>>> iap->ia_mode &= ~S_ISGID;
>>> } else {
>>> /* set ATTR_KILL_* bits and let VFS handle it */
>>> - iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
>>> + iap->ia_valid |= ATTR_KILL_SUID;
>>> + iap->ia_valid |=
>>> + setattr_should_drop_sgid(&nop_mnt_idmap, inode);
>>> }
>>> }
>>> }
>>> --
>>> 2.34.1
>>>
>>
>> --
>> Chuck Lever
>
>
> --
> Chuck Lever
--
Chuck Lever
On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote:
>
>
> > On May 2, 2023, at 12:50 PM, Chuck Lever III <[email protected]> wrote:
> >
> >
> >
> >> On May 2, 2023, at 9:49 AM, Chuck Lever III <[email protected]> wrote:
> >>
> >>>
> >>> On May 2, 2023, at 9:36 AM, Christian Brauner <[email protected]> wrote:
> >>>
> >>> We've aligned setgid behavior over multiple kernel releases. The details
> >>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
> >>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
> >>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
> >>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
> >>> Consistent setgid stripping behavior is now encapsulated in the
> >>> setattr_should_drop_sgid() helper which is used by all filesystems that
> >>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
> >>> raised in e.g., chown_common() and is subject to the
> >>> setattr_should_drop_sgid() check to determine whether the setgid bit can
> >>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
> >>> will cause notify_change() to strip it even if the caller had the
> >>> necessary privileges to retain it. Ensure that nfsd only raises
> >>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
> >>> setgid bit.
> >>>
> >>> Without this patch the setgid stripping tests in LTP will fail:
> >>>
> >>>> As you can see, the problem is S_ISGID (0002000) was dropped on a
> >>>> non-group-executable file while chown was invoked by super-user, while
> >>>
> >>> [...]
> >>>
> >>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
> >>>
> >>> [...]
> >>>
> >>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
> >>>
> >>> With this patch all tests pass.
> >>>
> >>> Reported-by: Sherry Yang <[email protected]>
> >>> Signed-off-by: Christian Brauner <[email protected]>
> >>
> >> There are some similar fstests failures this fix might address.
> >>
> >> I've applied this patch to the nfsd-fixes tree for broader
> >> testing.
> >
> > ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined!
> >
> > Did I apply this patch to the wrong kernel?
>
> setattr_should_drop_sgid() is not available to callers built as
> modules. It needs an EXPORT_SYMBOL or _GPL.
Hey Chuck,
Thanks for taking a look!
The required export is part of
commit 4f704d9a835 ("nfs: use vfs setgid helper")
which is in current mainline as of Monday this week which was part of my
v6.4/vfs.misc PR that was merged.
So this should all work fine on mainline. Seems I didn't use --base
which is why that info was missing.
Thanks!
Christian
Hi Christian,
On 03/05/23 12:30 pm, Christian Brauner wrote:
> On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote:
>>
>>
>>> On May 2, 2023, at 12:50 PM, Chuck Lever III <[email protected]> wrote:
>>>
>>>
>>>
>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <[email protected]> wrote:
>>>>
>>>>>
>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <[email protected]> wrote:
>>>>>
>>>>> We've aligned setgid behavior over multiple kernel releases. The details
>>>>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
>>>>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
>>>>> Consistent setgid stripping behavior is now encapsulated in the
>>>>> setattr_should_drop_sgid() helper which is used by all filesystems that
>>>>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
>>>>> raised in e.g., chown_common() and is subject to the
>>>>> setattr_should_drop_sgid() check to determine whether the setgid bit can
>>>>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
>>>>> will cause notify_change() to strip it even if the caller had the
>>>>> necessary privileges to retain it. Ensure that nfsd only raises
>>>>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
>>>>> setgid bit.
>>>>>
>>>>> Without this patch the setgid stripping tests in LTP will fail:
>>>>>
>>>>>> As you can see, the problem is S_ISGID (0002000) was dropped on a
>>>>>> non-group-executable file while chown was invoked by super-user, while
>>>>>
>>>>> [...]
>>>>>
>>>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>>>>
>>>>> [...]
>>>>>
>>>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>>>>
>>>>> With this patch all tests pass.
>>>>>
>>>>> Reported-by: Sherry Yang <[email protected]>
>>>>> Signed-off-by: Christian Brauner <[email protected]>
>>>>
We had a very similar report from kernel-test-robot.
https://lore.kernel.org/all/[email protected]/
Which points to commit: ed5a7047d201 ("attr: use consistent sgid
stripping checks")
And the above commit is backported to LTS kernels -- 5.10.y,5.15.y and
6.1.y
So would it be better to tag it to stable by adding "Cc:
<[email protected]>" ?
Thanks,
Harshit
>>>> There are some similar fstests failures this fix might address.
>>>>
>>>> I've applied this patch to the nfsd-fixes tree for broader
>>>> testing.
>>>
>>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined!
>>>
>>> Did I apply this patch to the wrong kernel?
>>
>> setattr_should_drop_sgid() is not available to callers built as
>> modules. It needs an EXPORT_SYMBOL or _GPL.
>
> Hey Chuck,
>
> Thanks for taking a look!
> The required export is part of
> commit 4f704d9a835 ("nfs: use vfs setgid helper")
> which is in current mainline as of Monday this week which was part of my
> v6.4/vfs.misc PR that was merged.
>
> So this should all work fine on mainline. Seems I didn't use --base
> which is why that info was missing.
>
> Thanks!
> Christian
>
>
> On May 3, 2023, at 6:53 AM, Harshit Mogalapalli <[email protected]> wrote:
>
> Hi Christian,
>
> On 03/05/23 12:30 pm, Christian Brauner wrote:
>> On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote:
>>>
>>>
>>>> On May 2, 2023, at 12:50 PM, Chuck Lever III <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <[email protected]> wrote:
>>>>>
>>>>>>
>>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <[email protected]> wrote:
>>>>>>
>>>>>> We've aligned setgid behavior over multiple kernel releases. The details
>>>>>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
>>>>>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
>>>>>> Consistent setgid stripping behavior is now encapsulated in the
>>>>>> setattr_should_drop_sgid() helper which is used by all filesystems that
>>>>>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
>>>>>> raised in e.g., chown_common() and is subject to the
>>>>>> setattr_should_drop_sgid() check to determine whether the setgid bit can
>>>>>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
>>>>>> will cause notify_change() to strip it even if the caller had the
>>>>>> necessary privileges to retain it. Ensure that nfsd only raises
>>>>>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
>>>>>> setgid bit.
>>>>>>
>>>>>> Without this patch the setgid stripping tests in LTP will fail:
>>>>>>
>>>>>>> As you can see, the problem is S_ISGID (0002000) was dropped on a
>>>>>>> non-group-executable file while chown was invoked by super-user, while
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>>>>>
>>>>>> With this patch all tests pass.
>>>>>>
>>>>>> Reported-by: Sherry Yang <[email protected]>
>>>>>> Signed-off-by: Christian Brauner <[email protected]>
>>>>>
>
> We had a very similar report from kernel-test-robot.
>
> https://lore.kernel.org/all/[email protected]/
>
> Which points to commit: ed5a7047d201 ("attr: use consistent sgid stripping checks")
>
> And the above commit is backported to LTS kernels -- 5.10.y,5.15.y and 6.1.y
>
> So would it be better to tag it to stable by adding "Cc: <[email protected]>" ?
>
> Thanks,
> Harshit
>
>>>>> There are some similar fstests failures this fix might address.
>>>>>
>>>>> I've applied this patch to the nfsd-fixes tree for broader
>>>>> testing.
>>>>
>>>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined!
>>>>
>>>> Did I apply this patch to the wrong kernel?
>>>
>>> setattr_should_drop_sgid() is not available to callers built as
>>> modules. It needs an EXPORT_SYMBOL or _GPL.
>> Hey Chuck,
>> Thanks for taking a look!
>> The required export is part of
>> commit 4f704d9a835 ("nfs: use vfs setgid helper")
>> which is in current mainline as of Monday this week which was part of my
>> v6.4/vfs.misc PR that was merged.
>> So this should all work fine on mainline. Seems I didn't use --base
>> which is why that info was missing.
>> Thanks!
>> Christian
I'll apply this to nfsd-next (6.5) and add Jeff's Reviewed-by
and a Cc: stable.
--
Chuck Lever
On Wed, May 03, 2023 at 02:05:48PM +0000, Chuck Lever III wrote:
>
>
> > On May 3, 2023, at 6:53 AM, Harshit Mogalapalli <[email protected]> wrote:
> >
> > Hi Christian,
> >
> > On 03/05/23 12:30 pm, Christian Brauner wrote:
> >> On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote:
> >>>
> >>>
> >>>> On May 2, 2023, at 12:50 PM, Chuck Lever III <[email protected]> wrote:
> >>>>
> >>>>
> >>>>
> >>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <[email protected]> wrote:
> >>>>>
> >>>>>>
> >>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <[email protected]> wrote:
> >>>>>>
> >>>>>> We've aligned setgid behavior over multiple kernel releases. The details
> >>>>>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
> >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
> >>>>>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
> >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
> >>>>>> Consistent setgid stripping behavior is now encapsulated in the
> >>>>>> setattr_should_drop_sgid() helper which is used by all filesystems that
> >>>>>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
> >>>>>> raised in e.g., chown_common() and is subject to the
> >>>>>> setattr_should_drop_sgid() check to determine whether the setgid bit can
> >>>>>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
> >>>>>> will cause notify_change() to strip it even if the caller had the
> >>>>>> necessary privileges to retain it. Ensure that nfsd only raises
> >>>>>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
> >>>>>> setgid bit.
> >>>>>>
> >>>>>> Without this patch the setgid stripping tests in LTP will fail:
> >>>>>>
> >>>>>>> As you can see, the problem is S_ISGID (0002000) was dropped on a
> >>>>>>> non-group-executable file while chown was invoked by super-user, while
> >>>>>>
> >>>>>> [...]
> >>>>>>
> >>>>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
> >>>>>>
> >>>>>> [...]
> >>>>>>
> >>>>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
> >>>>>>
> >>>>>> With this patch all tests pass.
> >>>>>>
> >>>>>> Reported-by: Sherry Yang <[email protected]>
> >>>>>> Signed-off-by: Christian Brauner <[email protected]>
> >>>>>
> >
> > We had a very similar report from kernel-test-robot.
> >
> > https://lore.kernel.org/all/[email protected]/
> >
> > Which points to commit: ed5a7047d201 ("attr: use consistent sgid stripping checks")
> >
> > And the above commit is backported to LTS kernels -- 5.10.y,5.15.y and 6.1.y
> >
> > So would it be better to tag it to stable by adding "Cc: <[email protected]>" ?
> >
> > Thanks,
> > Harshit
> >
> >>>>> There are some similar fstests failures this fix might address.
> >>>>>
> >>>>> I've applied this patch to the nfsd-fixes tree for broader
> >>>>> testing.
> >>>>
> >>>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined!
> >>>>
> >>>> Did I apply this patch to the wrong kernel?
> >>>
> >>> setattr_should_drop_sgid() is not available to callers built as
> >>> modules. It needs an EXPORT_SYMBOL or _GPL.
> >> Hey Chuck,
> >> Thanks for taking a look!
> >> The required export is part of
> >> commit 4f704d9a835 ("nfs: use vfs setgid helper")
> >> which is in current mainline as of Monday this week which was part of my
> >> v6.4/vfs.misc PR that was merged.
> >> So this should all work fine on mainline. Seems I didn't use --base
> >> which is why that info was missing.
> >> Thanks!
> >> Christian
>
> I'll apply this to nfsd-next (6.5) and add Jeff's Reviewed-by
> and a Cc: stable.
Thanks all! Appreciate the Cc: stable fixup.
> On May 3, 2023, at 7:05 AM, Chuck Lever III <[email protected]> wrote:
>
>
>
>> On May 3, 2023, at 6:53 AM, Harshit Mogalapalli <[email protected]> wrote:
>>
>> Hi Christian,
>>
>> On 03/05/23 12:30 pm, Christian Brauner wrote:
>>> On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote:
>>>>
>>>>
>>>>> On May 2, 2023, at 12:50 PM, Chuck Lever III <[email protected]> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <[email protected]> wrote:
>>>>>>
>>>>>>>
>>>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <[email protected]> wrote:
>>>>>>>
>>>>>>> We've aligned setgid behavior over multiple kernel releases. The details
>>>>>>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
>>>>>>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
>>>>>>> Consistent setgid stripping behavior is now encapsulated in the
>>>>>>> setattr_should_drop_sgid() helper which is used by all filesystems that
>>>>>>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
>>>>>>> raised in e.g., chown_common() and is subject to the
>>>>>>> setattr_should_drop_sgid() check to determine whether the setgid bit can
>>>>>>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
>>>>>>> will cause notify_change() to strip it even if the caller had the
>>>>>>> necessary privileges to retain it. Ensure that nfsd only raises
>>>>>>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
>>>>>>> setgid bit.
>>>>>>>
>>>>>>> Without this patch the setgid stripping tests in LTP will fail:
>>>>>>>
>>>>>>>> As you can see, the problem is S_ISGID (0002000) was dropped on a
>>>>>>>> non-group-executable file while chown was invoked by super-user, while
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>>>>>>
>>>>>>> With this patch all tests pass.
>>>>>>>
>>>>>>> Reported-by: Sherry Yang <[email protected]>
>>>>>>> Signed-off-by: Christian Brauner <[email protected]>
>>>>>>
>>
>> We had a very similar report from kernel-test-robot.
>>
>> https://lore.kernel.org/all/[email protected]/
>>
>> Which points to commit: ed5a7047d201 ("attr: use consistent sgid stripping checks")
>>
>> And the above commit is backported to LTS kernels -- 5.10.y,5.15.y and 6.1.y
>>
>> So would it be better to tag it to stable by adding "Cc: <[email protected]>" ?
>>
>> Thanks,
>> Harshit
>>
>>>>>> There are some similar fstests failures this fix might address.
>>>>>>
>>>>>> I've applied this patch to the nfsd-fixes tree for broader
>>>>>> testing.
>>>>>
>>>>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined!
>>>>>
>>>>> Did I apply this patch to the wrong kernel?
>>>>
>>>> setattr_should_drop_sgid() is not available to callers built as
>>>> modules. It needs an EXPORT_SYMBOL or _GPL.
>>> Hey Chuck,
>>> Thanks for taking a look!
>>> The required export is part of
>>> commit 4f704d9a835 ("nfs: use vfs setgid helper")
>>> which is in current mainline as of Monday this week which was part of my
>>> v6.4/vfs.misc PR that was merged.
>>> So this should all work fine on mainline. Seems I didn't use --base
>>> which is why that info was missing.
>>> Thanks!
>>> Christian
>
> I'll apply this to nfsd-next (6.5) and add Jeff's Reviewed-by
> and a Cc: stable.
Hi Chuck,
Just following up to see if you know when this patch will go to linux mainline and linux-stable?
Thanks,
Sherry
>
>
> --
> Chuck Lever
>
>
> On May 15, 2023, at 12:34 PM, Sherry Yang <[email protected]> wrote:
>
>
>
>> On May 3, 2023, at 7:05 AM, Chuck Lever III <[email protected]> wrote:
>>
>>
>>
>>> On May 3, 2023, at 6:53 AM, Harshit Mogalapalli <[email protected]> wrote:
>>>
>>> Hi Christian,
>>>
>>> On 03/05/23 12:30 pm, Christian Brauner wrote:
>>>> On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote:
>>>>>
>>>>>
>>>>>> On May 2, 2023, at 12:50 PM, Chuck Lever III <[email protected]> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <[email protected]> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <[email protected]> wrote:
>>>>>>>>
>>>>>>>> We've aligned setgid behavior over multiple kernel releases. The details
>>>>>>>> can be found in commit cf619f891971 ("Merge tag 'fs.ovl.setgid.v6.2' of
>>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping") and
>>>>>>>> commit 426b4ca2d6a5 ("Merge tag 'fs.setgid.v6.0' of
>>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux").
>>>>>>>> Consistent setgid stripping behavior is now encapsulated in the
>>>>>>>> setattr_should_drop_sgid() helper which is used by all filesystems that
>>>>>>>> strip setgid bits outside of vfs proper. Usually ATTR_KILL_SGID is
>>>>>>>> raised in e.g., chown_common() and is subject to the
>>>>>>>> setattr_should_drop_sgid() check to determine whether the setgid bit can
>>>>>>>> be retained. Since nfsd is raising ATTR_KILL_SGID unconditionally it
>>>>>>>> will cause notify_change() to strip it even if the caller had the
>>>>>>>> necessary privileges to retain it. Ensure that nfsd only raises
>>>>>>>> ATR_KILL_SGID if the caller lacks the necessary privileges to retain the
>>>>>>>> setgid bit.
>>>>>>>>
>>>>>>>> Without this patch the setgid stripping tests in LTP will fail:
>>>>>>>>
>>>>>>>>> As you can see, the problem is S_ISGID (0002000) was dropped on a
>>>>>>>>> non-group-executable file while chown was invoked by super-user, while
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
>>>>>>>>
>>>>>>>> With this patch all tests pass.
>>>>>>>>
>>>>>>>> Reported-by: Sherry Yang <[email protected]>
>>>>>>>> Signed-off-by: Christian Brauner <[email protected]>
>>>>>>>
>>>
>>> We had a very similar report from kernel-test-robot.
>>>
>>> https://lore.kernel.org/all/[email protected]/
>>>
>>> Which points to commit: ed5a7047d201 ("attr: use consistent sgid stripping checks")
>>>
>>> And the above commit is backported to LTS kernels -- 5.10.y,5.15.y and 6.1.y
>>>
>>> So would it be better to tag it to stable by adding "Cc: <[email protected]>" ?
>>>
>>> Thanks,
>>> Harshit
>>>
>>>>>>> There are some similar fstests failures this fix might address.
>>>>>>>
>>>>>>> I've applied this patch to the nfsd-fixes tree for broader
>>>>>>> testing.
>>>>>>
>>>>>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined!
>>>>>>
>>>>>> Did I apply this patch to the wrong kernel?
>>>>>
>>>>> setattr_should_drop_sgid() is not available to callers built as
>>>>> modules. It needs an EXPORT_SYMBOL or _GPL.
>>>> Hey Chuck,
>>>> Thanks for taking a look!
>>>> The required export is part of
>>>> commit 4f704d9a835 ("nfs: use vfs setgid helper")
>>>> which is in current mainline as of Monday this week which was part of my
>>>> v6.4/vfs.misc PR that was merged.
>>>> So this should all work fine on mainline. Seems I didn't use --base
>>>> which is why that info was missing.
>>>> Thanks!
>>>> Christian
>>
>> I'll apply this to nfsd-next (6.5) and add Jeff's Reviewed-by
>> and a Cc: stable.
>
> Hi Chuck,
>
> Just following up to see if you know when this patch will go to linux mainline and linux-stable?
Howdy- mentioned above: applied to nfsd-next for the v6.5 merge
window, which will open in several weeks.
I added a Cc: stable, so once it is in 6.5-rc it will be automatically
backported into the stable kernels.
--
Chuck Lever