| Message ID | 20230524154825.881414-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exportfs: check for error return value from exportfs_encode_*() | expand |
On Wed, 2023-05-24 at 18:48 +0300, Amir Goldstein wrote: > The exportfs_encode_*() helpers call the filesystem ->encode_fh() > method which returns a signed int. > > All the in-tree implementations of ->encode_fh() return a positive > integer and FILEID_INVALID (255) for error. > > Fortify the callers for possible future ->encode_fh() implementation > that will return a negative error value. > > name_to_handle_at() would propagate the returned error to the users > if filesystem ->encode_fh() method returns an error. > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Link: https://lore.kernel.org/linux-fsdevel/ca02955f-1877-4fde-b453-3c1d22794740@kili.mountain/ > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > Jan, > > This patch is on top of the patches you have queued on fsnotify branch. > > I am not sure about the handling of negative value in nfsfh.c. > > Jeff/Chuck, > > Could you please take a look. > > I've test this patch with fanotify LTP tests, xfstest -g exportfs tests > and some sanity xfstest nfs tests, but I did not try to inject errors > in encode_fh(). > > Please let me know what you think. > > Thanks, > Amir. > > > > fs/fhandle.c | 5 +++-- > fs/nfsd/nfsfh.c | 4 +++- > fs/notify/fanotify/fanotify.c | 2 +- > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/fs/fhandle.c b/fs/fhandle.c > index 4a635cf787fc..fd0d6a3b3699 100644 > --- a/fs/fhandle.c > +++ b/fs/fhandle.c > @@ -57,18 +57,19 @@ static long do_sys_name_to_handle(const struct path *path, > handle_bytes = handle_dwords * sizeof(u32); > handle->handle_bytes = handle_bytes; > if ((handle->handle_bytes > f_handle.handle_bytes) || > - (retval == FILEID_INVALID) || (retval == -ENOSPC)) { > + (retval == FILEID_INVALID) || (retval < 0)) { > /* As per old exportfs_encode_fh documentation > * we could return ENOSPC to indicate overflow > * But file system returned 255 always. So handle > * both the values > */ > + if (retval == FILEID_INVALID || retval == -ENOSPC) > + retval = -EOVERFLOW; > /* > * set the handle size to zero so we copy only > * non variable part of the file_handle > */ > handle_bytes = 0; > - retval = -EOVERFLOW; > } else > retval = 0; > /* copy the mount id */ > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > index 31e4505c0df3..0f5eacae5f43 100644 > --- a/fs/nfsd/nfsfh.c > +++ b/fs/nfsd/nfsfh.c > @@ -416,9 +416,11 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp, > int maxsize = (fhp->fh_maxsize - fhp->fh_handle.fh_size)/4; > int fh_flags = (exp->ex_flags & NFSEXP_NOSUBTREECHECK) ? 0 : > EXPORT_FH_CONNECTABLE; > + int fileid_type = > + exportfs_encode_fh(dentry, fid, &maxsize, fh_flags); > > fhp->fh_handle.fh_fileid_type = > - exportfs_encode_fh(dentry, fid, &maxsize, fh_flags); > + fileid_type > 0 ? fileid_type : FILEID_INVALID; > fhp->fh_handle.fh_size += maxsize * 4; > } else { > fhp->fh_handle.fh_fileid_type = FILEID_ROOT; > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index d2bbf1445a9e..9dac7f6e72d2 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -445,7 +445,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, > dwords = fh_len >> 2; > type = exportfs_encode_fid(inode, buf, &dwords); Are you sure this patch is against the HEAD? My tree has this call as exportfs_encode_inode_fh. > err = -EINVAL; > - if (!type || type == FILEID_INVALID || fh_len != dwords << 2) > + if (type <= 0 || type == FILEID_INVALID || fh_len != dwords << 2) > goto out_err; > > fh->type = type; I'm generally in favor of better guardrails for these sorts of operations, so ACK on the general idea around the patch.
On Wed, May 24, 2023 at 8:05 PM Jeff Layton <jlayton@kernel.org> wrote: > > On Wed, 2023-05-24 at 18:48 +0300, Amir Goldstein wrote: > > The exportfs_encode_*() helpers call the filesystem ->encode_fh() > > method which returns a signed int. > > > > All the in-tree implementations of ->encode_fh() return a positive > > integer and FILEID_INVALID (255) for error. > > > > Fortify the callers for possible future ->encode_fh() implementation > > that will return a negative error value. > > > > name_to_handle_at() would propagate the returned error to the users > > if filesystem ->encode_fh() method returns an error. > > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Link: https://lore.kernel.org/linux-fsdevel/ca02955f-1877-4fde-b453-3c1d22794740@kili.mountain/ > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > Jan, > > > > This patch is on top of the patches you have queued on fsnotify branch. > > > > I am not sure about the handling of negative value in nfsfh.c. > > > > Jeff/Chuck, > > > > Could you please take a look. > > > > I've test this patch with fanotify LTP tests, xfstest -g exportfs tests > > and some sanity xfstest nfs tests, but I did not try to inject errors > > in encode_fh(). > > > > Please let me know what you think. > > > > Thanks, > > Amir. > > > > > > > > fs/fhandle.c | 5 +++-- > > fs/nfsd/nfsfh.c | 4 +++- > > fs/notify/fanotify/fanotify.c | 2 +- > > 3 files changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/fs/fhandle.c b/fs/fhandle.c > > index 4a635cf787fc..fd0d6a3b3699 100644 > > --- a/fs/fhandle.c > > +++ b/fs/fhandle.c > > @@ -57,18 +57,19 @@ static long do_sys_name_to_handle(const struct path *path, > > handle_bytes = handle_dwords * sizeof(u32); > > handle->handle_bytes = handle_bytes; > > if ((handle->handle_bytes > f_handle.handle_bytes) || > > - (retval == FILEID_INVALID) || (retval == -ENOSPC)) { > > + (retval == FILEID_INVALID) || (retval < 0)) { > > /* As per old exportfs_encode_fh documentation > > * we could return ENOSPC to indicate overflow > > * But file system returned 255 always. So handle > > * both the values > > */ > > + if (retval == FILEID_INVALID || retval == -ENOSPC) > > + retval = -EOVERFLOW; > > /* > > * set the handle size to zero so we copy only > > * non variable part of the file_handle > > */ > > handle_bytes = 0; > > - retval = -EOVERFLOW; > > } else > > retval = 0; > > /* copy the mount id */ > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > > index 31e4505c0df3..0f5eacae5f43 100644 > > --- a/fs/nfsd/nfsfh.c > > +++ b/fs/nfsd/nfsfh.c > > @@ -416,9 +416,11 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp, > > int maxsize = (fhp->fh_maxsize - fhp->fh_handle.fh_size)/4; > > int fh_flags = (exp->ex_flags & NFSEXP_NOSUBTREECHECK) ? 0 : > > EXPORT_FH_CONNECTABLE; > > + int fileid_type = > > + exportfs_encode_fh(dentry, fid, &maxsize, fh_flags); > > > > fhp->fh_handle.fh_fileid_type = > > - exportfs_encode_fh(dentry, fid, &maxsize, fh_flags); > > + fileid_type > 0 ? fileid_type : FILEID_INVALID; > > fhp->fh_handle.fh_size += maxsize * 4; Specifically, I wanted to know what nfs developers think that updating fh_size should be skipped for invalid type? or doesn't matter? > > } else { > > fhp->fh_handle.fh_fileid_type = FILEID_ROOT; > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > index d2bbf1445a9e..9dac7f6e72d2 100644 > > --- a/fs/notify/fanotify/fanotify.c > > +++ b/fs/notify/fanotify/fanotify.c > > @@ -445,7 +445,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, > > dwords = fh_len >> 2; > > type = exportfs_encode_fid(inode, buf, &dwords); > > Are you sure this patch is against the HEAD? My tree has this call as > exportfs_encode_inode_fh. It isn't "This patch is on top of the patches you have queued on fsnotify branch." It could be applied also in the beginning of the encode_fid series in case anyone would be interested to backport this one. There is a minor conflict with the first "connectable" flag patch. If needed, I can post rebased series. > > > err = -EINVAL; > > - if (!type || type == FILEID_INVALID || fh_len != dwords << 2) > > + if (type <= 0 || type == FILEID_INVALID || fh_len != dwords << 2) > > > goto out_err; > > > > fh->type = type; > > I'm generally in favor of better guardrails for these sorts of > operations, so ACK on the general idea around the patch. > > -- > Jeff Layton <jlayton@kernel.org> Beyond the general idea, do you also ACK my fix to _fh_update() above? I wasn't 100% sure about it. Thanks, Amir.
On Wed, 2023-05-24 at 20:24 +0300, Amir Goldstein wrote: > On Wed, May 24, 2023 at 8:05 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > On Wed, 2023-05-24 at 18:48 +0300, Amir Goldstein wrote: > > > The exportfs_encode_*() helpers call the filesystem ->encode_fh() > > > method which returns a signed int. > > > > > > All the in-tree implementations of ->encode_fh() return a positive > > > integer and FILEID_INVALID (255) for error. > > > > > > Fortify the callers for possible future ->encode_fh() implementation > > > that will return a negative error value. > > > > > > name_to_handle_at() would propagate the returned error to the users > > > if filesystem ->encode_fh() method returns an error. > > > > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > > Link: https://lore.kernel.org/linux-fsdevel/ca02955f-1877-4fde-b453-3c1d22794740@kili.mountain/ > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > > > > Jan, > > > > > > This patch is on top of the patches you have queued on fsnotify branch. > > > > > > I am not sure about the handling of negative value in nfsfh.c. > > > > > > Jeff/Chuck, > > > > > > Could you please take a look. > > > > > > I've test this patch with fanotify LTP tests, xfstest -g exportfs tests > > > and some sanity xfstest nfs tests, but I did not try to inject errors > > > in encode_fh(). > > > > > > Please let me know what you think. > > > > > > Thanks, > > > Amir. > > > > > > > > > > > > fs/fhandle.c | 5 +++-- > > > fs/nfsd/nfsfh.c | 4 +++- > > > fs/notify/fanotify/fanotify.c | 2 +- > > > 3 files changed, 7 insertions(+), 4 deletions(-) > > > > > > diff --git a/fs/fhandle.c b/fs/fhandle.c > > > index 4a635cf787fc..fd0d6a3b3699 100644 > > > --- a/fs/fhandle.c > > > +++ b/fs/fhandle.c > > > @@ -57,18 +57,19 @@ static long do_sys_name_to_handle(const struct path *path, > > > handle_bytes = handle_dwords * sizeof(u32); > > > handle->handle_bytes = handle_bytes; > > > if ((handle->handle_bytes > f_handle.handle_bytes) || > > > - (retval == FILEID_INVALID) || (retval == -ENOSPC)) { > > > + (retval == FILEID_INVALID) || (retval < 0)) { > > > /* As per old exportfs_encode_fh documentation > > > * we could return ENOSPC to indicate overflow > > > * But file system returned 255 always. So handle > > > * both the values > > > */ > > > + if (retval == FILEID_INVALID || retval == -ENOSPC) > > > + retval = -EOVERFLOW; > > > /* > > > * set the handle size to zero so we copy only > > > * non variable part of the file_handle > > > */ > > > handle_bytes = 0; > > > - retval = -EOVERFLOW; > > > } else > > > retval = 0; > > > /* copy the mount id */ > > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > > > index 31e4505c0df3..0f5eacae5f43 100644 > > > --- a/fs/nfsd/nfsfh.c > > > +++ b/fs/nfsd/nfsfh.c > > > @@ -416,9 +416,11 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp, > > > int maxsize = (fhp->fh_maxsize - fhp->fh_handle.fh_size)/4; > > > int fh_flags = (exp->ex_flags & NFSEXP_NOSUBTREECHECK) ? 0 : > > > EXPORT_FH_CONNECTABLE; > > > + int fileid_type = > > > + exportfs_encode_fh(dentry, fid, &maxsize, fh_flags); > > > > > > fhp->fh_handle.fh_fileid_type = > > > - exportfs_encode_fh(dentry, fid, &maxsize, fh_flags); > > > + fileid_type > 0 ? fileid_type : FILEID_INVALID; > > > fhp->fh_handle.fh_size += maxsize * 4; > > Specifically, I wanted to know what nfs developers think that updating > fh_size should be skipped for invalid type? or doesn't matter? > It doesn't matter. When the callers see the type set to FILEID_INVALID, they'll go into error handling anyway and shouldn't try to do anything further with the size. > > > } else { > > > fhp->fh_handle.fh_fileid_type = FILEID_ROOT; > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > > index d2bbf1445a9e..9dac7f6e72d2 100644 > > > --- a/fs/notify/fanotify/fanotify.c > > > +++ b/fs/notify/fanotify/fanotify.c > > > @@ -445,7 +445,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, > > > dwords = fh_len >> 2; > > > type = exportfs_encode_fid(inode, buf, &dwords); > > > > Are you sure this patch is against the HEAD? My tree has this call as > > exportfs_encode_inode_fh. > > It isn't > > "This patch is on top of the patches you have queued on fsnotify branch." > > It could be applied also in the beginning of the encode_fid series > in case anyone would be interested to backport this one. > There is a minor conflict with the first "connectable" flag patch. > If needed, I can post rebased series. > It's not a big deal. I just wanted to make sure we didn't end up with merge conflicts. > > > > > err = -EINVAL; > > > - if (!type || type == FILEID_INVALID || fh_len != dwords << 2) > > > + if (type <= 0 || type == FILEID_INVALID || fh_len != dwords << 2) > > > > > goto out_err; > > > > > > fh->type = type; > > > > I'm generally in favor of better guardrails for these sorts of > > operations, so ACK on the general idea around the patch. > > > > -- > > Jeff Layton <jlayton@kernel.org> > > Beyond the general idea, do you also ACK my fix to _fh_update() > above? I wasn't 100% sure about it. > That looks like the right way to handle _fh_update(). I think that should also make it also treat a value of 0 as an error, which seems like the right thing to do (even of no caller tries to do that today). Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Wed 24-05-23 13:36:11, Jeff Layton wrote: > On Wed, 2023-05-24 at 20:24 +0300, Amir Goldstein wrote: > > On Wed, May 24, 2023 at 8:05 PM Jeff Layton <jlayton@kernel.org> wrote: ... > > > > Beyond the general idea, do you also ACK my fix to _fh_update() > > above? I wasn't 100% sure about it. > > > > That looks like the right way to handle _fh_update(). I think that > should also make it also treat a value of 0 as an error, which seems > like the right thing to do (even of no caller tries to do that today). > > Reviewed-by: Jeff Layton <jlayton@kernel.org> Thanks! I've added the patch into my tree. Honza
diff --git a/fs/fhandle.c b/fs/fhandle.c index 4a635cf787fc..fd0d6a3b3699 100644 --- a/fs/fhandle.c +++ b/fs/fhandle.c @@ -57,18 +57,19 @@ static long do_sys_name_to_handle(const struct path *path, handle_bytes = handle_dwords * sizeof(u32); handle->handle_bytes = handle_bytes; if ((handle->handle_bytes > f_handle.handle_bytes) || - (retval == FILEID_INVALID) || (retval == -ENOSPC)) { + (retval == FILEID_INVALID) || (retval < 0)) { /* As per old exportfs_encode_fh documentation * we could return ENOSPC to indicate overflow * But file system returned 255 always. So handle * both the values */ + if (retval == FILEID_INVALID || retval == -ENOSPC) + retval = -EOVERFLOW; /* * set the handle size to zero so we copy only * non variable part of the file_handle */ handle_bytes = 0; - retval = -EOVERFLOW; } else retval = 0; /* copy the mount id */ diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c index 31e4505c0df3..0f5eacae5f43 100644 --- a/fs/nfsd/nfsfh.c +++ b/fs/nfsd/nfsfh.c @@ -416,9 +416,11 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp, int maxsize = (fhp->fh_maxsize - fhp->fh_handle.fh_size)/4; int fh_flags = (exp->ex_flags & NFSEXP_NOSUBTREECHECK) ? 0 : EXPORT_FH_CONNECTABLE; + int fileid_type = + exportfs_encode_fh(dentry, fid, &maxsize, fh_flags); fhp->fh_handle.fh_fileid_type = - exportfs_encode_fh(dentry, fid, &maxsize, fh_flags); + fileid_type > 0 ? fileid_type : FILEID_INVALID; fhp->fh_handle.fh_size += maxsize * 4; } else { fhp->fh_handle.fh_fileid_type = FILEID_ROOT; diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index d2bbf1445a9e..9dac7f6e72d2 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -445,7 +445,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode, dwords = fh_len >> 2; type = exportfs_encode_fid(inode, buf, &dwords); err = -EINVAL; - if (!type || type == FILEID_INVALID || fh_len != dwords << 2) + if (type <= 0 || type == FILEID_INVALID || fh_len != dwords << 2) goto out_err; fh->type = type;
The exportfs_encode_*() helpers call the filesystem ->encode_fh() method which returns a signed int. All the in-tree implementations of ->encode_fh() return a positive integer and FILEID_INVALID (255) for error. Fortify the callers for possible future ->encode_fh() implementation that will return a negative error value. name_to_handle_at() would propagate the returned error to the users if filesystem ->encode_fh() method returns an error. Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Link: https://lore.kernel.org/linux-fsdevel/ca02955f-1877-4fde-b453-3c1d22794740@kili.mountain/ Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Jan, This patch is on top of the patches you have queued on fsnotify branch. I am not sure about the handling of negative value in nfsfh.c. Jeff/Chuck, Could you please take a look. I've test this patch with fanotify LTP tests, xfstest -g exportfs tests and some sanity xfstest nfs tests, but I did not try to inject errors in encode_fh(). Please let me know what you think. Thanks, Amir. fs/fhandle.c | 5 +++-- fs/nfsd/nfsfh.c | 4 +++- fs/notify/fanotify/fanotify.c | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-)