Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>,
        Christian Brauner <brauner@kernel.org>
CC:     Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Sherry Yang <sherry.yang@oracle.com>,
        Darren Kenny <darren.kenny@oracle.com>
Subject: Re: [PATCH] nfsd: use vfs setgid helper
Thread-Topic: [PATCH] nfsd: use vfs setgid helper
Thread-Index: AQHZfPsU0hWi9leffEW/mcTC+uI3U69G/7aAgAAykoCAABoxAIAA02kAgABBLwCAADWlAA==
Date:   Wed, 3 May 2023 14:05:48 +0000
Message-ID: <A5A8DAD5-7FFD-4CE6-ADC9-FF9F5E509869@oracle.com>
References: <20230502-agenda-regeln-04d2573bd0fd@brauner>
 <77C7061E-2316-4C73-89E5-7C8CA0AEB6FD@oracle.com>
 <51805A56-F815-405F-8CDF-4CD04A17436C@oracle.com>
 <741D94D5-4058-452E-830B-49D3BEBD5D8E@oracle.com>
 <20230503-mehrverbrauch-spargel-258668d27f53@brauner>
 <ac5c9882-8cc7-8d64-5784-fd71b04dde3a@oracle.com>
In-Reply-To: <ac5c9882-8cc7-8d64-5784-fd71b04dde3a@oracle.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?nkuBQpEPfk2I2EcSPx4+uSEIC/kcc5qw3EiwRE7W3zQbH9SaPK77fZ9Gye5/?=
 =?us-ascii?Q?RlHB677VTgFapZZzg0XZwKlJltVl8138OOOT574I0UEZ6k0fY5WA+2Z8t6zh?=
 =?us-ascii?Q?5aEl3CMqbkopXY4lF9/O1pUPOJVZ8WFngY1tsCJwY54xsmdejaiwe86uTOg7?=
 =?us-ascii?Q?pgaojZ0n4d+axiQqS0Rv4uMNcKSxEv2wCovyGhm8utaMWIknCAh6Uj8PM1kN?=
 =?us-ascii?Q?ABtimjlKLmX+xhLvq92OA7tXPT0jxPapAXzRPOV1ETsMF90BXv3bG5S7pkMA?=
 =?us-ascii?Q?7WsnD4gi6msSJhdMuWciQsjh1XwXex6FELg+xD9piyD9En4L7JmnHDSxn61r?=
 =?us-ascii?Q?279BMgr1peOX2Uq+mMr2+ZmsEMIK3z+mO8W+9vRxPgCVCTDFMBrKwpdBWSOq?=
 =?us-ascii?Q?Zdt6pzfzZUoJtGo0FeqEzTjR2jXmtS1eHuu0+QgByLRn1R8wj+1zkkONMGU9?=
 =?us-ascii?Q?6b77C59VW7w6/KoLYFxDgTWCyhYFK3SvZ1ABWchIlsLXksE/RTrNToTjFYsi?=
 =?us-ascii?Q?yZrQ0ACggdp7faLxyZILthkrWr6mgSriQNDrpRI6pPUY2+XmA7eKY1KcgACT?=
 =?us-ascii?Q?i2Anp1F6FRDDfds0IDI7PoVpZ7Xp2Hxzach7L4QIfb9yyaKtnysnRX22P8Yq?=
 =?us-ascii?Q?dlP5K3rWiy9lGSwiecFIhZQWMZ75sBXXkklQo48LMTRLmrsGH2mAL8jjTqlT?=
 =?us-ascii?Q?xv2Aj7EbncDLqOzlJn50fpFNTlQwIoSAqcJnHwkaZjTaA9u4fL1ft1shet/D?=
 =?us-ascii?Q?Tfck4gLG0RFIby6qbJsUhRloVQBQrPWoYHFuasEpr3yZio8Bu3ZVlRBLXF+y?=
 =?us-ascii?Q?hLLV9gmPnsqCpmrKBFW9pbGpaaSfFEW73O5fjwuqlnrTsYjMqn/ugDIZ81vN?=
 =?us-ascii?Q?Uv1doR+ScyBK5PhNNKyYjlEZ05quJ8yiff6hXouaFYNYYJafbwsP29pD+kKL?=
 =?us-ascii?Q?vUDsjJsyx4QTDfvJFhgf39WzZKJivsm7fUCs0i5yiVLPYSS0XdBtQXwPZ2Km?=
 =?us-ascii?Q?z7oGj2GUhM2DmoGxs24AzJq/r5LEGyIiamgDI0KNTbjOqQSIsMXbzF+oM3lk?=
 =?us-ascii?Q?M+kLJSOe08q9+B/UsNJOB9JUQeJMECZ9QhUvo0T4McexF/E4VhyUB/pb+NAr?=
 =?us-ascii?Q?NaTgRRYp3POXsw8+yYR/WR2jX3OJv0cL8gpjfxe2iGkEWNCqTYi3ycKE8c27?=
 =?us-ascii?Q?AtP073O1BiGQsWAYeMK0KPcOmSn4p7Qyr1gfIbjhxwpLzzBpJOTK2fIwCcRV?=
 =?us-ascii?Q?cLyoGmLlJe38sTQVCTT7P/jzfy3lfEYnUGPN3+AffMdVxYOKvveMD9N23xwv?=
 =?us-ascii?Q?0znDQaiOMfWfS7h7oW3DMrSNElxds4w43BzW9596JxTMB/v+vEC9mIEyKns0?=
 =?us-ascii?Q?ZDgIv2/wH4zSjBo98HW/hsnXn8UNh81JMOv0PqTr+1NuMePcyhP9tIBlORjf?=
 =?us-ascii?Q?sdgibY5dKN02ZFwfR1/ZZBhTRmhcChhOi3cT64FLwqq671Dmuzjf7CjVfQzU?=
 =?us-ascii?Q?ySOTm7zcdPjQSxO0ohX0s9Be3eBCeM3x/PAJ2FimKpxI8iyZ3yL6HFxcB5SQ?=
 =?us-ascii?Q?xsavt1rcCY++0wEPXStNZX9LUc7YFmR5jAzrvO+dMVZjJY9UuSRHwPZtdS6q?=
 =?us-ascii?Q?JA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <C62500715920C1408D6A7672B92E967B@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: jcGdkLwcB0G9gmovkeDfw6dwhP7s/Vx5E8jFDoHFw58qU0Nn+DnJ2sOKjcFMNhwkL95T+nyfEraUHYuw2eNCBAJ00N57Tt0qQbiQ9b6YGuIlJA8mct44qvdXXiI+lQV3svJpZErF10RF79e4h/4rkDUEuC4am2MEjvLE7zYm+WY62DAubzF6XIh5JEvNJryCfKNM70yvSSVPoMUdiP7izb/RQ9V54A8iHCVi5RziwgMdHGOS9OOjbMf5Ic6xZ3hkHJMrI83IFitTVqmzTi/2kZHE23FP2wBPsUqCIM/why5BpQVgipfK3wqvjaVhX++Ureq/KlOm2gREEc984nSUBROfuud0ybshCmVuhQXi4WjsRtvy21p2QMI/Zvg9TyVDRCdpsBt8lozdMfSciZ+pDfMNYBiQEFNfqBqjED52cwgbSa25lvVU+Ec5ojAlshS60yRJZvWsMbm0p/vbja/r7WNOXn60n91RYYnBW5kopTkEtEo9O6YuhjLJpVTGjpXyjkNjkaXdbmjNvPLznX3rGksV9ilEZN8xD+pP4hLCzHg7ismIOthzGnyL6gYYjMc60IQl0MiQiDc6UyDh/NA9skzaj4kNMtXDS2sVjjk0UGkDEvo5Oyup1i5fmIhWMnE+wqGbyPTrhgYFrQ1HQPE8C0W5J33M1x+zjiunX8yEsVLXcXpMWHfz3Han7O8lS1f0RibKmqrVvyzYZyipzFni1xqcVXik8hyrG6Wz1xOMIrZS+xr1I7nQZz1dO2u6p4VxYBK96/cKSejqcwogjgiUDHifXSicFUHkjac+R71XWu0=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d1400cab-fd2c-4c59-adc8-08db4bdf7f23
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 May 2023 14:05:48.5355
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: YoLcACMOH03YMiRw1+cJRTqd4IoVWMuuixIGeK0kuAZdkxnaqRG5cVHoQXM/0vWMtQCs8quZZy6Y+Q0aWNo0Fg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR10MB6661
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.942,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-03_09,2023-05-03_01,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0
 mlxlogscore=999 spamscore=0 phishscore=0 adultscore=0 mlxscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2303200000 definitions=main-2305030119
X-Proofpoint-GUID: ySA_ebg4G-KxKTzNCCgnkSUTbW5IhZsr
X-Proofpoint-ORIG-GUID: ySA_ebg4G-KxKTzNCCgnkSUTbW5IhZsr
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On May 3, 2023, at 6:53 AM, Harshit Mogalapalli <harshit.m.mogalapalli@or=
acle.com> wrote:
>=20
> Hi Christian,
>=20
> On 03/05/23 12:30 pm, Christian Brauner wrote:
>> On Tue, May 02, 2023 at 06:23:51PM +0000, Chuck Lever III wrote:
>>>=20
>>>=20
>>>> On May 2, 2023, at 12:50 PM, Chuck Lever III <chuck.lever@oracle.com> =
wrote:
>>>>=20
>>>>=20
>>>>=20
>>>>> On May 2, 2023, at 9:49 AM, Chuck Lever III <chuck.lever@oracle.com> =
wrote:
>>>>>=20
>>>>>>=20
>>>>>> On May 2, 2023, at 9:36 AM, Christian Brauner <brauner@kernel.org> w=
rote:
>>>>>>=20
>>>>>> We've aligned setgid behavior over multiple kernel releases. The det=
ails
>>>>>> 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 t=
hat
>>>>>> 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.
>>>>>>=20
>>>>>> Without this patch the setgid stripping tests in LTP will fail:
>>>>>>=20
>>>>>>> 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, wh=
ile
>>>>>>=20
>>>>>> [...]
>>>>>>=20
>>>>>>> fchown02.c:66: TFAIL: testfile2: wrong mode permissions 0100700, ex=
pected 0102700
>>>>>>=20
>>>>>> [...]
>>>>>>=20
>>>>>>> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, exp=
ected 0102700
>>>>>>=20
>>>>>> With this patch all tests pass.
>>>>>>=20
>>>>>> Reported-by: Sherry Yang <sherry.yang@oracle.com>
>>>>>> Signed-off-by: Christian Brauner <brauner@kernel.org>
>>>>>=20
>=20
> We had a very similar report from kernel-test-robot.
>=20
> https://lore.kernel.org/all/202210091600.dbe52cbf-yujie.liu@intel.com/
>=20
> Which points to commit: ed5a7047d201 ("attr: use consistent sgid strippin=
g checks")
>=20
> And the above commit is backported to LTS kernels -- 5.10.y,5.15.y and 6.=
1.y
>=20
> So would it be better to tag it to stable by adding "Cc: <stable@vger.ker=
nel.org>" ?
>=20
> Thanks,
> Harshit
>=20
>>>>> There are some similar fstests failures this fix might address.
>>>>>=20
>>>>> I've applied this patch to the nfsd-fixes tree for broader
>>>>> testing.
>>>>=20
>>>> ERROR: modpost: "setattr_should_drop_sgid" [fs/nfsd/nfsd.ko] undefined=
!
>>>>=20
>>>> Did I apply this patch to the wrong kernel?
>>>=20
>>> 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