> On May 2, 2023, at 12:50 PM, Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
>
>
>
>> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
>>
>>>
>>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@xxxxxxxxxx> 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 <sherry.yang@xxxxxxxxxx>
>>> Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
>>
>> 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
