public inbox for linux-nfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] nfsd: do not allow exporting of special kernel filesystems
@ 2026-01-21  8:50 Amir Goldstein
  2026-01-21  9:25 ` Amir Goldstein
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Amir Goldstein @ 2026-01-21  8:50 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Chuck Lever, Jeff Layton, Christoph Hellwig, Neil Brown, Jan Kara,
	linux-fsdevel, linux-nfs

pidfs and nsfs recently gained support for encode/decode of file handles
via name_to_handle_at(2)/opan_by_handle_at(2).

These special kernel filesystems have custom ->open() and ->permission()
export methods, which nfsd does not respect and it was never meant to be
used for exporting those filesystems by nfsd.

Therefore, do not allow nfsd to export filesystems with custom ->open()
or ->permission() methods.

Fixes: b3caba8f7a34a ("pidfs: implement file handle support")
Fixes: 5222470b2fbb3 ("nsfs: support file handles")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/nfsd/export.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

Christian,

I had enough of the stable file handles discussion [1].

This patch which I already suggested [2] on week ago, states a justified
technical reason why pidfs and nsfs should not be exported by nfsd,
so let's use this technical reasoning and stop the philosophic discussions
about what is a stable file handle is please.

Regarding cgroupfs, we can either deal with it later or not - it is not
a clear but as pidfs and nsfs which absolutely should be fixed
retroactively in stable kernels.

If you think that cgroupfs could benefit from "exhaustive" file handles [3]
then we can implement open_by_handle_at(FD_CGROUPFS_ROOT, ... and that
would classify cgroupfs the same as pidfs and nsfs.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20250912-work-namespace-v2-0-1a247645cef5@kernel.org/
[2] https://lore.kernel.org/linux-fsdevel/CAOQ4uxhkaGFtQRzTj2xaf2GJucoAY5CGiyUjB=8YA2zTbOtFvw@mail.gmail.com/
[3] https://lore.kernel.org/linux-fsdevel/20250912-work-namespace-v2-29-1a247645cef5@kernel.org/

diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
index 2a1499f2ad196..232dacac611e9 100644
--- a/fs/nfsd/export.c
+++ b/fs/nfsd/export.c
@@ -405,6 +405,7 @@ static struct svc_export *svc_export_lookup(struct svc_export *);
 static int check_export(const struct path *path, int *flags, unsigned char *uuid)
 {
 	struct inode *inode = d_inode(path->dentry);
+	const struct export_operations *nop = inode->i_sb->s_export_op;
 
 	/*
 	 * We currently export only dirs, regular files, and (for v4
@@ -422,13 +423,12 @@ static int check_export(const struct path *path, int *flags, unsigned char *uuid
 	if (*flags & NFSEXP_V4ROOT)
 		*flags |= NFSEXP_READONLY;
 
-	/* There are two requirements on a filesystem to be exportable.
-	 * 1:  We must be able to identify the filesystem from a number.
-	 *       either a device number (so FS_REQUIRES_DEV needed)
-	 *       or an FSID number (so NFSEXP_FSID or ->uuid is needed).
-	 * 2:  We must be able to find an inode from a filehandle.
-	 *       This means that s_export_op must be set.
-	 * 3: We must not currently be on an idmapped mount.
+	/*
+	 * The requirements for a filesystem to be exportable:
+	 * 1. The filehandle must identify a filesystem by number
+	 * 2. The filehandle must uniquely identify an inode
+	 * 3. The filesystem must not have custom filehandle open/perm methods
+	 * 4. The requested file must not reside on an idmapped mount
 	 */
 	if (!(inode->i_sb->s_type->fs_flags & FS_REQUIRES_DEV) &&
 	    !(*flags & NFSEXP_FSID) &&
@@ -437,11 +437,16 @@ static int check_export(const struct path *path, int *flags, unsigned char *uuid
 		return -EINVAL;
 	}
 
-	if (!exportfs_can_decode_fh(inode->i_sb->s_export_op)) {
+	if (!exportfs_can_decode_fh(nop)) {
 		dprintk("exp_export: export of invalid fs type.\n");
 		return -EINVAL;
 	}
 
+	if (nop->open || nop->permission) {
+		dprintk("exp_export: export of non-standard fs type.\n");
+		return -EINVAL;
+	}
+
 	if (is_idmapped_mnt(path->mnt)) {
 		dprintk("exp_export: export of idmapped mounts not yet supported.\n");
 		return -EINVAL;
-- 
2.52.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] nfsd: do not allow exporting of special kernel filesystems
  2026-01-21  8:50 [PATCH] nfsd: do not allow exporting of special kernel filesystems Amir Goldstein
@ 2026-01-21  9:25 ` Amir Goldstein
  2026-01-21  9:50 ` NeilBrown
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Amir Goldstein @ 2026-01-21  9:25 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Chuck Lever, Jeff Layton, Christoph Hellwig, Jan Kara,
	linux-fsdevel, linux-nfs, NeilBrown

[cc the correct email address for Neil]

On Wed, Jan 21, 2026 at 9:50 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> pidfs and nsfs recently gained support for encode/decode of file handles
> via name_to_handle_at(2)/opan_by_handle_at(2).
>
> These special kernel filesystems have custom ->open() and ->permission()
> export methods, which nfsd does not respect and it was never meant to be
> used for exporting those filesystems by nfsd.
>
> Therefore, do not allow nfsd to export filesystems with custom ->open()
> or ->permission() methods.
>
> Fixes: b3caba8f7a34a ("pidfs: implement file handle support")
> Fixes: 5222470b2fbb3 ("nsfs: support file handles")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/nfsd/export.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
>
> Christian,
>
> I had enough of the stable file handles discussion [1].
>
> This patch which I already suggested [2] on week ago, states a justified
> technical reason why pidfs and nsfs should not be exported by nfsd,
> so let's use this technical reasoning and stop the philosophic discussions
> about what is a stable file handle is please.
>
> Regarding cgroupfs, we can either deal with it later or not - it is not
> a clear but as pidfs and nsfs which absolutely should be fixed
> retroactively in stable kernels.
>
> If you think that cgroupfs could benefit from "exhaustive" file handles [3]
> then we can implement open_by_handle_at(FD_CGROUPFS_ROOT, ... and that
> would classify cgroupfs the same as pidfs and nsfs.
>
> Thanks,
> Amir.
>
> [1] https://lore.kernel.org/linux-fsdevel/20250912-work-namespace-v2-0-1a247645cef5@kernel.org/
> [2] https://lore.kernel.org/linux-fsdevel/CAOQ4uxhkaGFtQRzTj2xaf2GJucoAY5CGiyUjB=8YA2zTbOtFvw@mail.gmail.com/
> [3] https://lore.kernel.org/linux-fsdevel/20250912-work-namespace-v2-29-1a247645cef5@kernel.org/
>
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 2a1499f2ad196..232dacac611e9 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -405,6 +405,7 @@ static struct svc_export *svc_export_lookup(struct svc_export *);
>  static int check_export(const struct path *path, int *flags, unsigned char *uuid)
>  {
>         struct inode *inode = d_inode(path->dentry);
> +       const struct export_operations *nop = inode->i_sb->s_export_op;
>
>         /*
>          * We currently export only dirs, regular files, and (for v4
> @@ -422,13 +423,12 @@ static int check_export(const struct path *path, int *flags, unsigned char *uuid
>         if (*flags & NFSEXP_V4ROOT)
>                 *flags |= NFSEXP_READONLY;
>
> -       /* There are two requirements on a filesystem to be exportable.
> -        * 1:  We must be able to identify the filesystem from a number.
> -        *       either a device number (so FS_REQUIRES_DEV needed)
> -        *       or an FSID number (so NFSEXP_FSID or ->uuid is needed).
> -        * 2:  We must be able to find an inode from a filehandle.
> -        *       This means that s_export_op must be set.
> -        * 3: We must not currently be on an idmapped mount.
> +       /*
> +        * The requirements for a filesystem to be exportable:
> +        * 1. The filehandle must identify a filesystem by number
> +        * 2. The filehandle must uniquely identify an inode
> +        * 3. The filesystem must not have custom filehandle open/perm methods
> +        * 4. The requested file must not reside on an idmapped mount
>          */
>         if (!(inode->i_sb->s_type->fs_flags & FS_REQUIRES_DEV) &&
>             !(*flags & NFSEXP_FSID) &&
> @@ -437,11 +437,16 @@ static int check_export(const struct path *path, int *flags, unsigned char *uuid
>                 return -EINVAL;
>         }
>
> -       if (!exportfs_can_decode_fh(inode->i_sb->s_export_op)) {
> +       if (!exportfs_can_decode_fh(nop)) {
>                 dprintk("exp_export: export of invalid fs type.\n");
>                 return -EINVAL;
>         }
>
> +       if (nop->open || nop->permission) {
> +               dprintk("exp_export: export of non-standard fs type.\n");
> +               return -EINVAL;
> +       }
> +
>         if (is_idmapped_mnt(path->mnt)) {
>                 dprintk("exp_export: export of idmapped mounts not yet supported.\n");
>                 return -EINVAL;
> --
> 2.52.0
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] nfsd: do not allow exporting of special kernel filesystems
  2026-01-21  8:50 [PATCH] nfsd: do not allow exporting of special kernel filesystems Amir Goldstein
  2026-01-21  9:25 ` Amir Goldstein
@ 2026-01-21  9:50 ` NeilBrown
  2026-01-21 10:24   ` Amir Goldstein
  2026-01-21 10:12 ` Christoph Hellwig
  2026-01-21 11:45 ` Jeff Layton
  3 siblings, 1 reply; 9+ messages in thread
From: NeilBrown @ 2026-01-21  9:50 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Christian Brauner, Chuck Lever, Jeff Layton, Christoph Hellwig,
	Jan Kara, linux-fsdevel, linux-nfs

On Wed, 21 Jan 2026, Amir Goldstein wrote:
> pidfs and nsfs recently gained support for encode/decode of file handles
> via name_to_handle_at(2)/opan_by_handle_at(2).
> 
> These special kernel filesystems have custom ->open() and ->permission()
> export methods, which nfsd does not respect and it was never meant to be
> used for exporting those filesystems by nfsd.
> 
> Therefore, do not allow nfsd to export filesystems with custom ->open()
> or ->permission() methods.
> 
> Fixes: b3caba8f7a34a ("pidfs: implement file handle support")
> Fixes: 5222470b2fbb3 ("nsfs: support file handles")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/nfsd/export.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> Christian,
> 
> I had enough of the stable file handles discussion [1].
> 
> This patch which I already suggested [2] on week ago, states a justified
> technical reason why pidfs and nsfs should not be exported by nfsd,
> so let's use this technical reasoning and stop the philosophic discussions
> about what is a stable file handle is please.
> 
> Regarding cgroupfs, we can either deal with it later or not - it is not
> a clear but as pidfs and nsfs which absolutely should be fixed
> retroactively in stable kernels.
> 
> If you think that cgroupfs could benefit from "exhaustive" file handles [3]
> then we can implement open_by_handle_at(FD_CGROUPFS_ROOT, ... and that
> would classify cgroupfs the same as pidfs and nsfs.
> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20250912-work-namespace-v2-0-1a247645cef5@kernel.org/
> [2] https://lore.kernel.org/linux-fsdevel/CAOQ4uxhkaGFtQRzTj2xaf2GJucoAY5CGiyUjB=8YA2zTbOtFvw@mail.gmail.com/
> [3] https://lore.kernel.org/linux-fsdevel/20250912-work-namespace-v2-29-1a247645cef5@kernel.org/
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 2a1499f2ad196..232dacac611e9 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -405,6 +405,7 @@ static struct svc_export *svc_export_lookup(struct svc_export *);
>  static int check_export(const struct path *path, int *flags, unsigned char *uuid)
>  {
>  	struct inode *inode = d_inode(path->dentry);
> +	const struct export_operations *nop = inode->i_sb->s_export_op;
>  
>  	/*
>  	 * We currently export only dirs, regular files, and (for v4
> @@ -422,13 +423,12 @@ static int check_export(const struct path *path, int *flags, unsigned char *uuid
>  	if (*flags & NFSEXP_V4ROOT)
>  		*flags |= NFSEXP_READONLY;
>  
> -	/* There are two requirements on a filesystem to be exportable.
> -	 * 1:  We must be able to identify the filesystem from a number.
> -	 *       either a device number (so FS_REQUIRES_DEV needed)
> -	 *       or an FSID number (so NFSEXP_FSID or ->uuid is needed).
> -	 * 2:  We must be able to find an inode from a filehandle.
> -	 *       This means that s_export_op must be set.
> -	 * 3: We must not currently be on an idmapped mount.
> +	/*
> +	 * The requirements for a filesystem to be exportable:
> +	 * 1. The filehandle must identify a filesystem by number
> +	 * 2. The filehandle must uniquely identify an inode
> +	 * 3. The filesystem must not have custom filehandle open/perm methods
> +	 * 4. The requested file must not reside on an idmapped mount
>  	 */
>  	if (!(inode->i_sb->s_type->fs_flags & FS_REQUIRES_DEV) &&
>  	    !(*flags & NFSEXP_FSID) &&
> @@ -437,11 +437,16 @@ static int check_export(const struct path *path, int *flags, unsigned char *uuid
>  		return -EINVAL;
>  	}
>  
> -	if (!exportfs_can_decode_fh(inode->i_sb->s_export_op)) {
> +	if (!exportfs_can_decode_fh(nop)) {
>  		dprintk("exp_export: export of invalid fs type.\n");
>  		return -EINVAL;
>  	}
>  
> +	if (nop->open || nop->permission) {
> +		dprintk("exp_export: export of non-standard fs type.\n");
> +		return -EINVAL;
> +	}
> +

It is not immediately obvious that this is safe when nop is NULL, but it
is because exportfs_can_decode_fh() checks for that case.  As that is a
static inline a static analyser can easily confirm this.  So it is
probably OK.

"non-standard" is not totally clear.  "special" might be better, or it
might not.  It is only a dprintk so it probably doesn't matter much.

Reviewed-by: NeilBrown <neil@brown.name>

Thanks for posting this.  I think this is a good check to have whether
or not we decide to add a flag.

Thanks,
NeilBrown

>  	if (is_idmapped_mnt(path->mnt)) {
>  		dprintk("exp_export: export of idmapped mounts not yet supported.\n");
>  		return -EINVAL;
> -- 
> 2.52.0
> 
> 
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] nfsd: do not allow exporting of special kernel filesystems
  2026-01-21  8:50 [PATCH] nfsd: do not allow exporting of special kernel filesystems Amir Goldstein
  2026-01-21  9:25 ` Amir Goldstein
  2026-01-21  9:50 ` NeilBrown
@ 2026-01-21 10:12 ` Christoph Hellwig
  2026-01-21 10:35   ` Amir Goldstein
  2026-01-21 11:45 ` Jeff Layton
  3 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2026-01-21 10:12 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Christian Brauner, Chuck Lever, Jeff Layton, Christoph Hellwig,
	Neil Brown, Jan Kara, linux-fsdevel, linux-nfs

On Wed, Jan 21, 2026 at 09:50:27AM +0100, Amir Goldstein wrote:
> pidfs and nsfs recently gained support for encode/decode of file handles
> via name_to_handle_at(2)/opan_by_handle_at(2).
> 
> These special kernel filesystems have custom ->open() and ->permission()
> export methods, which nfsd does not respect and it was never meant to be
> used for exporting those filesystems by nfsd.
> 
> Therefore, do not allow nfsd to export filesystems with custom ->open()
> or ->permission() methods.

Yeah, this was added in and not used in the existing export_ops users.

> +	/*
> +	 * The requirements for a filesystem to be exportable:
> +	 * 1. The filehandle must identify a filesystem by number
> +	 * 2. The filehandle must uniquely identify an inode
> +	 * 3. The filesystem must not have custom filehandle open/perm methods
> +	 * 4. The requested file must not reside on an idmapped mount
>  	 */

Please spell out here why ->open and ->permission are not allowed.
Listing what the code does is generally not that useful, while why
it does that provides value.

While looking this I have to say the API documentation for these
methods in exportfs.h is unfortunately completely useless as well.
It doesn't mention the limitation that it's only used by the
non-exportfs code, and also doesn't mention why a file system
would implement or have to implement them :(  The commit messages
adding them are just as bad as well.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] nfsd: do not allow exporting of special kernel filesystems
  2026-01-21  9:50 ` NeilBrown
@ 2026-01-21 10:24   ` Amir Goldstein
  2026-01-21 14:50     ` Christoph Hellwig
  0 siblings, 1 reply; 9+ messages in thread
From: Amir Goldstein @ 2026-01-21 10:24 UTC (permalink / raw)
  To: NeilBrown
  Cc: Christian Brauner, Chuck Lever, Jeff Layton, Christoph Hellwig,
	Jan Kara, linux-fsdevel, linux-nfs

On Wed, Jan 21, 2026 at 10:50 AM NeilBrown <neilb@ownmail.net> wrote:
>
> On Wed, 21 Jan 2026, Amir Goldstein wrote:
> > pidfs and nsfs recently gained support for encode/decode of file handles
> > via name_to_handle_at(2)/opan_by_handle_at(2).
> >
> > These special kernel filesystems have custom ->open() and ->permission()
> > export methods, which nfsd does not respect and it was never meant to be
> > used for exporting those filesystems by nfsd.
> >
> > Therefore, do not allow nfsd to export filesystems with custom ->open()
> > or ->permission() methods.
> >
> > Fixes: b3caba8f7a34a ("pidfs: implement file handle support")
> > Fixes: 5222470b2fbb3 ("nsfs: support file handles")
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/nfsd/export.c | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > Christian,
> >
> > I had enough of the stable file handles discussion [1].
> >
> > This patch which I already suggested [2] on week ago, states a justified
> > technical reason why pidfs and nsfs should not be exported by nfsd,
> > so let's use this technical reasoning and stop the philosophic discussions
> > about what is a stable file handle is please.
> >
> > Regarding cgroupfs, we can either deal with it later or not - it is not
> > a clear but as pidfs and nsfs which absolutely should be fixed
> > retroactively in stable kernels.
> >
> > If you think that cgroupfs could benefit from "exhaustive" file handles [3]
> > then we can implement open_by_handle_at(FD_CGROUPFS_ROOT, ... and that
> > would classify cgroupfs the same as pidfs and nsfs.
> >
> > Thanks,
> > Amir.
> >
> > [1] https://lore.kernel.org/linux-fsdevel/20250912-work-namespace-v2-0-1a247645cef5@kernel.org/
> > [2] https://lore.kernel.org/linux-fsdevel/CAOQ4uxhkaGFtQRzTj2xaf2GJucoAY5CGiyUjB=8YA2zTbOtFvw@mail.gmail.com/
> > [3] https://lore.kernel.org/linux-fsdevel/20250912-work-namespace-v2-29-1a247645cef5@kernel.org/
> >
> > diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> > index 2a1499f2ad196..232dacac611e9 100644
> > --- a/fs/nfsd/export.c
> > +++ b/fs/nfsd/export.c
> > @@ -405,6 +405,7 @@ static struct svc_export *svc_export_lookup(struct svc_export *);
> >  static int check_export(const struct path *path, int *flags, unsigned char *uuid)
> >  {
> >       struct inode *inode = d_inode(path->dentry);
> > +     const struct export_operations *nop = inode->i_sb->s_export_op;
> >
> >       /*
> >        * We currently export only dirs, regular files, and (for v4
> > @@ -422,13 +423,12 @@ static int check_export(const struct path *path, int *flags, unsigned char *uuid
> >       if (*flags & NFSEXP_V4ROOT)
> >               *flags |= NFSEXP_READONLY;
> >
> > -     /* There are two requirements on a filesystem to be exportable.
> > -      * 1:  We must be able to identify the filesystem from a number.
> > -      *       either a device number (so FS_REQUIRES_DEV needed)
> > -      *       or an FSID number (so NFSEXP_FSID or ->uuid is needed).
> > -      * 2:  We must be able to find an inode from a filehandle.
> > -      *       This means that s_export_op must be set.
> > -      * 3: We must not currently be on an idmapped mount.
> > +     /*
> > +      * The requirements for a filesystem to be exportable:
> > +      * 1. The filehandle must identify a filesystem by number
> > +      * 2. The filehandle must uniquely identify an inode
> > +      * 3. The filesystem must not have custom filehandle open/perm methods
> > +      * 4. The requested file must not reside on an idmapped mount
> >        */
> >       if (!(inode->i_sb->s_type->fs_flags & FS_REQUIRES_DEV) &&
> >           !(*flags & NFSEXP_FSID) &&
> > @@ -437,11 +437,16 @@ static int check_export(const struct path *path, int *flags, unsigned char *uuid
> >               return -EINVAL;
> >       }
> >
> > -     if (!exportfs_can_decode_fh(inode->i_sb->s_export_op)) {
> > +     if (!exportfs_can_decode_fh(nop)) {
> >               dprintk("exp_export: export of invalid fs type.\n");
> >               return -EINVAL;
> >       }
> >
> > +     if (nop->open || nop->permission) {
> > +             dprintk("exp_export: export of non-standard fs type.\n");
> > +             return -EINVAL;
> > +     }
> > +
>
> It is not immediately obvious that this is safe when nop is NULL, but it
> is because exportfs_can_decode_fh() checks for that case.  As that is a
> static inline a static analyser can easily confirm this.  So it is
> probably OK.

Heh, in the RFC patch [1], I had those conditions wrapped in a helper
just below exportfs_can_decode_fh(), so this was more clear, but now
I tried to avoid adding a helper named exportfs_may_nfs_export()... ;)

>
> "non-standard" is not totally clear.  "special" might be better, or it
> might not.  It is only a dprintk so it probably doesn't matter much.
>

In said RFC patch it was using the same dprinkt() ("invalid fs type")
I am not sure that the distinction to two different classes is important.
printing the fstype->name, either with dprintk() or trace point would
be much more valuable IMO.

But Jeff has a patch to convert dprink() to trace point, so I will gladly
leave those decisions for him.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/CAOQ4uxhkaGFtQRzTj2xaf2GJucoAY5CGiyUjB=8YA2zTbOtFvw@mail.gmail.com/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] nfsd: do not allow exporting of special kernel filesystems
  2026-01-21 10:12 ` Christoph Hellwig
@ 2026-01-21 10:35   ` Amir Goldstein
  2026-01-21 14:53     ` Christoph Hellwig
  0 siblings, 1 reply; 9+ messages in thread
From: Amir Goldstein @ 2026-01-21 10:35 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Christian Brauner, Chuck Lever, Jeff Layton, Neil Brown, Jan Kara,
	linux-fsdevel, linux-nfs

On Wed, Jan 21, 2026 at 11:12 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Wed, Jan 21, 2026 at 09:50:27AM +0100, Amir Goldstein wrote:
> > pidfs and nsfs recently gained support for encode/decode of file handles
> > via name_to_handle_at(2)/opan_by_handle_at(2).
> >
> > These special kernel filesystems have custom ->open() and ->permission()
> > export methods, which nfsd does not respect and it was never meant to be
> > used for exporting those filesystems by nfsd.
> >
> > Therefore, do not allow nfsd to export filesystems with custom ->open()
> > or ->permission() methods.
>
> Yeah, this was added in and not used in the existing export_ops users.
>

Not used in existing users (nfsd) on purpose to my understanding
That's the point of this patch - to fix this misunderstanding

> > +     /*
> > +      * The requirements for a filesystem to be exportable:
> > +      * 1. The filehandle must identify a filesystem by number
> > +      * 2. The filehandle must uniquely identify an inode
> > +      * 3. The filesystem must not have custom filehandle open/perm methods
> > +      * 4. The requested file must not reside on an idmapped mount
> >        */
>
> Please spell out here why ->open and ->permission are not allowed.
> Listing what the code does is generally not that useful, while why
> it does that provides value.

This is what I had in the RFC patch:
/*
+ * Do not allow exporting to NFS filesystems with custom ->open() and
+ * ->permission() ops, which nfsd does not respect (e.g. pidfs, nsfs).
+ */

I took Chuck's suggestion to rewrite the requirements, but TBH,
I'd rather not touch the existing comment myself at all.
I prefer that Check and Jeff apply a separate patch to rewrite the
documentation if they feel that this is needed or to propose the
phrasing that they prefer.

>
> While looking this I have to say the API documentation for these
> methods in exportfs.h is unfortunately completely useless as well.
> It doesn't mention the limitation that it's only used by the
> non-exportfs code, and also doesn't mention why a file system
> would implement or have to implement them :(  The commit messages
> adding them are just as bad as well.

I will leave that to Christian for a followup patch or to suggest
the phrasing.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] nfsd: do not allow exporting of special kernel filesystems
  2026-01-21  8:50 [PATCH] nfsd: do not allow exporting of special kernel filesystems Amir Goldstein
                   ` (2 preceding siblings ...)
  2026-01-21 10:12 ` Christoph Hellwig
@ 2026-01-21 11:45 ` Jeff Layton
  3 siblings, 0 replies; 9+ messages in thread
From: Jeff Layton @ 2026-01-21 11:45 UTC (permalink / raw)
  To: Amir Goldstein, Christian Brauner
  Cc: Chuck Lever, Christoph Hellwig, Neil Brown, Jan Kara,
	linux-fsdevel, linux-nfs

On Wed, 2026-01-21 at 09:50 +0100, Amir Goldstein wrote:
> pidfs and nsfs recently gained support for encode/decode of file handles
> via name_to_handle_at(2)/opan_by_handle_at(2).
> 
> These special kernel filesystems have custom ->open() and ->permission()
> export methods, which nfsd does not respect and it was never meant to be
> used for exporting those filesystems by nfsd.
> 
> Therefore, do not allow nfsd to export filesystems with custom ->open()
> or ->permission() methods.
> 
> Fixes: b3caba8f7a34a ("pidfs: implement file handle support")
> Fixes: 5222470b2fbb3 ("nsfs: support file handles")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/nfsd/export.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> Christian,
> 
> I had enough of the stable file handles discussion [1].
> 
> This patch which I already suggested [2] on week ago, states a justified
> technical reason why pidfs and nsfs should not be exported by nfsd,
> so let's use this technical reasoning and stop the philosophic discussions
> about what is a stable file handle is please.
> 
> Regarding cgroupfs, we can either deal with it later or not - it is not
> a clear but as pidfs and nsfs which absolutely should be fixed
> retroactively in stable kernels.
> 
> If you think that cgroupfs could benefit from "exhaustive" file handles [3]
> then we can implement open_by_handle_at(FD_CGROUPFS_ROOT, ... and that
> would classify cgroupfs the same as pidfs and nsfs.
> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20250912-work-namespace-v2-0-1a247645cef5@kernel.org/
> [2] https://lore.kernel.org/linux-fsdevel/CAOQ4uxhkaGFtQRzTj2xaf2GJucoAY5CGiyUjB=8YA2zTbOtFvw@mail.gmail.com/
> [3] https://lore.kernel.org/linux-fsdevel/20250912-work-namespace-v2-29-1a247645cef5@kernel.org/
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index 2a1499f2ad196..232dacac611e9 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -405,6 +405,7 @@ static struct svc_export *svc_export_lookup(struct svc_export *);
>  static int check_export(const struct path *path, int *flags, unsigned char *uuid)
>  {
>  	struct inode *inode = d_inode(path->dentry);
> +	const struct export_operations *nop = inode->i_sb->s_export_op;
>  
>  	/*
>  	 * We currently export only dirs, regular files, and (for v4
> @@ -422,13 +423,12 @@ static int check_export(const struct path *path, int *flags, unsigned char *uuid
>  	if (*flags & NFSEXP_V4ROOT)
>  		*flags |= NFSEXP_READONLY;
>  
> -	/* There are two requirements on a filesystem to be exportable.
> -	 * 1:  We must be able to identify the filesystem from a number.
> -	 *       either a device number (so FS_REQUIRES_DEV needed)
> -	 *       or an FSID number (so NFSEXP_FSID or ->uuid is needed).
> -	 * 2:  We must be able to find an inode from a filehandle.
> -	 *       This means that s_export_op must be set.
> -	 * 3: We must not currently be on an idmapped mount.
> +	/*
> +	 * The requirements for a filesystem to be exportable:
> +	 * 1. The filehandle must identify a filesystem by number
> +	 * 2. The filehandle must uniquely identify an inode
> +	 * 3. The filesystem must not have custom filehandle open/perm methods
> +	 * 4. The requested file must not reside on an idmapped mount
>  	 */
>  	if (!(inode->i_sb->s_type->fs_flags & FS_REQUIRES_DEV) &&
>  	    !(*flags & NFSEXP_FSID) &&
> @@ -437,11 +437,16 @@ static int check_export(const struct path *path, int *flags, unsigned char *uuid
>  		return -EINVAL;
>  	}
>  
> -	if (!exportfs_can_decode_fh(inode->i_sb->s_export_op)) {
> +	if (!exportfs_can_decode_fh(nop)) {
>  		dprintk("exp_export: export of invalid fs type.\n");
>  		return -EINVAL;
>  	}
>  
> +	if (nop->open || nop->permission) {
> +		dprintk("exp_export: export of non-standard fs type.\n");
> +		return -EINVAL;
> +	}
> +
>  	if (is_idmapped_mnt(path->mnt)) {
>  		dprintk("exp_export: export of idmapped mounts not yet supported.\n");
>  		return -EINVAL;

Seems reasonable, though I agree with HCH that you should explain why
we're excluding these in the "requirements" above.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] nfsd: do not allow exporting of special kernel filesystems
  2026-01-21 10:24   ` Amir Goldstein
@ 2026-01-21 14:50     ` Christoph Hellwig
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2026-01-21 14:50 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: NeilBrown, Christian Brauner, Chuck Lever, Jeff Layton,
	Christoph Hellwig, Jan Kara, linux-fsdevel, linux-nfs

On Wed, Jan 21, 2026 at 11:24:47AM +0100, Amir Goldstein wrote:
> > It is not immediately obvious that this is safe when nop is NULL, but it
> > is because exportfs_can_decode_fh() checks for that case.  As that is a
> > static inline a static analyser can easily confirm this.  So it is
> > probably OK.
> 
> Heh, in the RFC patch [1], I had those conditions wrapped in a helper
> just below exportfs_can_decode_fh(), so this was more clear, but now
> I tried to avoid adding a helper named exportfs_may_nfs_export()... ;)

Please drop the nfs - exportfs is a generic layer, and not tried to
nfs, even if that's currently the only user.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] nfsd: do not allow exporting of special kernel filesystems
  2026-01-21 10:35   ` Amir Goldstein
@ 2026-01-21 14:53     ` Christoph Hellwig
  0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2026-01-21 14:53 UTC (permalink / raw)
  To: Amir Goldstein
  Cc: Christoph Hellwig, Christian Brauner, Chuck Lever, Jeff Layton,
	Neil Brown, Jan Kara, linux-fsdevel, linux-nfs

On Wed, Jan 21, 2026 at 11:35:11AM +0100, Amir Goldstein wrote:
> On Wed, Jan 21, 2026 at 11:12 AM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Wed, Jan 21, 2026 at 09:50:27AM +0100, Amir Goldstein wrote:
> > > pidfs and nsfs recently gained support for encode/decode of file handles
> > > via name_to_handle_at(2)/opan_by_handle_at(2).
> > >
> > > These special kernel filesystems have custom ->open() and ->permission()
> > > export methods, which nfsd does not respect and it was never meant to be
> > > used for exporting those filesystems by nfsd.
> > >
> > > Therefore, do not allow nfsd to export filesystems with custom ->open()
> > > or ->permission() methods.
> >
> > Yeah, this was added in and not used in the existing export_ops users.
> >
> 
> Not used in existing users (nfsd) on purpose to my understanding
> That's the point of this patch - to fix this misunderstanding

Well, I've dug through the documentation and commits that added it,
and there's absolutely nothing explaining the intent unfortunately.

> >
> > Please spell out here why ->open and ->permission are not allowed.
> > Listing what the code does is generally not that useful, while why
> > it does that provides value.
> 
> This is what I had in the RFC patch:
> /*
> + * Do not allow exporting to NFS filesystems with custom ->open() and
> + * ->permission() ops, which nfsd does not respect (e.g. pidfs, nsfs).
> + */
> 
> I took Chuck's suggestion to rewrite the requirements, but TBH,
> I'd rather not touch the existing comment myself at all.
> I prefer that Check and Jeff apply a separate patch to rewrite the
> documentation if they feel that this is needed or to propose the
> phrasing that they prefer.

I don't really care who writes the documentation, but if we reject
based on the presence of the methods we really need to document
the why.  

> > While looking this I have to say the API documentation for these
> > methods in exportfs.h is unfortunately completely useless as well.
> > It doesn't mention the limitation that it's only used by the
> > non-exportfs code, and also doesn't mention why a file system
> > would implement or have to implement them :(  The commit messages
> > adding them are just as bad as well.
> 
> I will leave that to Christian for a followup patch or to suggest
> the phrasing.

Yes, I think we really need a braindump from Christian here.  If
you don't want to add that to the documentation I'll find some
time to include it into a revision, because documenting these
kinds of things is really essential.  We're already confused only
2 years after this was added, and it's only going to get worse.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-01-21 14:53 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-21  8:50 [PATCH] nfsd: do not allow exporting of special kernel filesystems Amir Goldstein
2026-01-21  9:25 ` Amir Goldstein
2026-01-21  9:50 ` NeilBrown
2026-01-21 10:24   ` Amir Goldstein
2026-01-21 14:50     ` Christoph Hellwig
2026-01-21 10:12 ` Christoph Hellwig
2026-01-21 10:35   ` Amir Goldstein
2026-01-21 14:53     ` Christoph Hellwig
2026-01-21 11:45 ` Jeff Layton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox