| Message ID | 877fbxperp.fsf@notabene.neil.brown.name (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Thu, Aug 04, 2016 at 10:19:06AM +1000, NeilBrown wrote: > > > When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors. > In particular it can be tempting to return ENOENT, but this is not > handled well by nfsd. > > Rather than requiring strict adherence to error code code filesystems, > treat all unexpected error codes the same as ESTALE. This is safest. > > Signed-off-by: NeilBrown <neilb@suse.com> > --- > > I didn't add a dprintk for unexpected error messages, partly > because dprintk isn't usable in exportfs. I could have used pr_debug() > but I really didn't see much value. > > This has been tested together with the btrfs change, and it restores > correct functionality. I don't really like all this magic which is partially historic. I think we should instead allow the fs to return any error from the export operations, and forbid returning NULL entirely. Then the actualy caller (nfsd) can sort out which errors it wants to send over the wire. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 04, 2016 at 05:47:19AM -0700, Christoph Hellwig wrote: > On Thu, Aug 04, 2016 at 10:19:06AM +1000, NeilBrown wrote: > > > > > > When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors. > > In particular it can be tempting to return ENOENT, but this is not > > handled well by nfsd. > > > > Rather than requiring strict adherence to error code code filesystems, > > treat all unexpected error codes the same as ESTALE. This is safest. > > > > Signed-off-by: NeilBrown <neilb@suse.com> > > --- > > > > I didn't add a dprintk for unexpected error messages, partly > > because dprintk isn't usable in exportfs. I could have used pr_debug() > > but I really didn't see much value. > > > > This has been tested together with the btrfs change, and it restores > > correct functionality. > > I don't really like all this magic which is partially historic. I think > we should instead allow the fs to return any error from the export > operations, What errors other than ENOENT and ENOMEM do you think are reasonable? ENOENT is going to screw up both nfsd and open_by_fhandle_at, which are the only callers. > and forbid returning NULL entirely. Then the actualy caller > (nfsd) can sort out which errors it wants to send over the wire. The needs of those two callers don't look very different to me, and I can't recall seeing a correct use of an error other than ESTALE or ENOMEM, so I've been thinking of it more of a question of how to best handle a misbehaving filesystem. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 04 2016, Christoph Hellwig wrote: > On Thu, Aug 04, 2016 at 10:19:06AM +1000, NeilBrown wrote: >> >> >> When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors. >> In particular it can be tempting to return ENOENT, but this is not >> handled well by nfsd. >> >> Rather than requiring strict adherence to error code code filesystems, >> treat all unexpected error codes the same as ESTALE. This is safest. >> >> Signed-off-by: NeilBrown <neilb@suse.com> >> --- >> >> I didn't add a dprintk for unexpected error messages, partly >> because dprintk isn't usable in exportfs. I could have used pr_debug() >> but I really didn't see much value. >> >> This has been tested together with the btrfs change, and it restores >> correct functionality. > > I don't really like all this magic which is partially historic. I think > we should instead allow the fs to return any error from the export > operations, and forbid returning NULL entirely. Then the actualy caller > (nfsd) can sort out which errors it wants to send over the wire. I'm certainly open to that possibility. But is the "actual caller": nfsd_set_fh_dentry(), or fh_verify() or the various callers of fh_verify() which might have different rules about which error codess are acceptable? I could probably make an argument for having fh_verify() be careful about error codes, but as exportfs_decode_fh() is a more public interface, I think it is more important that it have well defined error options. Are there *any* errors that could sensibly be returned from exportfs_decode_fh() other than -ESTALE (there is no such file), or -ENOMEM (there probably is a file, but I cannot allocate a dentry for it) or -EACCES (there is such a file, but it isn't "acceptable") ??? If there aren't, why should we let them through? NeilBrown
On Thu, Aug 04 2016, NeilBrown wrote: > > > When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors. > In particular it can be tempting to return ENOENT, but this is not > handled well by nfsd. > > Rather than requiring strict adherence to error code code filesystems, > treat all unexpected error codes the same as ESTALE. This is safest. > > Signed-off-by: NeilBrown <neilb@suse.com> > --- > > I didn't add a dprintk for unexpected error messages, partly > because dprintk isn't usable in exportfs. I could have used pr_debug() > but I really didn't see much value. > > This has been tested together with the btrfs change, and it restores > correct functionality. > > Thanks, > NeilBrown Hi Bruce, I notice that this patch isn't in -next, but the btrfs change which motivated it is. That means, unless there is some other work around somewhere, btrfs might start having problems with nfs export. Can we get this patch into 4.9-rc?? Or has someone fixed it a different way? Thanks, NeilBrown > > fs/exportfs/expfs.c | 10 ++++++---- > include/linux/exportfs.h | 13 +++++++------ > 2 files changed, 13 insertions(+), 10 deletions(-) > > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c > index 207ba8d627ca..a4b531be9168 100644 > --- a/fs/exportfs/expfs.c > +++ b/fs/exportfs/expfs.c > @@ -428,10 +428,10 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid, > if (!nop || !nop->fh_to_dentry) > return ERR_PTR(-ESTALE); > result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type); > - if (!result) > - result = ERR_PTR(-ESTALE); > - if (IS_ERR(result)) > - return result; > + if (PTR_ERR(result) == -ENOMEM) > + return ERR_CAST(result); > + if (IS_ERR_OR_NULL(result)) > + return ERR_PTR(-ESTALE); > > if (d_is_dir(result)) { > /* > @@ -541,6 +541,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid, > > err_result: > dput(result); > + if (err != -ENOMEM) > + err = -ESTALE; > return ERR_PTR(err); > } > EXPORT_SYMBOL_GPL(exportfs_decode_fh); > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h > index b03c0625fa6e..5ab958cdc50b 100644 > --- a/include/linux/exportfs.h > +++ b/include/linux/exportfs.h > @@ -157,12 +157,13 @@ struct fid { > * @fh_to_dentry is given a &struct super_block (@sb) and a file handle > * fragment (@fh, @fh_len). It should return a &struct dentry which refers > * to the same file that the file handle fragment refers to. If it cannot, > - * it should return a %NULL pointer if the file was found but no acceptable > - * &dentries were available, or an %ERR_PTR error code indicating why it > - * couldn't be found (e.g. %ENOENT or %ENOMEM). Any suitable dentry can be > - * returned including, if necessary, a new dentry created with d_alloc_root. > - * The caller can then find any other extant dentries by following the > - * d_alias links. > + * it should return a %NULL pointer if the file cannot be found, or an > + * %ERR_PTR error code of %ENOMEM if a memory allocation failure occurred. > + * Any other error code is treated like %NULL, and will cause an %ESTALE error > + * for callers of exportfs_decode_fh(). > + * Any suitable dentry can be returned including, if necessary, a new dentry > + * created with d_alloc_root. The caller can then find any other extant > + * dentries by following the d_alias links. > * > * fh_to_parent: > * Same as @fh_to_dentry, except that it returns a pointer to the parent > -- > 2.9.2
On Thu, Oct 06, 2016 at 05:39:24PM +1100, NeilBrown wrote: > On Thu, Aug 04 2016, NeilBrown wrote: > > > > > > > When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors. > > In particular it can be tempting to return ENOENT, but this is not > > handled well by nfsd. > > > > Rather than requiring strict adherence to error code code filesystems, > > treat all unexpected error codes the same as ESTALE. This is safest. > > > > Signed-off-by: NeilBrown <neilb@suse.com> > > --- > > > > I didn't add a dprintk for unexpected error messages, partly > > because dprintk isn't usable in exportfs. I could have used pr_debug() > > but I really didn't see much value. > > > > This has been tested together with the btrfs change, and it restores > > correct functionality. > > > > Thanks, > > NeilBrown > > > Hi Bruce, > I notice that this patch isn't in -next, but the btrfs change which > motivated it is. > That means, unless there is some other work around somewhere, btrfs > might start having problems with nfs export. Whoops, I wonder what happened. Looking back.... Oh, I think I set it aside pending a response from Christoph, but I don't think that's really necessary. > Can we get this patch into 4.9-rc?? > > Or has someone fixed it a different way? No, I'll just apply. I need to send a pull request soon, I just have to make up my mind on COPY first. --b. > Thanks, > NeilBrown > > > > > > fs/exportfs/expfs.c | 10 ++++++---- > > include/linux/exportfs.h | 13 +++++++------ > > 2 files changed, 13 insertions(+), 10 deletions(-) > > > > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c > > index 207ba8d627ca..a4b531be9168 100644 > > --- a/fs/exportfs/expfs.c > > +++ b/fs/exportfs/expfs.c > > @@ -428,10 +428,10 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid, > > if (!nop || !nop->fh_to_dentry) > > return ERR_PTR(-ESTALE); > > result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type); > > - if (!result) > > - result = ERR_PTR(-ESTALE); > > - if (IS_ERR(result)) > > - return result; > > + if (PTR_ERR(result) == -ENOMEM) > > + return ERR_CAST(result); > > + if (IS_ERR_OR_NULL(result)) > > + return ERR_PTR(-ESTALE); > > > > if (d_is_dir(result)) { > > /* > > @@ -541,6 +541,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid, > > > > err_result: > > dput(result); > > + if (err != -ENOMEM) > > + err = -ESTALE; > > return ERR_PTR(err); > > } > > EXPORT_SYMBOL_GPL(exportfs_decode_fh); > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h > > index b03c0625fa6e..5ab958cdc50b 100644 > > --- a/include/linux/exportfs.h > > +++ b/include/linux/exportfs.h > > @@ -157,12 +157,13 @@ struct fid { > > * @fh_to_dentry is given a &struct super_block (@sb) and a file handle > > * fragment (@fh, @fh_len). It should return a &struct dentry which refers > > * to the same file that the file handle fragment refers to. If it cannot, > > - * it should return a %NULL pointer if the file was found but no acceptable > > - * &dentries were available, or an %ERR_PTR error code indicating why it > > - * couldn't be found (e.g. %ENOENT or %ENOMEM). Any suitable dentry can be > > - * returned including, if necessary, a new dentry created with d_alloc_root. > > - * The caller can then find any other extant dentries by following the > > - * d_alias links. > > + * it should return a %NULL pointer if the file cannot be found, or an > > + * %ERR_PTR error code of %ENOMEM if a memory allocation failure occurred. > > + * Any other error code is treated like %NULL, and will cause an %ESTALE error > > + * for callers of exportfs_decode_fh(). > > + * Any suitable dentry can be returned including, if necessary, a new dentry > > + * created with d_alloc_root. The caller can then find any other extant > > + * dentries by following the d_alias links. > > * > > * fh_to_parent: > > * Same as @fh_to_dentry, except that it returns a pointer to the parent > > -- > > 2.9.2 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c index 207ba8d627ca..a4b531be9168 100644 --- a/fs/exportfs/expfs.c +++ b/fs/exportfs/expfs.c @@ -428,10 +428,10 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid, if (!nop || !nop->fh_to_dentry) return ERR_PTR(-ESTALE); result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type); - if (!result) - result = ERR_PTR(-ESTALE); - if (IS_ERR(result)) - return result; + if (PTR_ERR(result) == -ENOMEM) + return ERR_CAST(result); + if (IS_ERR_OR_NULL(result)) + return ERR_PTR(-ESTALE); if (d_is_dir(result)) { /* @@ -541,6 +541,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid, err_result: dput(result); + if (err != -ENOMEM) + err = -ESTALE; return ERR_PTR(err); } EXPORT_SYMBOL_GPL(exportfs_decode_fh); diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h index b03c0625fa6e..5ab958cdc50b 100644 --- a/include/linux/exportfs.h +++ b/include/linux/exportfs.h @@ -157,12 +157,13 @@ struct fid { * @fh_to_dentry is given a &struct super_block (@sb) and a file handle * fragment (@fh, @fh_len). It should return a &struct dentry which refers * to the same file that the file handle fragment refers to. If it cannot, - * it should return a %NULL pointer if the file was found but no acceptable - * &dentries were available, or an %ERR_PTR error code indicating why it - * couldn't be found (e.g. %ENOENT or %ENOMEM). Any suitable dentry can be - * returned including, if necessary, a new dentry created with d_alloc_root. - * The caller can then find any other extant dentries by following the - * d_alias links. + * it should return a %NULL pointer if the file cannot be found, or an + * %ERR_PTR error code of %ENOMEM if a memory allocation failure occurred. + * Any other error code is treated like %NULL, and will cause an %ESTALE error + * for callers of exportfs_decode_fh(). + * Any suitable dentry can be returned including, if necessary, a new dentry + * created with d_alloc_root. The caller can then find any other extant + * dentries by following the d_alias links. * * fh_to_parent: * Same as @fh_to_dentry, except that it returns a pointer to the parent
When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors. In particular it can be tempting to return ENOENT, but this is not handled well by nfsd. Rather than requiring strict adherence to error code code filesystems, treat all unexpected error codes the same as ESTALE. This is safest. Signed-off-by: NeilBrown <neilb@suse.com> --- I didn't add a dprintk for unexpected error messages, partly because dprintk isn't usable in exportfs. I could have used pr_debug() but I really didn't see much value. This has been tested together with the btrfs change, and it restores correct functionality. Thanks, NeilBrown fs/exportfs/expfs.c | 10 ++++++---- include/linux/exportfs.h | 13 +++++++------ 2 files changed, 13 insertions(+), 10 deletions(-)