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 <[email protected]>
Link: https://lore.kernel.org/linux-fsdevel/[email protected]/
Signed-off-by: Amir Goldstein <[email protected]>
---
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);
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;
--
2.34.1
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 <[email protected]>
> Link: https://lore.kernel.org/linux-fsdevel/[email protected]/
> Signed-off-by: Amir Goldstein <[email protected]>
> ---
>
> 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.
--
Jeff Layton <[email protected]>
On Wed, May 24, 2023 at 8:05 PM Jeff Layton <[email protected]> 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 <[email protected]>
> > Link: https://lore.kernel.org/linux-fsdevel/[email protected]/
> > Signed-off-by: Amir Goldstein <[email protected]>
> > ---
> >
> > 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
> > > Link: https://lore.kernel.org/linux-fsdevel/[email protected]/
> > > Signed-off-by: Amir Goldstein <[email protected]>
> > > ---
> > >
> > > 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 <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
Thanks! I've added the patch into my tree.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR