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;
Message-ID: <d2150b0c17ace9fee06f75a4215f08e187227065.camel@kernel.org>
Subject: Re: [PATCH] NFSD: fix regression with setting ACLs.
From:   Jeff Layton <jlayton@kernel.org>
To:     NeilBrown <neilb@suse.de>,
        Chuck Lever III <chuck.lever@oracle.com>,
        Bruce Fields <bfields@fieldses.org>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Thu, 08 Sep 2022 07:15:25 -0400
In-Reply-To: <166260292002.30452.4279820246560338475@noble.neil.brown.name>
References: <166260292002.30452.4279820246560338475@noble.neil.brown.name>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2022-09-08 at 12:08 +1000, NeilBrown wrote:
> 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;
> =20
> -	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)

Looks sane.

Reviewed-by: Jeff Layton <jlayton@kernel.org>