| Message ID | 20230720-bz2223560-v2-1-070aaf2660b7@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: sanely handle inabilty to fetch pre/post attributes | expand |
On Fri, 21 Jul 2023, Jeff Layton wrote: > Collecting pre_op_attrs can fail, in which case it's probably best to > fail the whole operation. > > Change fh_fill_{pre,post,both}_attrs to return __be32. For the pre and > both functions, have the callers check the return code and abort the > operation if it failed. > > If fh_fill_post_attrs fails, then it's too late to do anything about it, > so most of those callers ignore the return value. If this happens, then > fh_post_saved will be false, which should cue the later stages to deal > with it. > > Suggested-by: Chuck Lever <chuck.lever@oracle.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs3proc.c | 4 +++- > fs/nfsd/nfs4proc.c | 14 ++++++------ > fs/nfsd/nfsfh.c | 26 ++++++++++++++--------- > fs/nfsd/nfsfh.h | 6 +++--- > fs/nfsd/vfs.c | 62 ++++++++++++++++++++++++++++++++++-------------------- > 5 files changed, 69 insertions(+), 43 deletions(-) > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > index fc8d5b7db9f8..268ef57751c4 100644 > --- a/fs/nfsd/nfs3proc.c > +++ b/fs/nfsd/nfs3proc.c > @@ -307,7 +307,9 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (!IS_POSIXACL(inode)) > iap->ia_mode &= ~current_umask(); > > - fh_fill_pre_attrs(fhp); > + status = fh_fill_pre_attrs(fhp); > + if (status != nfs_ok) > + goto out; > host_err = vfs_create(&nop_mnt_idmap, inode, child, iap->ia_mode, true); > if (host_err < 0) { > status = nfserrno(host_err); > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > index d8e7a533f9d2..9285e1eab4d5 100644 > --- a/fs/nfsd/nfs4proc.c > +++ b/fs/nfsd/nfs4proc.c > @@ -297,12 +297,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > } > > if (d_really_is_positive(child)) { > - status = nfs_ok; > - > /* NFSv4 protocol requires change attributes even though > * no change happened. > */ > - fh_fill_both_attrs(fhp); > + status = fh_fill_both_attrs(fhp); > + if (status != nfs_ok) > + goto out; > > switch (open->op_createmode) { > case NFS4_CREATE_UNCHECKED: > @@ -345,7 +345,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > if (!IS_POSIXACL(inode)) > iap->ia_mode &= ~current_umask(); > > - fh_fill_pre_attrs(fhp); > + status = fh_fill_pre_attrs(fhp); > + if (status != nfs_ok) > + goto out; > status = nfsd4_vfs_create(fhp, child, open); > if (status != nfs_ok) > goto out; > @@ -424,11 +426,11 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru > } else { > status = nfsd_lookup(rqstp, current_fh, > open->op_fname, open->op_fnamelen, *resfh); > - if (!status) > + if (status == nfs_ok) > /* NFSv4 protocol requires change attributes even though > * no change happened. > */ > - fh_fill_both_attrs(current_fh); > + status = fh_fill_both_attrs(current_fh); > } > if (status) > goto out; > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > index c291389a1d71..f7e68a91e826 100644 > --- a/fs/nfsd/nfsfh.c > +++ b/fs/nfsd/nfsfh.c > @@ -614,7 +614,7 @@ fh_update(struct svc_fh *fhp) > * @fhp: file handle to be updated > * > */ > -void fh_fill_pre_attrs(struct svc_fh *fhp) > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp) > { > bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE); > struct inode *inode; > @@ -622,12 +622,12 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) > __be32 err; > > if (fhp->fh_no_wcc || fhp->fh_pre_saved) > - return; > + return nfs_ok; > > inode = d_inode(fhp->fh_dentry); > err = fh_getattr(fhp, &stat); > if (err) > - return; > + return err; > > if (v4) > fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode); > @@ -636,6 +636,7 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) > fhp->fh_pre_ctime = stat.ctime; > fhp->fh_pre_size = stat.size; > fhp->fh_pre_saved = true; > + return nfs_ok; > } > > /** > @@ -643,26 +644,27 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) > * @fhp: file handle to be updated > * > */ > -void fh_fill_post_attrs(struct svc_fh *fhp) > +__be32 fh_fill_post_attrs(struct svc_fh *fhp) > { > bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE); > struct inode *inode = d_inode(fhp->fh_dentry); > __be32 err; > > if (fhp->fh_no_wcc) > - return; > + return nfs_ok; > > if (fhp->fh_post_saved) > printk("nfsd: inode locked twice during operation.\n"); > > err = fh_getattr(fhp, &fhp->fh_post_attr); > if (err) > - return; > + return err; > > fhp->fh_post_saved = true; > if (v4) > fhp->fh_post_change = > nfsd4_change_attribute(&fhp->fh_post_attr, inode); > + return nfs_ok; > } > > /** > @@ -672,16 +674,20 @@ void fh_fill_post_attrs(struct svc_fh *fhp) > * This is used when the directory wasn't changed, but wcc attributes > * are needed anyway. > */ > -void fh_fill_both_attrs(struct svc_fh *fhp) > +__be32 fh_fill_both_attrs(struct svc_fh *fhp) > { > - fh_fill_post_attrs(fhp); > - if (!fhp->fh_post_saved) > - return; > + __be32 err; > + > + err = fh_fill_post_attrs(fhp); > + if (err) > + return err; > + > fhp->fh_pre_change = fhp->fh_post_change; > fhp->fh_pre_mtime = fhp->fh_post_attr.mtime; > fhp->fh_pre_ctime = fhp->fh_post_attr.ctime; > fhp->fh_pre_size = fhp->fh_post_attr.size; > fhp->fh_pre_saved = true; > + return nfs_ok; > } > > /* > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h > index 4e0ecf0ae2cf..486803694acc 100644 > --- a/fs/nfsd/nfsfh.h > +++ b/fs/nfsd/nfsfh.h > @@ -294,7 +294,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp) > } > > u64 nfsd4_change_attribute(struct kstat *stat, struct inode *inode); > -extern void fh_fill_pre_attrs(struct svc_fh *fhp); > -extern void fh_fill_post_attrs(struct svc_fh *fhp); > -extern void fh_fill_both_attrs(struct svc_fh *fhp); > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp); > +__be32 fh_fill_post_attrs(struct svc_fh *fhp); > +__be32 fh_fill_both_attrs(struct svc_fh *fhp); > #endif /* _LINUX_NFSD_NFSFH_H */ > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 8a2321d19194..f200afd33630 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -1537,9 +1537,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > dput(dchild); > if (err) > goto out_unlock; > - fh_fill_pre_attrs(fhp); > - err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp); > - fh_fill_post_attrs(fhp); > + err = fh_fill_pre_attrs(fhp); > + if (err == nfs_ok) { > + err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp); > + fh_fill_post_attrs(fhp); Most error handling in nfsd is if (err) goto .... Here (and one other place I think) you have if (not err) do stuff; Do we want to be more consistent? I'm in two minds about this and I don't dislike your patch. But I noticed the inconsistency and thought I should mention it. Also, should we put a __must_check annotation on fh_fill_pre_attrs() and ..post..? Then I wouldn't have to manually check that you found and fixed all callers (which I haven't). Thanks, NeilBrown
On Fri, Jul 21, 2023 at 07:46:20AM +1000, NeilBrown wrote: > On Fri, 21 Jul 2023, Jeff Layton wrote: > > Collecting pre_op_attrs can fail, in which case it's probably best to > > fail the whole operation. > > > > Change fh_fill_{pre,post,both}_attrs to return __be32. For the pre and > > both functions, have the callers check the return code and abort the > > operation if it failed. > > > > If fh_fill_post_attrs fails, then it's too late to do anything about it, > > so most of those callers ignore the return value. If this happens, then > > fh_post_saved will be false, which should cue the later stages to deal > > with it. > > > > Suggested-by: Chuck Lever <chuck.lever@oracle.com> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/nfs3proc.c | 4 +++- > > fs/nfsd/nfs4proc.c | 14 ++++++------ > > fs/nfsd/nfsfh.c | 26 ++++++++++++++--------- > > fs/nfsd/nfsfh.h | 6 +++--- > > fs/nfsd/vfs.c | 62 ++++++++++++++++++++++++++++++++++-------------------- > > 5 files changed, 69 insertions(+), 43 deletions(-) > > > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > > index fc8d5b7db9f8..268ef57751c4 100644 > > --- a/fs/nfsd/nfs3proc.c > > +++ b/fs/nfsd/nfs3proc.c > > @@ -307,7 +307,9 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > if (!IS_POSIXACL(inode)) > > iap->ia_mode &= ~current_umask(); > > > > - fh_fill_pre_attrs(fhp); > > + status = fh_fill_pre_attrs(fhp); > > + if (status != nfs_ok) > > + goto out; > > host_err = vfs_create(&nop_mnt_idmap, inode, child, iap->ia_mode, true); > > if (host_err < 0) { > > status = nfserrno(host_err); > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > index d8e7a533f9d2..9285e1eab4d5 100644 > > --- a/fs/nfsd/nfs4proc.c > > +++ b/fs/nfsd/nfs4proc.c > > @@ -297,12 +297,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > } > > > > if (d_really_is_positive(child)) { > > - status = nfs_ok; > > - > > /* NFSv4 protocol requires change attributes even though > > * no change happened. > > */ > > - fh_fill_both_attrs(fhp); > > + status = fh_fill_both_attrs(fhp); > > + if (status != nfs_ok) > > + goto out; > > > > switch (open->op_createmode) { > > case NFS4_CREATE_UNCHECKED: > > @@ -345,7 +345,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > if (!IS_POSIXACL(inode)) > > iap->ia_mode &= ~current_umask(); > > > > - fh_fill_pre_attrs(fhp); > > + status = fh_fill_pre_attrs(fhp); > > + if (status != nfs_ok) > > + goto out; > > status = nfsd4_vfs_create(fhp, child, open); > > if (status != nfs_ok) > > goto out; > > @@ -424,11 +426,11 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru > > } else { > > status = nfsd_lookup(rqstp, current_fh, > > open->op_fname, open->op_fnamelen, *resfh); > > - if (!status) > > + if (status == nfs_ok) > > /* NFSv4 protocol requires change attributes even though > > * no change happened. > > */ > > - fh_fill_both_attrs(current_fh); > > + status = fh_fill_both_attrs(current_fh); > > } > > if (status) > > goto out; > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > > index c291389a1d71..f7e68a91e826 100644 > > --- a/fs/nfsd/nfsfh.c > > +++ b/fs/nfsd/nfsfh.c > > @@ -614,7 +614,7 @@ fh_update(struct svc_fh *fhp) > > * @fhp: file handle to be updated > > * > > */ > > -void fh_fill_pre_attrs(struct svc_fh *fhp) > > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp) > > { > > bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE); > > struct inode *inode; > > @@ -622,12 +622,12 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) > > __be32 err; > > > > if (fhp->fh_no_wcc || fhp->fh_pre_saved) > > - return; > > + return nfs_ok; > > > > inode = d_inode(fhp->fh_dentry); > > err = fh_getattr(fhp, &stat); > > if (err) > > - return; > > + return err; > > > > if (v4) > > fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode); > > @@ -636,6 +636,7 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) > > fhp->fh_pre_ctime = stat.ctime; > > fhp->fh_pre_size = stat.size; > > fhp->fh_pre_saved = true; > > + return nfs_ok; > > } > > > > /** > > @@ -643,26 +644,27 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) > > * @fhp: file handle to be updated > > * > > */ > > -void fh_fill_post_attrs(struct svc_fh *fhp) > > +__be32 fh_fill_post_attrs(struct svc_fh *fhp) > > { > > bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE); > > struct inode *inode = d_inode(fhp->fh_dentry); > > __be32 err; > > > > if (fhp->fh_no_wcc) > > - return; > > + return nfs_ok; > > > > if (fhp->fh_post_saved) > > printk("nfsd: inode locked twice during operation.\n"); > > > > err = fh_getattr(fhp, &fhp->fh_post_attr); > > if (err) > > - return; > > + return err; > > > > fhp->fh_post_saved = true; > > if (v4) > > fhp->fh_post_change = > > nfsd4_change_attribute(&fhp->fh_post_attr, inode); > > + return nfs_ok; > > } > > > > /** > > @@ -672,16 +674,20 @@ void fh_fill_post_attrs(struct svc_fh *fhp) > > * This is used when the directory wasn't changed, but wcc attributes > > * are needed anyway. > > */ > > -void fh_fill_both_attrs(struct svc_fh *fhp) > > +__be32 fh_fill_both_attrs(struct svc_fh *fhp) > > { > > - fh_fill_post_attrs(fhp); > > - if (!fhp->fh_post_saved) > > - return; > > + __be32 err; > > + > > + err = fh_fill_post_attrs(fhp); > > + if (err) > > + return err; > > + > > fhp->fh_pre_change = fhp->fh_post_change; > > fhp->fh_pre_mtime = fhp->fh_post_attr.mtime; > > fhp->fh_pre_ctime = fhp->fh_post_attr.ctime; > > fhp->fh_pre_size = fhp->fh_post_attr.size; > > fhp->fh_pre_saved = true; > > + return nfs_ok; > > } > > > > /* > > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h > > index 4e0ecf0ae2cf..486803694acc 100644 > > --- a/fs/nfsd/nfsfh.h > > +++ b/fs/nfsd/nfsfh.h > > @@ -294,7 +294,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp) > > } > > > > u64 nfsd4_change_attribute(struct kstat *stat, struct inode *inode); > > -extern void fh_fill_pre_attrs(struct svc_fh *fhp); > > -extern void fh_fill_post_attrs(struct svc_fh *fhp); > > -extern void fh_fill_both_attrs(struct svc_fh *fhp); > > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp); > > +__be32 fh_fill_post_attrs(struct svc_fh *fhp); > > +__be32 fh_fill_both_attrs(struct svc_fh *fhp); > > #endif /* _LINUX_NFSD_NFSFH_H */ > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > > index 8a2321d19194..f200afd33630 100644 > > --- a/fs/nfsd/vfs.c > > +++ b/fs/nfsd/vfs.c > > @@ -1537,9 +1537,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > > dput(dchild); > > if (err) > > goto out_unlock; > > - fh_fill_pre_attrs(fhp); > > - err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp); > > - fh_fill_post_attrs(fhp); > > + err = fh_fill_pre_attrs(fhp); > > + if (err == nfs_ok) { > > + err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp); > > + fh_fill_post_attrs(fhp); > > Most error handling in nfsd is > > if (err) > goto .... > > Here (and one other place I think) you have > if (not err) > do stuff; > > Do we want to be more consistent? Yes, unless being consistent makes this code unreadable. There doesn't seem to be a reason to drop that convention here. > I'm in two minds about this and I > don't dislike your patch. But I noticed the inconsistency and thought I > should mention it. > > Also, should we put a __must_check annotation on fh_fill_pre_attrs() and > ..post..? Then I wouldn't have to manually check that you found and > fixed all callers (which I haven't).
On Thu, 2023-07-20 at 19:09 -0400, Chuck Lever wrote: > On Fri, Jul 21, 2023 at 07:46:20AM +1000, NeilBrown wrote: > > On Fri, 21 Jul 2023, Jeff Layton wrote: > > > Collecting pre_op_attrs can fail, in which case it's probably best to > > > fail the whole operation. > > > > > > Change fh_fill_{pre,post,both}_attrs to return __be32. For the pre and > > > both functions, have the callers check the return code and abort the > > > operation if it failed. > > > > > > If fh_fill_post_attrs fails, then it's too late to do anything about it, > > > so most of those callers ignore the return value. If this happens, then > > > fh_post_saved will be false, which should cue the later stages to deal > > > with it. > > > > > > Suggested-by: Chuck Lever <chuck.lever@oracle.com> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > fs/nfsd/nfs3proc.c | 4 +++- > > > fs/nfsd/nfs4proc.c | 14 ++++++------ > > > fs/nfsd/nfsfh.c | 26 ++++++++++++++--------- > > > fs/nfsd/nfsfh.h | 6 +++--- > > > fs/nfsd/vfs.c | 62 ++++++++++++++++++++++++++++++++++-------------------- > > > 5 files changed, 69 insertions(+), 43 deletions(-) > > > > > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > > > index fc8d5b7db9f8..268ef57751c4 100644 > > > --- a/fs/nfsd/nfs3proc.c > > > +++ b/fs/nfsd/nfs3proc.c > > > @@ -307,7 +307,9 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > if (!IS_POSIXACL(inode)) > > > iap->ia_mode &= ~current_umask(); > > > > > > - fh_fill_pre_attrs(fhp); > > > + status = fh_fill_pre_attrs(fhp); > > > + if (status != nfs_ok) > > > + goto out; > > > host_err = vfs_create(&nop_mnt_idmap, inode, child, iap->ia_mode, true); > > > if (host_err < 0) { > > > status = nfserrno(host_err); > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > > index d8e7a533f9d2..9285e1eab4d5 100644 > > > --- a/fs/nfsd/nfs4proc.c > > > +++ b/fs/nfsd/nfs4proc.c > > > @@ -297,12 +297,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > } > > > > > > if (d_really_is_positive(child)) { > > > - status = nfs_ok; > > > - > > > /* NFSv4 protocol requires change attributes even though > > > * no change happened. > > > */ > > > - fh_fill_both_attrs(fhp); > > > + status = fh_fill_both_attrs(fhp); > > > + if (status != nfs_ok) > > > + goto out; > > > > > > switch (open->op_createmode) { > > > case NFS4_CREATE_UNCHECKED: > > > @@ -345,7 +345,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > if (!IS_POSIXACL(inode)) > > > iap->ia_mode &= ~current_umask(); > > > > > > - fh_fill_pre_attrs(fhp); > > > + status = fh_fill_pre_attrs(fhp); > > > + if (status != nfs_ok) > > > + goto out; > > > status = nfsd4_vfs_create(fhp, child, open); > > > if (status != nfs_ok) > > > goto out; > > > @@ -424,11 +426,11 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru > > > } else { > > > status = nfsd_lookup(rqstp, current_fh, > > > open->op_fname, open->op_fnamelen, *resfh); > > > - if (!status) > > > + if (status == nfs_ok) > > > /* NFSv4 protocol requires change attributes even though > > > * no change happened. > > > */ > > > - fh_fill_both_attrs(current_fh); > > > + status = fh_fill_both_attrs(current_fh); > > > } > > > if (status) > > > goto out; > > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > > > index c291389a1d71..f7e68a91e826 100644 > > > --- a/fs/nfsd/nfsfh.c > > > +++ b/fs/nfsd/nfsfh.c > > > @@ -614,7 +614,7 @@ fh_update(struct svc_fh *fhp) > > > * @fhp: file handle to be updated > > > * > > > */ > > > -void fh_fill_pre_attrs(struct svc_fh *fhp) > > > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp) > > > { > > > bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE); > > > struct inode *inode; > > > @@ -622,12 +622,12 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) > > > __be32 err; > > > > > > if (fhp->fh_no_wcc || fhp->fh_pre_saved) > > > - return; > > > + return nfs_ok; > > > > > > inode = d_inode(fhp->fh_dentry); > > > err = fh_getattr(fhp, &stat); > > > if (err) > > > - return; > > > + return err; > > > > > > if (v4) > > > fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode); > > > @@ -636,6 +636,7 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) > > > fhp->fh_pre_ctime = stat.ctime; > > > fhp->fh_pre_size = stat.size; > > > fhp->fh_pre_saved = true; > > > + return nfs_ok; > > > } > > > > > > /** > > > @@ -643,26 +644,27 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) > > > * @fhp: file handle to be updated > > > * > > > */ > > > -void fh_fill_post_attrs(struct svc_fh *fhp) > > > +__be32 fh_fill_post_attrs(struct svc_fh *fhp) > > > { > > > bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE); > > > struct inode *inode = d_inode(fhp->fh_dentry); > > > __be32 err; > > > > > > if (fhp->fh_no_wcc) > > > - return; > > > + return nfs_ok; > > > > > > if (fhp->fh_post_saved) > > > printk("nfsd: inode locked twice during operation.\n"); > > > > > > err = fh_getattr(fhp, &fhp->fh_post_attr); > > > if (err) > > > - return; > > > + return err; > > > > > > fhp->fh_post_saved = true; > > > if (v4) > > > fhp->fh_post_change = > > > nfsd4_change_attribute(&fhp->fh_post_attr, inode); > > > + return nfs_ok; > > > } > > > > > > /** > > > @@ -672,16 +674,20 @@ void fh_fill_post_attrs(struct svc_fh *fhp) > > > * This is used when the directory wasn't changed, but wcc attributes > > > * are needed anyway. > > > */ > > > -void fh_fill_both_attrs(struct svc_fh *fhp) > > > +__be32 fh_fill_both_attrs(struct svc_fh *fhp) > > > { > > > - fh_fill_post_attrs(fhp); > > > - if (!fhp->fh_post_saved) > > > - return; > > > + __be32 err; > > > + > > > + err = fh_fill_post_attrs(fhp); > > > + if (err) > > > + return err; > > > + > > > fhp->fh_pre_change = fhp->fh_post_change; > > > fhp->fh_pre_mtime = fhp->fh_post_attr.mtime; > > > fhp->fh_pre_ctime = fhp->fh_post_attr.ctime; > > > fhp->fh_pre_size = fhp->fh_post_attr.size; > > > fhp->fh_pre_saved = true; > > > + return nfs_ok; > > > } > > > > > > /* > > > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h > > > index 4e0ecf0ae2cf..486803694acc 100644 > > > --- a/fs/nfsd/nfsfh.h > > > +++ b/fs/nfsd/nfsfh.h > > > @@ -294,7 +294,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp) > > > } > > > > > > u64 nfsd4_change_attribute(struct kstat *stat, struct inode *inode); > > > -extern void fh_fill_pre_attrs(struct svc_fh *fhp); > > > -extern void fh_fill_post_attrs(struct svc_fh *fhp); > > > -extern void fh_fill_both_attrs(struct svc_fh *fhp); > > > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp); > > > +__be32 fh_fill_post_attrs(struct svc_fh *fhp); > > > +__be32 fh_fill_both_attrs(struct svc_fh *fhp); > > > #endif /* _LINUX_NFSD_NFSFH_H */ > > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > > > index 8a2321d19194..f200afd33630 100644 > > > --- a/fs/nfsd/vfs.c > > > +++ b/fs/nfsd/vfs.c > > > @@ -1537,9 +1537,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, > > > dput(dchild); > > > if (err) > > > goto out_unlock; > > > - fh_fill_pre_attrs(fhp); > > > - err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp); > > > - fh_fill_post_attrs(fhp); > > > + err = fh_fill_pre_attrs(fhp); > > > + if (err == nfs_ok) { > > > + err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp); > > > + fh_fill_post_attrs(fhp); > > > > Most error handling in nfsd is > > > > if (err) > > goto .... > > > > Here (and one other place I think) you have > > if (not err) > > do stuff; > > > > Do we want to be more consistent? > > Yes, unless being consistent makes this code unreadable. There > doesn't seem to be a reason to drop that convention here. > My usual test for this is to use gotos if unwinding errors is complex enough to warrant it, and to just use the second form if the code is fairly simple. But...if you want gotos everywhere, then so be it. I'll respin this. > > > I'm in two minds about this and I > > don't dislike your patch. But I noticed the inconsistency and thought I > > should mention it. > > > > Also, should we put a __must_check annotation on fh_fill_pre_attrs() and > > ..post..? Then I wouldn't have to manually check that you found and > > fixed all callers (which I haven't). Maybe for the "pre" and "both" ones. We would _not_ want to add __must_check for the post one, since most of the callers (correctly) ignore that return value. I'll plan to roll that in.
diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c index fc8d5b7db9f8..268ef57751c4 100644 --- a/fs/nfsd/nfs3proc.c +++ b/fs/nfsd/nfs3proc.c @@ -307,7 +307,9 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, if (!IS_POSIXACL(inode)) iap->ia_mode &= ~current_umask(); - fh_fill_pre_attrs(fhp); + status = fh_fill_pre_attrs(fhp); + if (status != nfs_ok) + goto out; host_err = vfs_create(&nop_mnt_idmap, inode, child, iap->ia_mode, true); if (host_err < 0) { status = nfserrno(host_err); diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c index d8e7a533f9d2..9285e1eab4d5 100644 --- a/fs/nfsd/nfs4proc.c +++ b/fs/nfsd/nfs4proc.c @@ -297,12 +297,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, } if (d_really_is_positive(child)) { - status = nfs_ok; - /* NFSv4 protocol requires change attributes even though * no change happened. */ - fh_fill_both_attrs(fhp); + status = fh_fill_both_attrs(fhp); + if (status != nfs_ok) + goto out; switch (open->op_createmode) { case NFS4_CREATE_UNCHECKED: @@ -345,7 +345,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp, if (!IS_POSIXACL(inode)) iap->ia_mode &= ~current_umask(); - fh_fill_pre_attrs(fhp); + status = fh_fill_pre_attrs(fhp); + if (status != nfs_ok) + goto out; status = nfsd4_vfs_create(fhp, child, open); if (status != nfs_ok) goto out; @@ -424,11 +426,11 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru } else { status = nfsd_lookup(rqstp, current_fh, open->op_fname, open->op_fnamelen, *resfh); - if (!status) + if (status == nfs_ok) /* NFSv4 protocol requires change attributes even though * no change happened. */ - fh_fill_both_attrs(current_fh); + status = fh_fill_both_attrs(current_fh); } if (status) goto out; diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c index c291389a1d71..f7e68a91e826 100644 --- a/fs/nfsd/nfsfh.c +++ b/fs/nfsd/nfsfh.c @@ -614,7 +614,7 @@ fh_update(struct svc_fh *fhp) * @fhp: file handle to be updated * */ -void fh_fill_pre_attrs(struct svc_fh *fhp) +__be32 fh_fill_pre_attrs(struct svc_fh *fhp) { bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE); struct inode *inode; @@ -622,12 +622,12 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) __be32 err; if (fhp->fh_no_wcc || fhp->fh_pre_saved) - return; + return nfs_ok; inode = d_inode(fhp->fh_dentry); err = fh_getattr(fhp, &stat); if (err) - return; + return err; if (v4) fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode); @@ -636,6 +636,7 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) fhp->fh_pre_ctime = stat.ctime; fhp->fh_pre_size = stat.size; fhp->fh_pre_saved = true; + return nfs_ok; } /** @@ -643,26 +644,27 @@ void fh_fill_pre_attrs(struct svc_fh *fhp) * @fhp: file handle to be updated * */ -void fh_fill_post_attrs(struct svc_fh *fhp) +__be32 fh_fill_post_attrs(struct svc_fh *fhp) { bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE); struct inode *inode = d_inode(fhp->fh_dentry); __be32 err; if (fhp->fh_no_wcc) - return; + return nfs_ok; if (fhp->fh_post_saved) printk("nfsd: inode locked twice during operation.\n"); err = fh_getattr(fhp, &fhp->fh_post_attr); if (err) - return; + return err; fhp->fh_post_saved = true; if (v4) fhp->fh_post_change = nfsd4_change_attribute(&fhp->fh_post_attr, inode); + return nfs_ok; } /** @@ -672,16 +674,20 @@ void fh_fill_post_attrs(struct svc_fh *fhp) * This is used when the directory wasn't changed, but wcc attributes * are needed anyway. */ -void fh_fill_both_attrs(struct svc_fh *fhp) +__be32 fh_fill_both_attrs(struct svc_fh *fhp) { - fh_fill_post_attrs(fhp); - if (!fhp->fh_post_saved) - return; + __be32 err; + + err = fh_fill_post_attrs(fhp); + if (err) + return err; + fhp->fh_pre_change = fhp->fh_post_change; fhp->fh_pre_mtime = fhp->fh_post_attr.mtime; fhp->fh_pre_ctime = fhp->fh_post_attr.ctime; fhp->fh_pre_size = fhp->fh_post_attr.size; fhp->fh_pre_saved = true; + return nfs_ok; } /* diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h index 4e0ecf0ae2cf..486803694acc 100644 --- a/fs/nfsd/nfsfh.h +++ b/fs/nfsd/nfsfh.h @@ -294,7 +294,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp) } u64 nfsd4_change_attribute(struct kstat *stat, struct inode *inode); -extern void fh_fill_pre_attrs(struct svc_fh *fhp); -extern void fh_fill_post_attrs(struct svc_fh *fhp); -extern void fh_fill_both_attrs(struct svc_fh *fhp); +__be32 fh_fill_pre_attrs(struct svc_fh *fhp); +__be32 fh_fill_post_attrs(struct svc_fh *fhp); +__be32 fh_fill_both_attrs(struct svc_fh *fhp); #endif /* _LINUX_NFSD_NFSFH_H */ diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 8a2321d19194..f200afd33630 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -1537,9 +1537,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp, dput(dchild); if (err) goto out_unlock; - fh_fill_pre_attrs(fhp); - err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp); - fh_fill_post_attrs(fhp); + err = fh_fill_pre_attrs(fhp); + if (err == nfs_ok) { + err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp); + fh_fill_post_attrs(fhp); + } out_unlock: inode_unlock(dentry->d_inode); return err; @@ -1632,13 +1634,16 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp, inode_unlock(dentry->d_inode); goto out_drop_write; } - fh_fill_pre_attrs(fhp); + err = fh_fill_pre_attrs(fhp); + if (err) + goto out_unlock; host_err = vfs_symlink(&nop_mnt_idmap, 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_fill_post_attrs(fhp); +out_unlock: inode_unlock(dentry->d_inode); if (!err) err = nfserrno(commit_metadata(fhp)); @@ -1700,7 +1705,9 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp, err = nfserr_noent; if (d_really_is_negative(dold)) goto out_dput; - fh_fill_pre_attrs(ffhp); + err = fh_fill_pre_attrs(ffhp); + if (err != nfs_ok) + goto out_dput; host_err = vfs_link(dold, &nop_mnt_idmap, dirp, dnew, NULL); fh_fill_post_attrs(ffhp); inode_unlock(dirp); @@ -1786,8 +1793,12 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, } trap = lock_rename(tdentry, fdentry); - fh_fill_pre_attrs(ffhp); - fh_fill_pre_attrs(tfhp); + err = fh_fill_pre_attrs(ffhp); + if (err != nfs_ok) + goto out_unlock; + err = fh_fill_pre_attrs(tfhp); + if (err != nfs_ok) + goto out_unlock; odentry = lookup_one_len(fname, fdentry, flen); host_err = PTR_ERR(odentry); @@ -1854,6 +1865,7 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen, fh_fill_post_attrs(ffhp); fh_fill_post_attrs(tfhp); } +out_unlock: unlock_rename(tdentry, fdentry); fh_drop_write(ffhp); @@ -1913,12 +1925,14 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type, goto out_unlock; } rinode = d_inode(rdentry); - ihold(rinode); + err = fh_fill_pre_attrs(fhp); + if (err != nfs_ok) + goto out_unlock; + ihold(rinode); if (!type) type = d_inode(rdentry)->i_mode & S_IFMT; - fh_fill_pre_attrs(fhp); if (type != S_IFDIR) { int retries; @@ -2338,16 +2352,17 @@ nfsd_removexattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name) return nfserrno(ret); inode_lock(fhp->fh_dentry->d_inode); - fh_fill_pre_attrs(fhp); - - ret = __vfs_removexattr_locked(&nop_mnt_idmap, fhp->fh_dentry, - name, NULL); - - fh_fill_post_attrs(fhp); + err = fh_fill_pre_attrs(fhp); + if (err == nfs_ok) { + ret = __vfs_removexattr_locked(&nop_mnt_idmap, fhp->fh_dentry, + name, NULL); + err = nfsd_xattr_errno(ret); + fh_fill_post_attrs(fhp); + } inode_unlock(fhp->fh_dentry->d_inode); fh_drop_write(fhp); - return nfsd_xattr_errno(ret); + return err; } __be32 @@ -2365,15 +2380,16 @@ nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name, if (ret) return nfserrno(ret); inode_lock(fhp->fh_dentry->d_inode); - fh_fill_pre_attrs(fhp); - - ret = __vfs_setxattr_locked(&nop_mnt_idmap, fhp->fh_dentry, name, buf, - len, flags, NULL); - fh_fill_post_attrs(fhp); + err = fh_fill_pre_attrs(fhp); + if (err != nfs_ok) { + ret = __vfs_setxattr_locked(&nop_mnt_idmap, fhp->fh_dentry, + name, buf, len, flags, NULL); + fh_fill_post_attrs(fhp); + err = nfsd_xattr_errno(ret); + } inode_unlock(fhp->fh_dentry->d_inode); fh_drop_write(fhp); - - return nfsd_xattr_errno(ret); + return err; } #endif
Collecting pre_op_attrs can fail, in which case it's probably best to fail the whole operation. Change fh_fill_{pre,post,both}_attrs to return __be32. For the pre and both functions, have the callers check the return code and abort the operation if it failed. If fh_fill_post_attrs fails, then it's too late to do anything about it, so most of those callers ignore the return value. If this happens, then fh_post_saved will be false, which should cue the later stages to deal with it. Suggested-by: Chuck Lever <chuck.lever@oracle.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs3proc.c | 4 +++- fs/nfsd/nfs4proc.c | 14 ++++++------ fs/nfsd/nfsfh.c | 26 ++++++++++++++--------- fs/nfsd/nfsfh.h | 6 +++--- fs/nfsd/vfs.c | 62 ++++++++++++++++++++++++++++++++++-------------------- 5 files changed, 69 insertions(+), 43 deletions(-)