2022-09-08 02:26:29

by NeilBrown

Subject: [PATCH] NFSD: fix regression with setting ACLs.

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.

Remove this test, and instead avoid calling notify_change() when
ia_valid is 0.

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.

Fixes: c0cbe70742f4 ("NFSD: add posix ACLs to struct nfsd_attrs")
Signed-off-by: NeilBrown <[email protected]>
---
fs/nfsd/vfs.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

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 *fhp,
int accmode = NFSD_MAY_SATTR;
umode_t ftype = 0;
__be32 err;
- int host_err;
+ int host_err = 0;
bool get_write_count;
bool size_change = (iap->ia_valid & ATTR_SIZE);

@@ -395,9 +395,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
if (S_ISLNK(inode->i_mode))
iap->ia_valid &= ~ATTR_MODE;

- if (!iap->ia_valid)
- return 0;
-
nfsd_sanitize_attrs(inode, iap);

if (check_guard && guardtime != inode->i_ctime.tv_sec)
@@ -448,8 +445,10 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
goto out_unlock;
}

- iap->ia_valid |= ATTR_CTIME;
- host_err = notify_change(&init_user_ns, dentry, iap, NULL);
+ if (iap->ia_valid) {
+ iap->ia_valid |= ATTR_CTIME;
+ host_err = notify_change(&init_user_ns, dentry, iap, NULL);
+ }

out_unlock:
if (attr->na_seclabel && attr->na_seclabel->len)
--
2.37.1

2022-09-08 11:18:10

by Jeff Layton

Subject: Re: [PATCH] NFSD: fix regression with setting ACLs.

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.
>
> Remove this test, and instead avoid calling notify_change() when
> ia_valid is 0.
>
> 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.
>
> Fixes: c0cbe70742f4 ("NFSD: add posix ACLs to struct nfsd_attrs")
> Signed-off-by: NeilBrown <[email protected]>
> ---
> fs/nfsd/vfs.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> 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 *fhp,
> int accmode = NFSD_MAY_SATTR;
> umode_t ftype = 0;
> __be32 err;
> - int host_err;
> + int host_err = 0;
> bool get_write_count;
> bool size_change = (iap->ia_valid & ATTR_SIZE);
>
> @@ -395,9 +395,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (S_ISLNK(inode->i_mode))
> iap->ia_valid &= ~ATTR_MODE;
>
> - if (!iap->ia_valid)
> - return 0;
> -
> nfsd_sanitize_attrs(inode, iap);
>
> if (check_guard && guardtime != inode->i_ctime.tv_sec)
> @@ -448,8 +445,10 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> goto out_unlock;
> }
>
> - iap->ia_valid |= ATTR_CTIME;
> - host_err = notify_change(&init_user_ns, dentry, iap, NULL);
> + if (iap->ia_valid) {
> + iap->ia_valid |= ATTR_CTIME;
> + host_err = notify_change(&init_user_ns, dentry, iap, NULL);
> + }
>
> out_unlock:
> if (attr->na_seclabel && attr->na_seclabel->len)

Looks sane.

Reviewed-by: Jeff Layton <[email protected]>

2022-09-08 21:58:33

by Chuck Lever III

Subject: Re: [PATCH] NFSD: fix regression with setting ACLs.

> On Sep 7, 2022, at 10:08 PM, NeilBrown <[email protected]> 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.
>
> Remove this test, and instead avoid calling notify_change() when
> ia_valid is 0.
>
> 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.
>
> Fixes: c0cbe70742f4 ("NFSD: add posix ACLs to struct nfsd_attrs")
> Signed-off-by: NeilBrown <[email protected]>
> ---
> fs/nfsd/vfs.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> 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 *fhp,
> int accmode = NFSD_MAY_SATTR;
> umode_t ftype = 0;
> __be32 err;
> - int host_err;
> + int host_err = 0;
> bool get_write_count;
> bool size_change = (iap->ia_valid & ATTR_SIZE);
>
> @@ -395,9 +395,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> if (S_ISLNK(inode->i_mode))
> iap->ia_valid &= ~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);
>
> if (check_guard && guardtime != inode->i_ctime.tv_sec)
> @@ -448,8 +445,10 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> goto out_unlock;
> }
>
> - iap->ia_valid |= ATTR_CTIME;
> - host_err = notify_change(&init_user_ns, dentry, iap, NULL);
> + if (iap->ia_valid) {
> + iap->ia_valid |= ATTR_CTIME;
> + host_err = notify_change(&init_user_ns, dentry, iap, NULL);
> + }
>
> out_unlock:
> if (attr->na_seclabel && attr->na_seclabel->len)
> --
> 2.37.1
>

--
Chuck Lever