Re: [PATCH] nfsd: use vfs setgid helper

[Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>]

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 <chuck.lever@oracle.com> wrote:
>>>
>>>
>>>
>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@oracle.com> wrote:
>>>>
>>>>>
>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> 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@oracle.com>
>>>>> Signed-off-by: Christian Brauner <brauner@kernel.org>
>>>>

We had a very similar report from kernel-test-robot.

https://lore.kernel.org/all/202210091600.dbe52cbf-yujie.liu@intel.com/

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: 
<stable@vger.kernel.org>" ?

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
> 
>