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:     Neil Brown <neilb@suse.de>
CC:     Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] NFSD: fix regression with setting ACLs.
Thread-Topic: [PATCH] NFSD: fix regression with setting ACLs.
Thread-Index: AQHYwyfx77joTMXnVUySUNNO2fuZsq3WFUIA
Date:   Thu, 8 Sep 2022 21:56:02 +0000
Message-ID: <CC08244F-518D-496D-ACDD-7DBA912C6AAD@oracle.com>
References: <166260292002.30452.4279820246560338475@noble.neil.brown.name>
In-Reply-To: <166260292002.30452.4279820246560338475@noble.neil.brown.name>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?fkgQa2WTWB9zOdqrah+3wZnfyC+wSLJqEjloaPeUqgPwP9F/hcyHfI+By6ye?=
 =?us-ascii?Q?6RApFLkcwODipbK0HsGJ/3QkDHWPjftJBpQ45OHSCzfDGpmk7XsISbqlB+Ee?=
 =?us-ascii?Q?EekyP0U9/NcULaijAkvrDBAEf2+uR0E3Nhl9rInd5Khr7cE6TkGdm36GfeWt?=
 =?us-ascii?Q?hw2IUUzpEwVv24FsunPQCeMzeFHw9kMmP1a+/jE5atCs6/48SfcOAl1G8EO8?=
 =?us-ascii?Q?ScB9VpArC9ZrwrNW0g+oqXraZU9883IpT56MbQjMVETtSDvv4EJjXd3LQ5mc?=
 =?us-ascii?Q?EzxPiYBZ0UbC5FHMKMPyKDOvVor1nlLmWgJMPvL7gjUP19Y/FFOBDSVFiilu?=
 =?us-ascii?Q?N49rLPRhHHqPQHmmaRf9VHlgi/t/5EA6/tNH1BsUVoFqhKnSVB29Dy+A7shq?=
 =?us-ascii?Q?1asarTO8tHBv58HoJb4ZEM5HweIGno992206f0+TI+Yxsfz+5VVPvy8S0p3T?=
 =?us-ascii?Q?4499AJflNIWutUvLraTRUzIbmsyn6QY7gsjfslEnrC4M/4FJIwGu7rjhK5GQ?=
 =?us-ascii?Q?rGndI/v6FLxpT7PfsJKrnJ25IFeh30/e/78Mt5QF0ULxwdJguodK+KblxWbX?=
 =?us-ascii?Q?dhg2R/3Bo3h3EwiHeWPgCC6yMMaTum++u1SAw4yogBHDIlsj0cIv+c/QEY99?=
 =?us-ascii?Q?qjVy9pq/YKHG9f3Y7ZprQ7Lce/XL5ZVgLov6u1+h3Ic/OC/xMoqNc4dTeOku?=
 =?us-ascii?Q?hXbIrr1+BPQtQzpjR6LFTV+E53ZVxN/xx1sO/rUF5AkXYQq5CrSknklpjB9m?=
 =?us-ascii?Q?QYifzxDAqTDBMSr9DjjBfALIdsbhnRfuUpRzakZCbQWQw/Ku0W7PXj4H6fCy?=
 =?us-ascii?Q?KtmfUpfUAFpSYI2Maq8QwF+QeLoFdAl8aNaN+D2URuHuk7GGfqcNjVKcNJkW?=
 =?us-ascii?Q?ntemHKvsFf1epw/qM1lreljpzOmzucJcZ2mCwPaz3TSNkNmBFkmSlBK1Msrm?=
 =?us-ascii?Q?2V58vhHOdXzepHx25mwrSwHB/mALq2iL9y4ju6MBwyVtQfPFU429oE1SjWYW?=
 =?us-ascii?Q?/I5yYdUzLAN18PmVRW9Z/5yBvUhF9LCJO7WqTvxzntanlNzdSs8SLJbn96v6?=
 =?us-ascii?Q?aoaUO7AjjfoBn2QihQqonrIeMCP9ScQ36ZZPuZPd1wWdLiL1zw6fjeoZFeGv?=
 =?us-ascii?Q?rZ2AtmSR1yxHIzmr64d5ybyodBO3sR/XoqO8JmVN2DnR5AoGSsjWxhXMB5DI?=
 =?us-ascii?Q?wEA2jIihqufdkIpbLpFHcil6lJzOJtHdzcRmSy8Fj3hHljPbh+Audj0iGHga?=
 =?us-ascii?Q?v3s+Ub3F2exafxuWtq7NwII4JsN7qEtRWuvVmFFhQidUouZkEtbA/QC6CYAW?=
 =?us-ascii?Q?JDoroT7RD7u5ElQoKPdG+NIZvYxYHxp4MPureYrFRIAnNbZYJO+T/Mijrj/5?=
 =?us-ascii?Q?JBrcQT6Ee/PH3mevrLJ11vgo7YMfDr5pt9kj6e3Wh0/KXK+hlka6i8xhTsGb?=
 =?us-ascii?Q?brigu5F2B0WDvLHpMq6evvRv/YWMNEXszP1l75htTQw2nykJGK7J3SKLTpx+?=
 =?us-ascii?Q?/lGY3dwSv4E9e8NHp+jgbZLBuKgA5hEjf93l6wy9GJwmGdhchpU4iMsZk35n?=
 =?us-ascii?Q?EVKeLWOnFmEZpqq20uy/dw2Dnf0e6vNXUl1NQmThN9QHpwrL4wxGDPjEQYiL?=
 =?us-ascii?Q?bA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4FB41F466085A34BA69B5F5D4CEF6710@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
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: 5b26a7d1-48fb-4bb2-8b2b-08da91e4ec3c
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Sep 2022 21:56:02.8151
 (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: ZBNi3A4qv+ScUnV41+AFqQK2oEjVP3u2IIRYfy00I8HAknFQqGxVYus+djgWZUf01qsHjLZZe5iJi7Er4TIZbw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4546
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-08_12,2022-09-08_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 adultscore=0 malwarescore=0
 mlxscore=0 phishscore=0 mlxlogscore=999 suspectscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2207270000
 definitions=main-2209080078
X-Proofpoint-ORIG-GUID: liLAe7gYoxaTXkeHs-okBYPvjsGB7GN7
X-Proofpoint-GUID: liLAe7gYoxaTXkeHs-okBYPvjsGB7GN7
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 Sep 7, 2022, at 10:08 PM, NeilBrown <neilb@suse.de> wrote:
>=20
>=20
> A recent patch moved ACL setting into nfsd_setattr().
> Unfortunately it didn't work as nfsd_setattr() aborts early if
> iap->ia_valid is 0.
>=20
> Remove this test, and instead avoid calling notify_change() when
> ia_valid is 0.
>=20
> This means that nfsd_setattr() will now *always* lock the inode.
> Previously it didn't if only a ATTR_MODE change was requested on a
> symlink (see Commit 15b7a1b86d66 ("[PATCH] knfsd: fix setattr-on-symlink
> error return")). I don't think this change really matters.
>=20
> Fixes: c0cbe70742f4 ("NFSD: add posix ACLs to struct nfsd_attrs")
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/vfs.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>=20
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 9f486b788ed0..bffb31540df8 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -353,7 +353,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *f=
hp,
> 	int		accmode =3D NFSD_MAY_SATTR;
> 	umode_t		ftype =3D 0;
> 	__be32		err;
> -	int		host_err;
> +	int		host_err =3D 0;
> 	bool		get_write_count;
> 	bool		size_change =3D (iap->ia_valid & ATTR_SIZE);
>=20
> @@ -395,9 +395,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *f=
hp,
> 	if (S_ISLNK(inode->i_mode))
> 		iap->ia_valid &=3D ~ATTR_MODE;

Thank you for the fix, Neil. I've applied this to 6.0-rc.


> -	if (!iap->ia_valid)
> -		return 0;
> -
> 	nfsd_sanitize_attrs(inode, iap);
>=20
> 	if (check_guard && guardtime !=3D inode->i_ctime.tv_sec)
> @@ -448,8 +445,10 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *=
fhp,
> 			goto out_unlock;
> 	}
>=20
> -	iap->ia_valid |=3D ATTR_CTIME;
> -	host_err =3D notify_change(&init_user_ns, dentry, iap, NULL);
> +	if (iap->ia_valid) {
> +		iap->ia_valid |=3D ATTR_CTIME;
> +		host_err =3D notify_change(&init_user_ns, dentry, iap, NULL);
> +	}
>=20
> out_unlock:
> 	if (attr->na_seclabel && attr->na_seclabel->len)
> --=20
> 2.37.1
>=20

--
Chuck Lever