[PATCH] exportfs: check for error return value from exportfs_encode_*()

[PATCH] exportfs: check for error return value from exportfs_encode_*()

[Amir Goldstein]

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);
 	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

Re: [PATCH] exportfs: check for error return value from exportfs_encode_*()

[Jeff Layton]

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.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH] exportfs: check for error return value from exportfs_encode_*()

[Amir Goldstein]

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.

Re: [PATCH] exportfs: check for error return value from exportfs_encode_*()

[Jeff Layton]

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>

Re: [PATCH] exportfs: check for error return value from exportfs_encode_*()

[Jan Kara]

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

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR