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@xxxxxxx> > --- > 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@xxxxxxxxxx>