> On May 15, 2023, at 12:34 PM, Sherry Yang <sherry.yang@xxxxxxxxxx> wrote:
>
>
>
>> On May 3, 2023, at 7:05 AM, Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
>>
>>
>>
>>> On May 3, 2023, at 6:53 AM, Harshit Mogalapalli <harshit.m.mogalapalli@xxxxxxxxxx> 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 <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>
>>>>>>>
>>>
>>> We had a very similar report from kernel-test-robot.
>>>
>>> https://lore.kernel.org/all/202210091600.dbe52cbf-yujie.liu@xxxxxxxxx/
>>>
>>> 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@xxxxxxxxxxxxxxx>" ?
>>>
>>> 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
