From: Jeff Layton <jlayton@kernel.org>
To: NeilBrown <neilb@suse.de>, Chuck Lever <chuck.lever@oracle.com>
Cc: linux-nfs@vger.kernel.org
Subject: Re: [PATCH 2/8] NFSD: change nfsd_create() to unlock directory before returning.
Date: Wed, 06 Jul 2022 09:24:52 -0400 [thread overview]
Message-ID: <9219d609cd9921742b6c34b2ea7b00e1b6e340df.camel@kernel.org> (raw)
In-Reply-To: <165708109257.1940.11051756818329976731.stgit@noble.brown>
On Wed, 2022-07-06 at 14:18 +1000, NeilBrown wrote:
> nfsd_create() usually exits with the directory still locked. This
> relies on other code to unlock the directory. Planned future patches
> will change how directory locking works so the unlock step may be less
> trivial. It is cleaner to have lock and unlock in the same function.
>
> nfsd4_create() performs some extra changes after the creation and before
> the unlock - setting security label and an ACL. To allow for these to
> still be done while locked, we create a function nfsd4_post_create() and
> pass it to nfsd_create() when needed.
>
> nfsd_symlink() DOES usually unlock the directory, but nfsd4_create() may
> add a label or ACL - with the directory unlocked. I don't think symlinks
> have ACLs and don't know if they can have labels, so I don't know if
> this is of any practical consequence. For consistency nfsd_symlink() is
> changed to accept the same callback and call it if given.
>
> nfsd_symlink() didn't unlock the directory if lookup_one_len() gave an
> error. This is untidy and potentially confusing, and has now been
> fixed. It isn't a practical problem as an eventual fh_put() will unlock
> if needed.
>
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
> fs/nfsd/nfs3proc.c | 11 ++++++-----
> fs/nfsd/nfs4proc.c | 38 ++++++++++++++++++++++++--------------
> fs/nfsd/nfsproc.c | 5 +++--
> fs/nfsd/vfs.c | 40 +++++++++++++++++++++++++++-------------
> fs/nfsd/vfs.h | 11 ++++++++---
> 5 files changed, 68 insertions(+), 37 deletions(-)
>
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 981a3a7a6e16..38255365ef71 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -378,8 +378,8 @@ nfsd3_proc_mkdir(struct svc_rqst *rqstp)
> fh_copy(&resp->dirfh, &argp->fh);
> fh_init(&resp->fh, NFS3_FHSIZE);
> resp->status = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
> - &argp->attrs, S_IFDIR, 0, &resp->fh);
> - fh_unlock(&resp->dirfh);
> + &argp->attrs, S_IFDIR, 0, &resp->fh,
> + NULL, NULL);
> return rpc_success;
> }
>
> @@ -414,7 +414,8 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp)
> fh_copy(&resp->dirfh, &argp->ffh);
> fh_init(&resp->fh, NFS3_FHSIZE);
> resp->status = nfsd_symlink(rqstp, &resp->dirfh, argp->fname,
> - argp->flen, argp->tname, &resp->fh);
> + argp->flen, argp->tname, &resp->fh,
> + NULL, NULL);
> kfree(argp->tname);
> out:
> return rpc_success;
> @@ -453,8 +454,8 @@ nfsd3_proc_mknod(struct svc_rqst *rqstp)
>
> type = nfs3_ftypes[argp->ftype];
> resp->status = nfsd_create(rqstp, &resp->dirfh, argp->name, argp->len,
> - &argp->attrs, type, rdev, &resp->fh);
> - fh_unlock(&resp->dirfh);
> + &argp->attrs, type, rdev, &resp->fh,
> + NULL, NULL);
> out:
> return rpc_success;
> }
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 60591ceb4985..3279daab909d 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -780,6 +780,18 @@ nfsd4_commit(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> (__be32 *)commit->co_verf.data);
> }
>
> +static void nfsd4_post_create(struct svc_fh *fh, void *vcreate)
> +{
> + struct nfsd4_create *create = vcreate;
> +
> + if (create->cr_label.len)
> + nfsd4_security_inode_setsecctx(fh, &create->cr_label,
> + create->cr_bmval);
> +
> + if (create->cr_acl != NULL)
> + do_set_nfs4_acl(fh, create->cr_acl, create->cr_bmval);
> +}
> +
> static __be32
> nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> union nfsd4_op_u *u)
> @@ -805,7 +817,8 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> case NF4LNK:
> status = nfsd_symlink(rqstp, &cstate->current_fh,
> create->cr_name, create->cr_namelen,
> - create->cr_data, &resfh);
> + create->cr_data, &resfh,
> + nfsd4_post_create, create);
> break;
>
> case NF4BLK:
> @@ -816,7 +829,8 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> goto out_umask;
> status = nfsd_create(rqstp, &cstate->current_fh,
> create->cr_name, create->cr_namelen,
> - &create->cr_iattr, S_IFBLK, rdev, &resfh);
> + &create->cr_iattr, S_IFBLK, rdev, &resfh,
> + nfsd4_post_create, create);
> break;
>
> case NF4CHR:
> @@ -827,26 +841,30 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> goto out_umask;
> status = nfsd_create(rqstp, &cstate->current_fh,
> create->cr_name, create->cr_namelen,
> - &create->cr_iattr, S_IFCHR, rdev, &resfh);
> + &create->cr_iattr, S_IFCHR, rdev, &resfh,
> + nfsd4_post_create, create);
> break;
>
> case NF4SOCK:
> status = nfsd_create(rqstp, &cstate->current_fh,
> create->cr_name, create->cr_namelen,
> - &create->cr_iattr, S_IFSOCK, 0, &resfh);
> + &create->cr_iattr, S_IFSOCK, 0, &resfh,
> + nfsd4_post_create, create);
> break;
>
> case NF4FIFO:
> status = nfsd_create(rqstp, &cstate->current_fh,
> create->cr_name, create->cr_namelen,
> - &create->cr_iattr, S_IFIFO, 0, &resfh);
> + &create->cr_iattr, S_IFIFO, 0, &resfh,
> + nfsd4_post_create, create);
> break;
>
> case NF4DIR:
> create->cr_iattr.ia_valid &= ~ATTR_SIZE;
> status = nfsd_create(rqstp, &cstate->current_fh,
> create->cr_name, create->cr_namelen,
> - &create->cr_iattr, S_IFDIR, 0, &resfh);
> + &create->cr_iattr, S_IFDIR, 0, &resfh,
> + nfsd4_post_create, create);
> break;
>
> default:
> @@ -856,14 +874,6 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> if (status)
> goto out;
>
> - if (create->cr_label.len)
> - nfsd4_security_inode_setsecctx(&resfh, &create->cr_label, create->cr_bmval);
> -
> - if (create->cr_acl != NULL)
> - do_set_nfs4_acl(&resfh, create->cr_acl,
> - create->cr_bmval);
> -
> - fh_unlock(&cstate->current_fh);
> set_change_info(&create->cr_cinfo, &cstate->current_fh);
> fh_dup2(&cstate->current_fh, &resfh);
> out:
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index fcdab8a8a41f..a25b8e321662 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -493,7 +493,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp)
>
> fh_init(&newfh, NFS_FHSIZE);
> resp->status = nfsd_symlink(rqstp, &argp->ffh, argp->fname, argp->flen,
> - argp->tname, &newfh);
> + argp->tname, &newfh, NULL, NULL);
>
> kfree(argp->tname);
> fh_put(&argp->ffh);
> @@ -522,7 +522,8 @@ nfsd_proc_mkdir(struct svc_rqst *rqstp)
> argp->attrs.ia_valid &= ~ATTR_SIZE;
> fh_init(&resp->fh, NFS_FHSIZE);
> resp->status = nfsd_create(rqstp, &argp->fh, argp->name, argp->len,
> - &argp->attrs, S_IFDIR, 0, &resp->fh);
> + &argp->attrs, S_IFDIR, 0, &resp->fh,
> + NULL, NULL);
> fh_put(&argp->fh);
> if (resp->status != nfs_ok)
> goto out;
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index d79db56475d4..1e7ca39e8a49 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1366,8 +1366,10 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
> */
> __be32
> nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> - char *fname, int flen, struct iattr *iap,
> - int type, dev_t rdev, struct svc_fh *resfhp)
> + char *fname, int flen, struct iattr *iap,
> + int type, dev_t rdev, struct svc_fh *resfhp,
> + void (*post_create)(struct svc_fh *fh, void *data),
> + void *data)
> {
> struct dentry *dentry, *dchild = NULL;
> __be32 err;
> @@ -1389,8 +1391,10 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> fh_lock_nested(fhp, I_MUTEX_PARENT);
> dchild = lookup_one_len(fname, dentry, flen);
> host_err = PTR_ERR(dchild);
> - if (IS_ERR(dchild))
> - return nfserrno(host_err);
> + if (IS_ERR(dchild)) {
> + err = nfserrno(host_err);
> + goto out_unlock;
> + }
> err = fh_compose(resfhp, fhp->fh_export, dchild, fhp);
> /*
> * We unconditionally drop our ref to dchild as fh_compose will have
> @@ -1398,9 +1402,14 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> */
> dput(dchild);
> if (err)
> - return err;
> - return nfsd_create_locked(rqstp, fhp, fname, flen, iap, type,
> - rdev, resfhp);
> + goto out_unlock;
> + err = nfsd_create_locked(rqstp, fhp, fname, flen, iap, type,
> + rdev, resfhp);
> + if (!err && post_create)
> + post_create(resfhp, data);
> +out_unlock:
> + fh_unlock(fhp);
> + return err;
> }
>
> /*
> @@ -1447,9 +1456,11 @@ nfsd_readlink(struct svc_rqst *rqstp, struct svc_fh *fhp, char *buf, int *lenp)
> */
> __be32
> nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> - char *fname, int flen,
> - char *path,
> - struct svc_fh *resfhp)
> + char *fname, int flen,
> + char *path,
> + struct svc_fh *resfhp,
> + void (*post_create)(struct svc_fh *fh, void *data),
> + void *data)
> {
> struct dentry *dentry, *dnew;
> __be32 err, cerr;
> @@ -1474,12 +1485,12 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> dentry = fhp->fh_dentry;
> dnew = lookup_one_len(fname, dentry, flen);
> host_err = PTR_ERR(dnew);
> - if (IS_ERR(dnew))
> + if (IS_ERR(dnew)) {
> + fh_unlock(fhp);
> goto out_nfserr;
> -
> + }
> host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
> err = nfserrno(host_err);
> - fh_unlock(fhp);
> if (!err)
> err = nfserrno(commit_metadata(fhp));
>
> @@ -1488,6 +1499,9 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
> cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp);
> dput(dnew);
> if (err==0) err = cerr;
> + if (!err && post_create)
> + post_create(resfhp, data);
> + fh_unlock(fhp);
> out:
> return err;
>
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 26347d76f44a..9f4fd3060200 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -66,8 +66,10 @@ __be32 nfsd_create_locked(struct svc_rqst *, struct svc_fh *,
> char *name, int len, struct iattr *attrs,
> int type, dev_t rdev, struct svc_fh *res);
> __be32 nfsd_create(struct svc_rqst *, struct svc_fh *,
> - char *name, int len, struct iattr *attrs,
> - int type, dev_t rdev, struct svc_fh *res);
> + char *name, int len, struct iattr *attrs,
> + int type, dev_t rdev, struct svc_fh *res,
> + void (*post_create)(struct svc_fh *fh, void *data),
> + void *data);
> __be32 nfsd_access(struct svc_rqst *, struct svc_fh *, u32 *, u32 *);
> __be32 nfsd_create_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> struct svc_fh *resfhp, struct iattr *iap);
> @@ -111,7 +113,10 @@ __be32 nfsd_readlink(struct svc_rqst *, struct svc_fh *,
> char *, int *);
> __be32 nfsd_symlink(struct svc_rqst *, struct svc_fh *,
> char *name, int len, char *path,
> - struct svc_fh *res);
> + struct svc_fh *res,
> + void (*post_create)(struct svc_fh *fh,
> + void *data),
> + void *data);
> __be32 nfsd_link(struct svc_rqst *, struct svc_fh *,
> char *, int, struct svc_fh *);
> ssize_t nfsd_copy_file_range(struct file *, u64,
>
>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
next prev parent reply other threads:[~2022-07-06 13:25 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-06 4:18 [PATCH 0/8] NFSD: clean up locking NeilBrown
2022-07-06 4:18 ` [PATCH 1/8] NFSD: drop rqstp arg to do_set_nfs4_acl() NeilBrown
2022-07-06 13:17 ` Jeff Layton
2022-07-06 4:18 ` [PATCH 4/8] NFSD: only call fh_unlock() once in nfsd_link() NeilBrown
2022-07-06 13:31 ` Jeff Layton
2022-07-06 16:29 ` Chuck Lever III
2022-07-06 4:18 ` [PATCH 8/8] NFSD: discard fh_locked flag and fh_lock/fh_unlock NeilBrown
2022-07-06 14:12 ` Jeff Layton
2022-07-06 4:18 ` [PATCH 7/8] NFSD: use (un)lock_inode instead of fh_(un)lock for file operations NeilBrown
2022-07-06 14:10 ` Jeff Layton
2022-07-06 16:30 ` Chuck Lever III
2022-07-07 1:33 ` NeilBrown
2022-07-06 4:18 ` [PATCH 2/8] NFSD: change nfsd_create() to unlock directory before returning NeilBrown
2022-07-06 13:24 ` Jeff Layton [this message]
2022-07-06 16:29 ` Chuck Lever III
2022-07-06 4:18 ` [PATCH 6/8] NFSD: use explicit lock/unlock for directory ops NeilBrown
2022-07-06 14:05 ` Jeff Layton
2022-07-06 16:29 ` Chuck Lever III
2022-07-15 16:11 ` Jeff Layton
2022-07-15 18:22 ` Jeff Layton
2022-07-17 23:59 ` NeilBrown
2022-07-17 23:43 ` NeilBrown
2022-07-06 4:18 ` [PATCH 5/8] NFSD: reduce locking in nfsd_lookup() NeilBrown
2022-07-06 13:47 ` Jeff Layton
2022-07-07 1:26 ` NeilBrown
2022-07-06 16:29 ` Chuck Lever III
2022-07-07 1:29 ` NeilBrown
2022-07-06 4:18 ` [PATCH 3/8] NFSD: always drop directory lock in nfsd_unlink() NeilBrown
2022-07-06 13:30 ` Jeff Layton
2022-07-06 16:29 ` [PATCH 0/8] NFSD: clean up locking Chuck Lever III
2022-07-12 2:33 ` NeilBrown
2022-07-12 14:17 ` Chuck Lever III
2022-07-13 4:32 ` NeilBrown
2022-07-13 14:15 ` Chuck Lever III
2022-07-13 19:13 ` Jeff Layton
2022-07-14 14:32 ` Chuck Lever III
2022-07-15 2:36 ` NeilBrown
2022-07-15 13:01 ` Jeff Layton
2022-07-15 13:53 ` Jeff Layton
2022-07-15 14:06 ` Chuck Lever III
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9219d609cd9921742b6c34b2ea7b00e1b6e340df.camel@kernel.org \
--to=jlayton@kernel.org \
--cc=chuck.lever@oracle.com \
--cc=linux-nfs@vger.kernel.org \
--cc=neilb@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox