| Message ID | 165881793056.21666.12904500892707412393.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: clean up locking | expand |
> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote: > > The NFS protocol includes attributes when creating symlinks. > Linux does store attributes for symlinks and allows them to be set, > though they are not used for permission checking. > > NFSD currently doesn't set standard (struct iattr) attributes when > creating symlinks, but for NFSv4 it does set ACLs and security labels. > This is inconsistent. > > To improve consistency, pass the provided attributes into nfsd_symlink() > and call nfsd_create_setattr() to set them. > > We ignore any error from nfsd_create_setattr(). It isn't really clear > what should be done if a file is successfully created, but the > attributes cannot be set. NFS doesn't allow partial success to be > reported. Reporting failure is probably more misleading than reporting > success, so the status is ignored. > > Signed-off-by: NeilBrown <neilb@suse.de> While I was trying to get more information about how security labels are supposed to be applied to symlinks, Jeff, Bruce, and I had a brief private discussion about it. Jeff has the impression that NFSD should not apply ACLs/security labels to symlinks, and we believe that would be somewhat of a change in behavior. Now is a good time to hoist that private discussion to the list, so I'm mentioning it here. I'm thinking it would be appropriate to introduce that change in this series, and probably right here in this patch, if we agree that is the right thing to do going forward. Otherwise, after a brief glance at the series, it looks better to me than the previous approach. I will try to dig in later this week so we can get this work merged at the end of the upcoming merge window. > --- > fs/nfsd/nfs3proc.c | 3 ++- > fs/nfsd/nfs4proc.c | 2 +- > fs/nfsd/nfsproc.c | 3 ++- > fs/nfsd/vfs.c | 11 ++++++----- > fs/nfsd/vfs.h | 5 +++-- > 5 files changed, 14 insertions(+), 10 deletions(-) > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > index 289eb844d086..5e369096e42f 100644 > --- a/fs/nfsd/nfs3proc.c > +++ b/fs/nfsd/nfs3proc.c > @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp) > { > struct nfsd3_symlinkargs *argp = rqstp->rq_argp; > struct nfsd3_diropres *resp = rqstp->rq_resp; > + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; > > if (argp->tlen == 0) { > resp->status = nfserr_inval; > @@ -417,7 +418,7 @@ 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, &attrs, &resp->fh); > kfree(argp->tname); > out: > return rpc_success; > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index ba750d76f515..ee72c94732f0 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -809,7 +809,7 @@ 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, &attrs, &resfh); > break; > > case NF4BLK: > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > index 594d6f85c89f..d09d516188d2 100644 > --- a/fs/nfsd/nfsproc.c > +++ b/fs/nfsd/nfsproc.c > @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp) > { > struct nfsd_symlinkargs *argp = rqstp->rq_argp; > struct nfsd_stat *resp = rqstp->rq_resp; > + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; > struct svc_fh newfh; > > if (argp->tlen > NFS_MAXPATHLEN) { > @@ -495,7 +496,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, &attrs, &newfh); > > kfree(argp->tname); > fh_put(&argp->ffh); > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index a85dc4dd4f3a..91c9ea09f921 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1451,9 +1451,9 @@ 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 nfsd_attrs *attrs, > + struct svc_fh *resfhp) > { > struct dentry *dentry, *dnew; > __be32 err, cerr; > @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > > host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); > err = nfserrno(host_err); > + cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); > + if (!err) > + nfsd_create_setattr(rqstp, fhp, resfhp, attrs); > fh_unlock(fhp); > if (!err) > err = nfserrno(commit_metadata(fhp)); > - > fh_drop_write(fhp); > > - cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); > dput(dnew); > if (err==0) err = cerr; > out: > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > index 9bb0e3957982..f3f43ca3ac6b 100644 > --- a/fs/nfsd/vfs.h > +++ b/fs/nfsd/vfs.h > @@ -114,8 +114,9 @@ __be32 nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, > __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); > + char *name, int len, char *path, > + struct nfsd_attrs *attrs, > + struct svc_fh *res); > __be32 nfsd_link(struct svc_rqst *, struct svc_fh *, > char *, int, struct svc_fh *); > ssize_t nfsd_copy_file_range(struct file *, u64, > > -- Chuck Lever
On Tue, 2022-07-26 at 12:40 +0000, Chuck Lever III wrote: > > > On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote: > > > > The NFS protocol includes attributes when creating symlinks. > > Linux does store attributes for symlinks and allows them to be set, > > though they are not used for permission checking. > > > > NFSD currently doesn't set standard (struct iattr) attributes when > > creating symlinks, but for NFSv4 it does set ACLs and security labels. > > This is inconsistent. > > > > To improve consistency, pass the provided attributes into nfsd_symlink() > > and call nfsd_create_setattr() to set them. > > > > We ignore any error from nfsd_create_setattr(). It isn't really clear > > what should be done if a file is successfully created, but the > > attributes cannot be set. NFS doesn't allow partial success to be > > reported. Reporting failure is probably more misleading than reporting > > success, so the status is ignored. > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > While I was trying to get more information about how > security labels are supposed to be applied to symlinks, > Jeff, Bruce, and I had a brief private discussion about > it. Jeff has the impression that NFSD should not apply > ACLs/security labels to symlinks, and we believe that > would be somewhat of a change in behavior. > > Now is a good time to hoist that private discussion to > the list, so I'm mentioning it here. I'm thinking it > would be appropriate to introduce that change in this > series, and probably right here in this patch, if we > agree that is the right thing to do going forward. > > Otherwise, after a brief glance at the series, it looks > better to me than the previous approach. I will try to > dig in later this week so we can get this work merged at > the end of the upcoming merge window. > My take was a bit more nuanced... ;) The kernel clearly doesn't do any mode/acl/label enforcement when traversing symlinks, but I think we do at least need to allow security labels to be set on them. We can set labels on symlinks on local filesystems. Installing software in (e.g.) SELinux labeled environments might go awry if we don't allow this. It could also make a difference in who is allowed to unlink them too. NFSv4 ACLs are more ambiguous. It's probably not terribly harmful to allow them to be set on symlinks, but the value of doing so is not clear. We could probably just ignore them and no one would care. OTOH, maybe DELETE permission actually matters here? Do we enforce that in any way today? These might be good questions to take to the IETF list... > > > --- > > fs/nfsd/nfs3proc.c | 3 ++- > > fs/nfsd/nfs4proc.c | 2 +- > > fs/nfsd/nfsproc.c | 3 ++- > > fs/nfsd/vfs.c | 11 ++++++----- > > fs/nfsd/vfs.h | 5 +++-- > > 5 files changed, 14 insertions(+), 10 deletions(-) > > > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > > index 289eb844d086..5e369096e42f 100644 > > --- a/fs/nfsd/nfs3proc.c > > +++ b/fs/nfsd/nfs3proc.c > > @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp) > > { > > struct nfsd3_symlinkargs *argp = rqstp->rq_argp; > > struct nfsd3_diropres *resp = rqstp->rq_resp; > > + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; > > > > if (argp->tlen == 0) { > > resp->status = nfserr_inval; > > @@ -417,7 +418,7 @@ 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, &attrs, &resp->fh); > > kfree(argp->tname); > > out: > > return rpc_success; > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > index ba750d76f515..ee72c94732f0 100644 > > --- a/fs/nfsd/nfs4proc.c > > +++ b/fs/nfsd/nfs4proc.c > > @@ -809,7 +809,7 @@ 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, &attrs, &resfh); > > break; > > > > case NF4BLK: > > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > > index 594d6f85c89f..d09d516188d2 100644 > > --- a/fs/nfsd/nfsproc.c > > +++ b/fs/nfsd/nfsproc.c > > @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp) > > { > > struct nfsd_symlinkargs *argp = rqstp->rq_argp; > > struct nfsd_stat *resp = rqstp->rq_resp; > > + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; > > struct svc_fh newfh; > > > > if (argp->tlen > NFS_MAXPATHLEN) { > > @@ -495,7 +496,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, &attrs, &newfh); > > > > kfree(argp->tname); > > fh_put(&argp->ffh); > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > > index a85dc4dd4f3a..91c9ea09f921 100644 > > --- a/fs/nfsd/vfs.c > > +++ b/fs/nfsd/vfs.c > > @@ -1451,9 +1451,9 @@ 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 nfsd_attrs *attrs, > > + struct svc_fh *resfhp) > > { > > struct dentry *dentry, *dnew; > > __be32 err, cerr; > > @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > > host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); > > err = nfserrno(host_err); > > + cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); > > + if (!err) > > + nfsd_create_setattr(rqstp, fhp, resfhp, attrs); > > fh_unlock(fhp); > > if (!err) > > err = nfserrno(commit_metadata(fhp)); > > - > > fh_drop_write(fhp); > > > > - cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); > > dput(dnew); > > if (err==0) err = cerr; > > out: > > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > > index 9bb0e3957982..f3f43ca3ac6b 100644 > > --- a/fs/nfsd/vfs.h > > +++ b/fs/nfsd/vfs.h > > @@ -114,8 +114,9 @@ __be32 nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, > > __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); > > + char *name, int len, char *path, > > + struct nfsd_attrs *attrs, > > + struct svc_fh *res); > > __be32 nfsd_link(struct svc_rqst *, struct svc_fh *, > > char *, int, struct svc_fh *); > > ssize_t nfsd_copy_file_range(struct file *, u64, > > > > > > -- > Chuck Lever > > >
> On Jul 26, 2022, at 8:53 AM, Jeff Layton <jlayton@kernel.org> wrote: > > On Tue, 2022-07-26 at 12:40 +0000, Chuck Lever III wrote: >> >>> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote: >>> >>> The NFS protocol includes attributes when creating symlinks. >>> Linux does store attributes for symlinks and allows them to be set, >>> though they are not used for permission checking. >>> >>> NFSD currently doesn't set standard (struct iattr) attributes when >>> creating symlinks, but for NFSv4 it does set ACLs and security labels. >>> This is inconsistent. >>> >>> To improve consistency, pass the provided attributes into nfsd_symlink() >>> and call nfsd_create_setattr() to set them. >>> >>> We ignore any error from nfsd_create_setattr(). It isn't really clear >>> what should be done if a file is successfully created, but the >>> attributes cannot be set. NFS doesn't allow partial success to be >>> reported. Reporting failure is probably more misleading than reporting >>> success, so the status is ignored. >>> >>> Signed-off-by: NeilBrown <neilb@suse.de> >> >> While I was trying to get more information about how >> security labels are supposed to be applied to symlinks, >> Jeff, Bruce, and I had a brief private discussion about >> it. Jeff has the impression that NFSD should not apply >> ACLs/security labels to symlinks, and we believe that >> would be somewhat of a change in behavior. >> >> Now is a good time to hoist that private discussion to >> the list, so I'm mentioning it here. I'm thinking it >> would be appropriate to introduce that change in this >> series, and probably right here in this patch, if we >> agree that is the right thing to do going forward. >> >> Otherwise, after a brief glance at the series, it looks >> better to me than the previous approach. I will try to >> dig in later this week so we can get this work merged at >> the end of the upcoming merge window. >> > > My take was a bit more nuanced... ;) I was summarizing. Please feel free to correct me. > The kernel clearly doesn't do any mode/acl/label enforcement when > traversing symlinks, but I think we do at least need to allow security > labels to be set on them. We can set labels on symlinks on local > filesystems. Installing software in (e.g.) SELinux labeled environments > might go awry if we don't allow this. It could also make a difference in > who is allowed to unlink them too. > > NFSv4 ACLs are more ambiguous. It's probably not terribly harmful to > allow them to be set on symlinks, but the value of doing so is not > clear. We could probably just ignore them and no one would care. OTOH, > maybe DELETE permission actually matters here? Do we enforce that in any > way today? Sure -- we can always punt on making behavior changes now, and simply continue to support what has been supported. I'm not aware of many regression or functional tests in this area, so it's going to be difficult to proceed with modifications that alter behavior. > These might be good questions to take to the IETF list... Agreed. I made some informal inquiries to a few folks directly, but asking publicly wouldn't hurt. It also wouldn't hurt to ask implementors of other NFSv4 implementations. >>> --- >>> fs/nfsd/nfs3proc.c | 3 ++- >>> fs/nfsd/nfs4proc.c | 2 +- >>> fs/nfsd/nfsproc.c | 3 ++- >>> fs/nfsd/vfs.c | 11 ++++++----- >>> fs/nfsd/vfs.h | 5 +++-- >>> 5 files changed, 14 insertions(+), 10 deletions(-) >>> >>> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c >>> index 289eb844d086..5e369096e42f 100644 >>> --- a/fs/nfsd/nfs3proc.c >>> +++ b/fs/nfsd/nfs3proc.c >>> @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp) >>> { >>> struct nfsd3_symlinkargs *argp = rqstp->rq_argp; >>> struct nfsd3_diropres *resp = rqstp->rq_resp; >>> + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; >>> >>> if (argp->tlen == 0) { >>> resp->status = nfserr_inval; >>> @@ -417,7 +418,7 @@ 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, &attrs, &resp->fh); >>> kfree(argp->tname); >>> out: >>> return rpc_success; >>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c >>> index ba750d76f515..ee72c94732f0 100644 >>> --- a/fs/nfsd/nfs4proc.c >>> +++ b/fs/nfsd/nfs4proc.c >>> @@ -809,7 +809,7 @@ 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, &attrs, &resfh); >>> break; >>> >>> case NF4BLK: >>> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c >>> index 594d6f85c89f..d09d516188d2 100644 >>> --- a/fs/nfsd/nfsproc.c >>> +++ b/fs/nfsd/nfsproc.c >>> @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp) >>> { >>> struct nfsd_symlinkargs *argp = rqstp->rq_argp; >>> struct nfsd_stat *resp = rqstp->rq_resp; >>> + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; >>> struct svc_fh newfh; >>> >>> if (argp->tlen > NFS_MAXPATHLEN) { >>> @@ -495,7 +496,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, &attrs, &newfh); >>> >>> kfree(argp->tname); >>> fh_put(&argp->ffh); >>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c >>> index a85dc4dd4f3a..91c9ea09f921 100644 >>> --- a/fs/nfsd/vfs.c >>> +++ b/fs/nfsd/vfs.c >>> @@ -1451,9 +1451,9 @@ 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 nfsd_attrs *attrs, >>> + struct svc_fh *resfhp) >>> { >>> struct dentry *dentry, *dnew; >>> __be32 err, cerr; >>> @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, >>> >>> host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); >>> err = nfserrno(host_err); >>> + cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); >>> + if (!err) >>> + nfsd_create_setattr(rqstp, fhp, resfhp, attrs); >>> fh_unlock(fhp); >>> if (!err) >>> err = nfserrno(commit_metadata(fhp)); >>> - >>> fh_drop_write(fhp); >>> >>> - cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); >>> dput(dnew); >>> if (err==0) err = cerr; >>> out: >>> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h >>> index 9bb0e3957982..f3f43ca3ac6b 100644 >>> --- a/fs/nfsd/vfs.h >>> +++ b/fs/nfsd/vfs.h >>> @@ -114,8 +114,9 @@ __be32 nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, >>> __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); >>> + char *name, int len, char *path, >>> + struct nfsd_attrs *attrs, >>> + struct svc_fh *res); >>> __be32 nfsd_link(struct svc_rqst *, struct svc_fh *, >>> char *, int, struct svc_fh *); >>> ssize_t nfsd_copy_file_range(struct file *, u64, >>> >>> >> >> -- >> Chuck Lever >> >> >> > > -- > Jeff Layton <jlayton@kernel.org> -- Chuck Lever
On Tue, 26 Jul 2022, Chuck Lever III wrote: > > > On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote: > > > > The NFS protocol includes attributes when creating symlinks. > > Linux does store attributes for symlinks and allows them to be set, > > though they are not used for permission checking. > > > > NFSD currently doesn't set standard (struct iattr) attributes when > > creating symlinks, but for NFSv4 it does set ACLs and security labels. > > This is inconsistent. > > > > To improve consistency, pass the provided attributes into nfsd_symlink() > > and call nfsd_create_setattr() to set them. > > > > We ignore any error from nfsd_create_setattr(). It isn't really clear > > what should be done if a file is successfully created, but the > > attributes cannot be set. NFS doesn't allow partial success to be > > reported. Reporting failure is probably more misleading than reporting > > success, so the status is ignored. > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > While I was trying to get more information about how > security labels are supposed to be applied to symlinks, > Jeff, Bruce, and I had a brief private discussion about > it. Jeff has the impression that NFSD should not apply > ACLs/security labels to symlinks, and we believe that > would be somewhat of a change in behavior. > > Now is a good time to hoist that private discussion to > the list, so I'm mentioning it here. I'm thinking it > would be appropriate to introduce that change in this > series, and probably right here in this patch, if we > agree that is the right thing to do going forward. One option would be to avoid changing behaviour at this point. That could be achieved by a one-line change in nfsd_symlink() in this patch to clear the ia_valid field. That way iattr attrs would still not be set on symlinks, but ACLs/security labels would. As and when we change our mind on what we *should* be doing, any such change can then happen right there in nfsd_symlink() by making modifications to the nfsd_attrs content, so it would be explicit in the code. My feeling on policy is that it is our job to implement the NFSv4 protocol. If the protocol sends attributes, and if the filesystem allows them to be set, when who are we to stand in the way? I appreciate that might be an overly-narrow perspective. > > Otherwise, after a brief glance at the series, it looks > better to me than the previous approach. I will try to > dig in later this week so we can get this work merged at > the end of the upcoming merge window. Thanks, NeilBrown > > > > --- > > fs/nfsd/nfs3proc.c | 3 ++- > > fs/nfsd/nfs4proc.c | 2 +- > > fs/nfsd/nfsproc.c | 3 ++- > > fs/nfsd/vfs.c | 11 ++++++----- > > fs/nfsd/vfs.h | 5 +++-- > > 5 files changed, 14 insertions(+), 10 deletions(-) > > > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > > index 289eb844d086..5e369096e42f 100644 > > --- a/fs/nfsd/nfs3proc.c > > +++ b/fs/nfsd/nfs3proc.c > > @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp) > > { > > struct nfsd3_symlinkargs *argp = rqstp->rq_argp; > > struct nfsd3_diropres *resp = rqstp->rq_resp; > > + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; > > > > if (argp->tlen == 0) { > > resp->status = nfserr_inval; > > @@ -417,7 +418,7 @@ 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, &attrs, &resp->fh); > > kfree(argp->tname); > > out: > > return rpc_success; > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > index ba750d76f515..ee72c94732f0 100644 > > --- a/fs/nfsd/nfs4proc.c > > +++ b/fs/nfsd/nfs4proc.c > > @@ -809,7 +809,7 @@ 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, &attrs, &resfh); > > break; > > > > case NF4BLK: > > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > > index 594d6f85c89f..d09d516188d2 100644 > > --- a/fs/nfsd/nfsproc.c > > +++ b/fs/nfsd/nfsproc.c > > @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp) > > { > > struct nfsd_symlinkargs *argp = rqstp->rq_argp; > > struct nfsd_stat *resp = rqstp->rq_resp; > > + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; > > struct svc_fh newfh; > > > > if (argp->tlen > NFS_MAXPATHLEN) { > > @@ -495,7 +496,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, &attrs, &newfh); > > > > kfree(argp->tname); > > fh_put(&argp->ffh); > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > > index a85dc4dd4f3a..91c9ea09f921 100644 > > --- a/fs/nfsd/vfs.c > > +++ b/fs/nfsd/vfs.c > > @@ -1451,9 +1451,9 @@ 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 nfsd_attrs *attrs, > > + struct svc_fh *resfhp) > > { > > struct dentry *dentry, *dnew; > > __be32 err, cerr; > > @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > > host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); > > err = nfserrno(host_err); > > + cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); > > + if (!err) > > + nfsd_create_setattr(rqstp, fhp, resfhp, attrs); > > fh_unlock(fhp); > > if (!err) > > err = nfserrno(commit_metadata(fhp)); > > - > > fh_drop_write(fhp); > > > > - cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); > > dput(dnew); > > if (err==0) err = cerr; > > out: > > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > > index 9bb0e3957982..f3f43ca3ac6b 100644 > > --- a/fs/nfsd/vfs.h > > +++ b/fs/nfsd/vfs.h > > @@ -114,8 +114,9 @@ __be32 nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, > > __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); > > + char *name, int len, char *path, > > + struct nfsd_attrs *attrs, > > + struct svc_fh *res); > > __be32 nfsd_link(struct svc_rqst *, struct svc_fh *, > > char *, int, struct svc_fh *); > > ssize_t nfsd_copy_file_range(struct file *, u64, > > > > > > -- > Chuck Lever > > > >
> On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote: > > The NFS protocol includes attributes when creating symlinks. > Linux does store attributes for symlinks and allows them to be set, > though they are not used for permission checking. > > NFSD currently doesn't set standard (struct iattr) attributes when > creating symlinks, but for NFSv4 it does set ACLs and security labels. > This is inconsistent. > > To improve consistency, pass the provided attributes into nfsd_symlink() > and call nfsd_create_setattr() to set them. This patch actually introduces a behavior change, if I'm reading it correctly. NFSD will now permit an NFSv4 client to specify attributes on creation, correct? I'm wondering if there will be fallout for our test cases. In any event, not an objection to this patch, but wanted the behavior modification to be noted at least in the review comments. > We ignore any error from nfsd_create_setattr(). It isn't really clear > what should be done if a file is successfully created, but the > attributes cannot be set. NFS doesn't allow partial success to be > reported. Reporting failure is probably more misleading than reporting > success, so the status is ignored. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs3proc.c | 3 ++- > fs/nfsd/nfs4proc.c | 2 +- > fs/nfsd/nfsproc.c | 3 ++- > fs/nfsd/vfs.c | 11 ++++++----- > fs/nfsd/vfs.h | 5 +++-- > 5 files changed, 14 insertions(+), 10 deletions(-) > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > index 289eb844d086..5e369096e42f 100644 > --- a/fs/nfsd/nfs3proc.c > +++ b/fs/nfsd/nfs3proc.c > @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp) > { > struct nfsd3_symlinkargs *argp = rqstp->rq_argp; > struct nfsd3_diropres *resp = rqstp->rq_resp; > + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; > > if (argp->tlen == 0) { > resp->status = nfserr_inval; > @@ -417,7 +418,7 @@ 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, &attrs, &resp->fh); > kfree(argp->tname); > out: > return rpc_success; > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index ba750d76f515..ee72c94732f0 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -809,7 +809,7 @@ 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, &attrs, &resfh); > break; > > case NF4BLK: > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > index 594d6f85c89f..d09d516188d2 100644 > --- a/fs/nfsd/nfsproc.c > +++ b/fs/nfsd/nfsproc.c > @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp) > { > struct nfsd_symlinkargs *argp = rqstp->rq_argp; > struct nfsd_stat *resp = rqstp->rq_resp; > + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; > struct svc_fh newfh; > > if (argp->tlen > NFS_MAXPATHLEN) { > @@ -495,7 +496,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, &attrs, &newfh); > > kfree(argp->tname); > fh_put(&argp->ffh); > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index a85dc4dd4f3a..91c9ea09f921 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1451,9 +1451,9 @@ 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 nfsd_attrs *attrs, > + struct svc_fh *resfhp) It would be nice if nfsd_symlink() had a kdoc comment like the one that nfsd_create_setattr() has. > { > struct dentry *dentry, *dnew; > __be32 err, cerr; > @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > > host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); > err = nfserrno(host_err); > + cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); > + if (!err) > + nfsd_create_setattr(rqstp, fhp, resfhp, attrs); > fh_unlock(fhp); > if (!err) > err = nfserrno(commit_metadata(fhp)); > - > fh_drop_write(fhp); > > - cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); > dput(dnew); > if (err==0) err = cerr; > out: > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > index 9bb0e3957982..f3f43ca3ac6b 100644 > --- a/fs/nfsd/vfs.h > +++ b/fs/nfsd/vfs.h > @@ -114,8 +114,9 @@ __be32 nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, > __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); > + char *name, int len, char *path, > + struct nfsd_attrs *attrs, > + struct svc_fh *res); > __be32 nfsd_link(struct svc_rqst *, struct svc_fh *, > char *, int, struct svc_fh *); > ssize_t nfsd_copy_file_range(struct file *, u64, > > -- Chuck Lever
On Fri, 29 Jul 2022, Chuck Lever III wrote: > > > On Jul 26, 2022, at 2:45 AM, NeilBrown <neilb@suse.de> wrote: > > > > The NFS protocol includes attributes when creating symlinks. > > Linux does store attributes for symlinks and allows them to be set, > > though they are not used for permission checking. > > > > NFSD currently doesn't set standard (struct iattr) attributes when > > creating symlinks, but for NFSv4 it does set ACLs and security labels. > > This is inconsistent. > > > > To improve consistency, pass the provided attributes into nfsd_symlink() > > and call nfsd_create_setattr() to set them. > > This patch actually introduces a behavior change, if I'm reading > it correctly. NFSD will now permit an NFSv4 client to specify > attributes on creation, correct? I'm wondering if there will be > fallout for our test cases. > > In any event, not an objection to this patch, but wanted the > behavior modification to be noted at least in the review comments. > Yes, this patch changes behaviour for v2, v3, and v4. Whatever attributes are received from the client when creating a symlink are passed on to the filesystem. With the Linux client, the only attributes are attr.ia_mode = S_IFLNK | S_IRWXUGO; attr.ia_valid = ATTR_MODE; so the final outcome will be unchanged. Other clients might sent different attributes, and if they did they probably expect them to be honoured. As you say, making this explicit in the commit comment is appropriate. Maybe: NOTE: this results in a behaviour change for all NFS versions when the client sends non-default attributes with a SYMLINK request. Thanks, NeilBrown > > > We ignore any error from nfsd_create_setattr(). It isn't really clear > > what should be done if a file is successfully created, but the > > attributes cannot be set. NFS doesn't allow partial success to be > > reported. Reporting failure is probably more misleading than reporting > > success, so the status is ignored. > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > fs/nfsd/nfs3proc.c | 3 ++- > > fs/nfsd/nfs4proc.c | 2 +- > > fs/nfsd/nfsproc.c | 3 ++- > > fs/nfsd/vfs.c | 11 ++++++----- > > fs/nfsd/vfs.h | 5 +++-- > > 5 files changed, 14 insertions(+), 10 deletions(-) > > > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > > index 289eb844d086..5e369096e42f 100644 > > --- a/fs/nfsd/nfs3proc.c > > +++ b/fs/nfsd/nfs3proc.c > > @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp) > > { > > struct nfsd3_symlinkargs *argp = rqstp->rq_argp; > > struct nfsd3_diropres *resp = rqstp->rq_resp; > > + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; > > > > if (argp->tlen == 0) { > > resp->status = nfserr_inval; > > @@ -417,7 +418,7 @@ 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, &attrs, &resp->fh); > > kfree(argp->tname); > > out: > > return rpc_success; > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > index ba750d76f515..ee72c94732f0 100644 > > --- a/fs/nfsd/nfs4proc.c > > +++ b/fs/nfsd/nfs4proc.c > > @@ -809,7 +809,7 @@ 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, &attrs, &resfh); > > break; > > > > case NF4BLK: > > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > > index 594d6f85c89f..d09d516188d2 100644 > > --- a/fs/nfsd/nfsproc.c > > +++ b/fs/nfsd/nfsproc.c > > @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp) > > { > > struct nfsd_symlinkargs *argp = rqstp->rq_argp; > > struct nfsd_stat *resp = rqstp->rq_resp; > > + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; > > struct svc_fh newfh; > > > > if (argp->tlen > NFS_MAXPATHLEN) { > > @@ -495,7 +496,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, &attrs, &newfh); > > > > kfree(argp->tname); > > fh_put(&argp->ffh); > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > > index a85dc4dd4f3a..91c9ea09f921 100644 > > --- a/fs/nfsd/vfs.c > > +++ b/fs/nfsd/vfs.c > > @@ -1451,9 +1451,9 @@ 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 nfsd_attrs *attrs, > > + struct svc_fh *resfhp) > > It would be nice if nfsd_symlink() had a kdoc comment like the one > that nfsd_create_setattr() has. > > > > { > > struct dentry *dentry, *dnew; > > __be32 err, cerr; > > @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > > host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); > > err = nfserrno(host_err); > > + cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); > > + if (!err) > > + nfsd_create_setattr(rqstp, fhp, resfhp, attrs); > > fh_unlock(fhp); > > if (!err) > > err = nfserrno(commit_metadata(fhp)); > > - > > fh_drop_write(fhp); > > > > - cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); > > dput(dnew); > > if (err==0) err = cerr; > > out: > > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > > index 9bb0e3957982..f3f43ca3ac6b 100644 > > --- a/fs/nfsd/vfs.h > > +++ b/fs/nfsd/vfs.h > > @@ -114,8 +114,9 @@ __be32 nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, > > __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); > > + char *name, int len, char *path, > > + struct nfsd_attrs *attrs, > > + struct svc_fh *res); > > __be32 nfsd_link(struct svc_rqst *, struct svc_fh *, > > char *, int, struct svc_fh *); > > ssize_t nfsd_copy_file_range(struct file *, u64, > > > > > > -- > Chuck Lever > > > >
diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c index 289eb844d086..5e369096e42f 100644 --- a/fs/nfsd/nfs3proc.c +++ b/fs/nfsd/nfs3proc.c @@ -391,6 +391,7 @@ nfsd3_proc_symlink(struct svc_rqst *rqstp) { struct nfsd3_symlinkargs *argp = rqstp->rq_argp; struct nfsd3_diropres *resp = rqstp->rq_resp; + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; if (argp->tlen == 0) { resp->status = nfserr_inval; @@ -417,7 +418,7 @@ 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, &attrs, &resp->fh); kfree(argp->tname); out: return rpc_success; diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index ba750d76f515..ee72c94732f0 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -809,7 +809,7 @@ 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, &attrs, &resfh); break; case NF4BLK: diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c index 594d6f85c89f..d09d516188d2 100644 --- a/fs/nfsd/nfsproc.c +++ b/fs/nfsd/nfsproc.c @@ -474,6 +474,7 @@ nfsd_proc_symlink(struct svc_rqst *rqstp) { struct nfsd_symlinkargs *argp = rqstp->rq_argp; struct nfsd_stat *resp = rqstp->rq_resp; + struct nfsd_attrs attrs = { .iattr = &argp->attrs }; struct svc_fh newfh; if (argp->tlen > NFS_MAXPATHLEN) { @@ -495,7 +496,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, &attrs, &newfh); kfree(argp->tname); fh_put(&argp->ffh); diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index a85dc4dd4f3a..91c9ea09f921 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1451,9 +1451,9 @@ 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 nfsd_attrs *attrs, + struct svc_fh *resfhp) { struct dentry *dentry, *dnew; __be32 err, cerr; @@ -1483,13 +1483,14 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path); err = nfserrno(host_err); + cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); + if (!err) + nfsd_create_setattr(rqstp, fhp, resfhp, attrs); fh_unlock(fhp); if (!err) err = nfserrno(commit_metadata(fhp)); - fh_drop_write(fhp); - cerr = fh_compose(resfhp, fhp->fh_export, dnew, fhp); dput(dnew); if (err==0) err = cerr; out: diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h index 9bb0e3957982..f3f43ca3ac6b 100644 --- a/fs/nfsd/vfs.h +++ b/fs/nfsd/vfs.h @@ -114,8 +114,9 @@ __be32 nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, __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); + char *name, int len, char *path, + struct nfsd_attrs *attrs, + struct svc_fh *res); __be32 nfsd_link(struct svc_rqst *, struct svc_fh *, char *, int, struct svc_fh *); ssize_t nfsd_copy_file_range(struct file *, u64,
The NFS protocol includes attributes when creating symlinks. Linux does store attributes for symlinks and allows them to be set, though they are not used for permission checking. NFSD currently doesn't set standard (struct iattr) attributes when creating symlinks, but for NFSv4 it does set ACLs and security labels. This is inconsistent. To improve consistency, pass the provided attributes into nfsd_symlink() and call nfsd_create_setattr() to set them. We ignore any error from nfsd_create_setattr(). It isn't really clear what should be done if a file is successfully created, but the attributes cannot be set. NFS doesn't allow partial success to be reported. Reporting failure is probably more misleading than reporting success, so the status is ignored. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfs3proc.c | 3 ++- fs/nfsd/nfs4proc.c | 2 +- fs/nfsd/nfsproc.c | 3 ++- fs/nfsd/vfs.c | 11 ++++++----- fs/nfsd/vfs.h | 5 +++-- 5 files changed, 14 insertions(+), 10 deletions(-)