Hey,

Some filesystems like kernfs and pidfs support file handles as a
convenience to enable the use of name_to_handle_at(2) and
open_by_handle_at(2) but don't want to and cannot be reliably exported.
Add a flag that allows them to mark their export operations accordingly
and make NFS check for its presence.

@Amir, I'll reorder the patches such that this series comes prior to the
pidfs file handle series. Doing it that way will mean that there's never
a state where pidfs supports file handles while also being exportable.
It's probably not a big deal but it's definitely cleaner. It also means
the last patch in this series to mark pidfs as non-exportable can be
dropped. Instead pidfs export operations will be marked as
non-exportable in the patch that they are added in.

Thanks!
Christian

---
Christian Brauner (4):
      exportfs: add flag to indicate local file handles
      kernfs: restrict to local file handles
      ovl: restrict to exportable file handles
      pidfs: restrict to local file handles

 fs/kernfs/mount.c        | 1 +
 fs/nfsd/export.c         | 8 +++++++-
 fs/overlayfs/util.c      | 7 ++++++-
 fs/pidfs.c               | 1 +
 include/linux/exportfs.h | 1 +
 5 files changed, 16 insertions(+), 2 deletions(-)
---
base-commit: 74e20c5946ab3f8ad959ea34f63f21e157d3ebae
change-id: 20241201-work-exportfs-cd49bee773c5

On Sun, Dec 01, 2024 at 02:12:24PM +0100, Christian Brauner wrote:
> Hey,
> 
> Some filesystems like kernfs and pidfs support file handles as a
> convenience to enable the use of name_to_handle_at(2) and
> open_by_handle_at(2) but don't want to and cannot be reliably exported.
> Add a flag that allows them to mark their export operations accordingly
> and make NFS check for its presence.
> 
> @Amir, I'll reorder the patches such that this series comes prior to the
> pidfs file handle series. Doing it that way will mean that there's never
> a state where pidfs supports file handles while also being exportable.
> It's probably not a big deal but it's definitely cleaner. It also means
> the last patch in this series to mark pidfs as non-exportable can be
> dropped. Instead pidfs export operations will be marked as
> non-exportable in the patch that they are added in.

Can you please invert the polarity?  Marking something as not supporting
is always awkward.  Clearly marking it as supporting something (and
writing down in detail what is required for that) is much better, even
it might cause a little more churn initially.

On Thu, Dec 5, 2024 at 1:38 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sun, Dec 01, 2024 at 02:12:24PM +0100, Christian Brauner wrote:
> > Hey,
> >
> > Some filesystems like kernfs and pidfs support file handles as a
> > convenience to enable the use of name_to_handle_at(2) and
> > open_by_handle_at(2) but don't want to and cannot be reliably exported.
> > Add a flag that allows them to mark their export operations accordingly
> > and make NFS check for its presence.
> >
> > @Amir, I'll reorder the patches such that this series comes prior to the
> > pidfs file handle series. Doing it that way will mean that there's never
> > a state where pidfs supports file handles while also being exportable.
> > It's probably not a big deal but it's definitely cleaner. It also means
> > the last patch in this series to mark pidfs as non-exportable can be
> > dropped. Instead pidfs export operations will be marked as
> > non-exportable in the patch that they are added in.
>
> Can you please invert the polarity?  Marking something as not supporting
> is always awkward.  Clearly marking it as supporting something (and
> writing down in detail what is required for that) is much better, even
> it might cause a little more churn initially.
>

Churn would be a bit annoying, but I guess it makes sense.
I agree with Christian that it should be done as cleanup to allow for
easier backport.

Please suggest a name for this opt-in flag.
EXPORT_OP_NFS_EXPORT???

Thanks,
Amir.

On Thu, Dec 05, 2024 at 12:57:28PM +0100, Amir Goldstein wrote:
> On Thu, Dec 5, 2024 at 1:38 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Sun, Dec 01, 2024 at 02:12:24PM +0100, Christian Brauner wrote:
> > > Hey,
> > >
> > > Some filesystems like kernfs and pidfs support file handles as a
> > > convenience to enable the use of name_to_handle_at(2) and
> > > open_by_handle_at(2) but don't want to and cannot be reliably exported.
> > > Add a flag that allows them to mark their export operations accordingly
> > > and make NFS check for its presence.
> > >
> > > @Amir, I'll reorder the patches such that this series comes prior to the
> > > pidfs file handle series. Doing it that way will mean that there's never
> > > a state where pidfs supports file handles while also being exportable.
> > > It's probably not a big deal but it's definitely cleaner. It also means
> > > the last patch in this series to mark pidfs as non-exportable can be
> > > dropped. Instead pidfs export operations will be marked as
> > > non-exportable in the patch that they are added in.
> >
> > Can you please invert the polarity?  Marking something as not supporting
> > is always awkward.  Clearly marking it as supporting something (and
> > writing down in detail what is required for that) is much better, even
> > it might cause a little more churn initially.
> >
> 
> Churn would be a bit annoying, but I guess it makes sense.
> I agree with Christian that it should be done as cleanup to allow for
> easier backport.
> 
> Please suggest a name for this opt-in flag.
> EXPORT_OP_NFS_EXPORT???

That's probably too specific to NFS--

AFAICT the goal here is to prevent exporting {pid,kern}fs file handles
to other nodes, correct?  Because we don't want to allow a process on
another computer to mess around with processes on the local computer?

How about:

/* file handles can be used by a process on another node */
#define EXPORT_OP_ALLOW_REMOTE_NODES	(...)

--D

> Thanks,
> Amir.
> 

On Fri, Dec 6, 2024 at 5:03 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Thu, Dec 05, 2024 at 12:57:28PM +0100, Amir Goldstein wrote:
> > On Thu, Dec 5, 2024 at 1:38 AM Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > On Sun, Dec 01, 2024 at 02:12:24PM +0100, Christian Brauner wrote:
> > > > Hey,
> > > >
> > > > Some filesystems like kernfs and pidfs support file handles as a
> > > > convenience to enable the use of name_to_handle_at(2) and
> > > > open_by_handle_at(2) but don't want to and cannot be reliably exported.
> > > > Add a flag that allows them to mark their export operations accordingly
> > > > and make NFS check for its presence.
> > > >
> > > > @Amir, I'll reorder the patches such that this series comes prior to the
> > > > pidfs file handle series. Doing it that way will mean that there's never
> > > > a state where pidfs supports file handles while also being exportable.
> > > > It's probably not a big deal but it's definitely cleaner. It also means
> > > > the last patch in this series to mark pidfs as non-exportable can be
> > > > dropped. Instead pidfs export operations will be marked as
> > > > non-exportable in the patch that they are added in.
> > >
> > > Can you please invert the polarity?  Marking something as not supporting
> > > is always awkward.  Clearly marking it as supporting something (and
> > > writing down in detail what is required for that) is much better, even
> > > it might cause a little more churn initially.
> > >
> >
> > Churn would be a bit annoying, but I guess it makes sense.
> > I agree with Christian that it should be done as cleanup to allow for
> > easier backport.
> >
> > Please suggest a name for this opt-in flag.
> > EXPORT_OP_NFS_EXPORT???
>
> That's probably too specific to NFS--
>
> AFAICT the goal here is to prevent exporting {pid,kern}fs file handles
> to other nodes, correct?  Because we don't want to allow a process on
> another computer to mess around with processes on the local computer?
>
> How about:
>
> /* file handles can be used by a process on another node */
> #define EXPORT_OP_ALLOW_REMOTE_NODES    (...)

This has a sound of security which is incorrect IMO.
The fact that we block nfsd export of cgroups does not prevent
any type of userland file server from exporting cgroup file handles.

I hate to be a pain, but IMO, the claim that inverted polarity is clearer
is not a consensus and there are plenty of counter examples.
I do not object to inverting the polarity if a flag name is found
that explains the property well, but IMO, this is not it.

Maybe opt-out of nfsd export is a little less safer than opt-in, but
1. opt-out is and will remain the rare exception for export_operations
2. at least the flag name EXPORT_OP_LOCAL_FILE_HANDLE
    is pretty clear IMO

Plus, as I wrote in another email, the fact that pidfs is SB_NOUSER,
so userspace is not allowed to mount it into the namespace and
userland file servers cannot export the filesystem itself.
That property itself (SB_NOUSER), is therefore a good enough indication
to deny nfsd export of this fs.
So really the immediate need for an explicit flag is only to stop exporting
kernfs/cgroupfs and I don't see this need spreading much further
(perhaps to nsfs) and therefore, the value of inverting to opt-in is
questionable IMO.

Thanks,
Amir.

On Sat, Dec 07, 2024 at 09:49:02AM +0100, Amir Goldstein wrote:
> > /* file handles can be used by a process on another node */
> > #define EXPORT_OP_ALLOW_REMOTE_NODES    (...)
> 
> This has a sound of security which is incorrect IMO.
> The fact that we block nfsd export of cgroups does not prevent
> any type of userland file server from exporting cgroup file handles.

So what is the purpose of the flag?  Asking for a coherent name and
description was the other bigger ask for me.

> Maybe opt-out of nfsd export is a little less safer than opt-in, but
> 1. opt-out is and will remain the rare exception for export_operations
> 2. at least the flag name EXPORT_OP_LOCAL_FILE_HANDLE
>     is pretty clear IMO

Even after this thread I have absolutely no idea what problem it tries
to solve.  Maybe that's not just the flag names fault, and not of opt-in
vs out, but both certainly don't help.

> Plus, as I wrote in another email, the fact that pidfs is SB_NOUSER,
> so userspace is not allowed to mount it into the namespace and
> userland file servers cannot export the filesystem itself.
> That property itself (SB_NOUSER), is therefore a good enough indication
> to deny nfsd export of this fs.

So check SB_NOUSER in nfsd and be done with it?

On Mon, Dec 9, 2024 at 8:49 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Sat, Dec 07, 2024 at 09:49:02AM +0100, Amir Goldstein wrote:
> > > /* file handles can be used by a process on another node */
> > > #define EXPORT_OP_ALLOW_REMOTE_NODES    (...)
> >
> > This has a sound of security which is incorrect IMO.
> > The fact that we block nfsd export of cgroups does not prevent
> > any type of userland file server from exporting cgroup file handles.
>
> So what is the purpose of the flag?  Asking for a coherent name and
> description was the other bigger ask for me.
>
> > Maybe opt-out of nfsd export is a little less safer than opt-in, but
> > 1. opt-out is and will remain the rare exception for export_operations
> > 2. at least the flag name EXPORT_OP_LOCAL_FILE_HANDLE
> >     is pretty clear IMO
>
> Even after this thread I have absolutely no idea what problem it tries
> to solve.  Maybe that's not just the flag names fault, and not of opt-in
> vs out, but both certainly don't help.
>
> > Plus, as I wrote in another email, the fact that pidfs is SB_NOUSER,
> > so userspace is not allowed to mount it into the namespace and
> > userland file servers cannot export the filesystem itself.
> > That property itself (SB_NOUSER), is therefore a good enough indication
> > to deny nfsd export of this fs.
>
> So check SB_NOUSER in nfsd and be done with it?
>

That will work for the new user (pidfs).

I think SB_KERNMOUNT qualifies as well, because it signifies
a mount that does not belong to any user's mount namespace.

For example, tmpfs (shmem) can be exported via nfs, but trying to
export an anonymous memfd should fail.

To be clear, exporting pidfs or internal shmem via an anonymous fd is
probably not possible with existing userspace tools, but with all the new
mount_fd and magic link apis, I can never be sure what can be made possible
to achieve when the user holds an anonymous fd.

The thinking behind adding the EXPORT_OP_LOCAL_FILE_HANDLE flag
was that when kernfs/cgroups was added exportfs support with commit
aa8188253474 ("kernfs: add exportfs operations"), there was no intention
to export cgroupfs over nfs, only local to uses, but that was never enforced,
so we thought it would be good to add this restriction and backport it to
stable kernels.

[CC patch authors]

I tried to look for some property of cgroupfs that will make it not eligible
for nfs such as the SB_KERNMOUNT and SB_NOUSER indicators, but
as far as I can see cgroups looks like any other non-blockdev filesystem.

Maybe we were wrong about the assumption that cgroupfs should be treated
specially and deny export cgroups over nfs??

Thanks,
Amir.

On Mon, Dec 09, 2024 at 09:58:58AM +0100, Amir Goldstein wrote:
> To be clear, exporting pidfs or internal shmem via an anonymous fd is
> probably not possible with existing userspace tools, but with all the new
> mount_fd and magic link apis, I can never be sure what can be made possible
> to achieve when the user holds an anonymous fd.
> 
> The thinking behind adding the EXPORT_OP_LOCAL_FILE_HANDLE flag
> was that when kernfs/cgroups was added exportfs support with commit
> aa8188253474 ("kernfs: add exportfs operations"), there was no intention
> to export cgroupfs over nfs, only local to uses, but that was never enforced,
> so we thought it would be good to add this restriction and backport it to
> stable kernels.

Can you please explain what the problem with exporting these file
systems over NFS is?  Yes, it's not going to be very useful.  But what
is actually problematic about it?  Any why is it not problematic with
a userland nfs server?  We really need to settle that argumet before
deciding a flag name or polarity.

On Mon, Dec 9, 2024 at 2:46 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Dec 09, 2024 at 09:58:58AM +0100, Amir Goldstein wrote:
> > To be clear, exporting pidfs or internal shmem via an anonymous fd is
> > probably not possible with existing userspace tools, but with all the new
> > mount_fd and magic link apis, I can never be sure what can be made possible
> > to achieve when the user holds an anonymous fd.
> >
> > The thinking behind adding the EXPORT_OP_LOCAL_FILE_HANDLE flag
> > was that when kernfs/cgroups was added exportfs support with commit
> > aa8188253474 ("kernfs: add exportfs operations"), there was no intention
> > to export cgroupfs over nfs, only local to uses, but that was never enforced,
> > so we thought it would be good to add this restriction and backport it to
> > stable kernels.
>
> Can you please explain what the problem with exporting these file
> systems over NFS is?  Yes, it's not going to be very useful.  But what
> is actually problematic about it?  Any why is it not problematic with
> a userland nfs server?  We really need to settle that argumet before
> deciding a flag name or polarity.
>

I agree that it is not the end of the world and users do have to explicitly
use fsid= argument to be able to export cgroupfs via nfsd.

The idea for this patch started from the claim that Jeff wrote that cgroups
is not allowed for nfsd export, but I couldn't find where it is not allowed.

I have no issue personally with leaving cgroupfs exportable via nfsd
and changing restricting only SB_NOUSER and SB_KERNMOUNT fs.

Jeff, Chuck, what is your opinion w.r.t exportability of cgroupfs via nfsd?

Thanks,
Amir.

On 12/9/24 11:30 AM, Amir Goldstein wrote:
> On Mon, Dec 9, 2024 at 2:46 PM Christoph Hellwig <hch@infradead.org> wrote:
>>
>> On Mon, Dec 09, 2024 at 09:58:58AM +0100, Amir Goldstein wrote:
>>> To be clear, exporting pidfs or internal shmem via an anonymous fd is
>>> probably not possible with existing userspace tools, but with all the new
>>> mount_fd and magic link apis, I can never be sure what can be made possible
>>> to achieve when the user holds an anonymous fd.
>>>
>>> The thinking behind adding the EXPORT_OP_LOCAL_FILE_HANDLE flag
>>> was that when kernfs/cgroups was added exportfs support with commit
>>> aa8188253474 ("kernfs: add exportfs operations"), there was no intention
>>> to export cgroupfs over nfs, only local to uses, but that was never enforced,
>>> so we thought it would be good to add this restriction and backport it to
>>> stable kernels.
>>
>> Can you please explain what the problem with exporting these file
>> systems over NFS is?  Yes, it's not going to be very useful.  But what
>> is actually problematic about it?  Any why is it not problematic with
>> a userland nfs server?  We really need to settle that argumet before
>> deciding a flag name or polarity.
>>
> 
> I agree that it is not the end of the world and users do have to explicitly
> use fsid= argument to be able to export cgroupfs via nfsd.
> 
> The idea for this patch started from the claim that Jeff wrote that cgroups
> is not allowed for nfsd export, but I couldn't find where it is not allowed.
> 
> I have no issue personally with leaving cgroupfs exportable via nfsd
> and changing restricting only SB_NOUSER and SB_KERNMOUNT fs.
> 
> Jeff, Chuck, what is your opinion w.r.t exportability of cgroupfs via nfsd?

We all seem to be hard-pressed to find a usage scenario where exporting
pseudo-filesystems via NFS is valuable. But maybe someone has done it
and has a good reason for it.

The issue is whether such export should be consistently and actively
prevented.

I'm not aware of any specific security issues with it.


-- 
Chuck Lever

On Mon, 2024-12-09 at 11:35 -0500, Chuck Lever wrote:
> On 12/9/24 11:30 AM, Amir Goldstein wrote:
> > On Mon, Dec 9, 2024 at 2:46 PM Christoph Hellwig <hch@infradead.org> wrote:
> > > 
> > > On Mon, Dec 09, 2024 at 09:58:58AM +0100, Amir Goldstein wrote:
> > > > To be clear, exporting pidfs or internal shmem via an anonymous fd is
> > > > probably not possible with existing userspace tools, but with all the new
> > > > mount_fd and magic link apis, I can never be sure what can be made possible
> > > > to achieve when the user holds an anonymous fd.
> > > > 
> > > > The thinking behind adding the EXPORT_OP_LOCAL_FILE_HANDLE flag
> > > > was that when kernfs/cgroups was added exportfs support with commit
> > > > aa8188253474 ("kernfs: add exportfs operations"), there was no intention
> > > > to export cgroupfs over nfs, only local to uses, but that was never enforced,
> > > > so we thought it would be good to add this restriction and backport it to
> > > > stable kernels.
> > > 
> > > Can you please explain what the problem with exporting these file
> > > systems over NFS is?  Yes, it's not going to be very useful.  But what
> > > is actually problematic about it?  Any why is it not problematic with
> > > a userland nfs server?  We really need to settle that argumet before
> > > deciding a flag name or polarity.
> > > 
> > 
> > I agree that it is not the end of the world and users do have to explicitly
> > use fsid= argument to be able to export cgroupfs via nfsd.
> > 
> > The idea for this patch started from the claim that Jeff wrote that cgroups
> > is not allowed for nfsd export, but I couldn't find where it is not allowed.
> > 

I think that must have been a wrong assumption on my part. I don't see
anything that specifically prevents that either. If cgroupfs is mounted
and you tell mountd to export it, I don't see what would prevent that.

To be clear, I don't see how you would trick bog-standard mountd into
exporting a filesystem that isn't mounted into its namespace, however.
Writing a replacement for mountd is always a possibilty.

> > I have no issue personally with leaving cgroupfs exportable via nfsd
> > and changing restricting only SB_NOUSER and SB_KERNMOUNT fs.
> > 
> > Jeff, Chuck, what is your opinion w.r.t exportability of cgroupfs via nfsd?
> 
> We all seem to be hard-pressed to find a usage scenario where exporting
> pseudo-filesystems via NFS is valuable. But maybe someone has done it
> and has a good reason for it.
> 
> The issue is whether such export should be consistently and actively
> prevented.
> 
> I'm not aware of any specific security issues with it.
> 
> 

I'm not either, but we are in new territory here. nfsd is a network
service, so it does present more of an attack surface vs. local access.

In general, you do have to take active steps to export a filesystem,
but if someone exports / with "crossmnt", everything mounted is
potentially accessible. That's obviously a dumb thing to do, but people
make mistakes, and it's possible that doing this could be part of a
wider exploit.

I tend to think it safest to make exporting via nfsd an opt-in thing on
a per-fs basis (along the lines of this patchset). If someone wants to
allow access to more "exotic" filesystems, let them argue their use-
case on the list first.
-- 
Jeff Layton <jlayton@kernel.org>

On 12/9/24 12:15 PM, Jeff Layton wrote:
> On Mon, 2024-12-09 at 11:35 -0500, Chuck Lever wrote:
>> On 12/9/24 11:30 AM, Amir Goldstein wrote:
>>> On Mon, Dec 9, 2024 at 2:46 PM Christoph Hellwig <hch@infradead.org> wrote:
>>>>
>>>> On Mon, Dec 09, 2024 at 09:58:58AM +0100, Amir Goldstein wrote:
>>>>> To be clear, exporting pidfs or internal shmem via an anonymous fd is
>>>>> probably not possible with existing userspace tools, but with all the new
>>>>> mount_fd and magic link apis, I can never be sure what can be made possible
>>>>> to achieve when the user holds an anonymous fd.
>>>>>
>>>>> The thinking behind adding the EXPORT_OP_LOCAL_FILE_HANDLE flag
>>>>> was that when kernfs/cgroups was added exportfs support with commit
>>>>> aa8188253474 ("kernfs: add exportfs operations"), there was no intention
>>>>> to export cgroupfs over nfs, only local to uses, but that was never enforced,
>>>>> so we thought it would be good to add this restriction and backport it to
>>>>> stable kernels.
>>>>
>>>> Can you please explain what the problem with exporting these file
>>>> systems over NFS is?  Yes, it's not going to be very useful.  But what
>>>> is actually problematic about it?  Any why is it not problematic with
>>>> a userland nfs server?  We really need to settle that argumet before
>>>> deciding a flag name or polarity.
>>>>
>>>
>>> I agree that it is not the end of the world and users do have to explicitly
>>> use fsid= argument to be able to export cgroupfs via nfsd.
>>>
>>> The idea for this patch started from the claim that Jeff wrote that cgroups
>>> is not allowed for nfsd export, but I couldn't find where it is not allowed.
>>>
> 
> I think that must have been a wrong assumption on my part. I don't see
> anything that specifically prevents that either. If cgroupfs is mounted
> and you tell mountd to export it, I don't see what would prevent that.
> 
> To be clear, I don't see how you would trick bog-standard mountd into
> exporting a filesystem that isn't mounted into its namespace, however.
> Writing a replacement for mountd is always a possibilty.
> 
>>> I have no issue personally with leaving cgroupfs exportable via nfsd
>>> and changing restricting only SB_NOUSER and SB_KERNMOUNT fs.
>>>
>>> Jeff, Chuck, what is your opinion w.r.t exportability of cgroupfs via nfsd?
>>
>> We all seem to be hard-pressed to find a usage scenario where exporting
>> pseudo-filesystems via NFS is valuable. But maybe someone has done it
>> and has a good reason for it.
>>
>> The issue is whether such export should be consistently and actively
>> prevented.
>>
>> I'm not aware of any specific security issues with it.
>>
>>
> 
> I'm not either, but we are in new territory here. nfsd is a network
> service, so it does present more of an attack surface vs. local access.
> 
> In general, you do have to take active steps to export a filesystem,
> but if someone exports / with "crossmnt", everything mounted is
> potentially accessible. That's obviously a dumb thing to do, but people
> make mistakes, and it's possible that doing this could be part of a
> wider exploit.
> 
> I tend to think it safest to make exporting via nfsd an opt-in thing on
> a per-fs basis (along the lines of this patchset). If someone wants to
> allow access to more "exotic" filesystems, let them argue their use-
> case on the list first.

If we were starting from scratch, 100% agree.

The current situation is that these file systems appear to be exportable
(and not only via NFS). The proposal is that this facility is to be
taken away. This can easily turn into a behavior regression for someone
if we're not careful.


-- 
Chuck Lever

On Mon, Dec 09, 2024 at 12:20:10PM -0500, Chuck Lever wrote:
> On 12/9/24 12:15 PM, Jeff Layton wrote:
> > On Mon, 2024-12-09 at 11:35 -0500, Chuck Lever wrote:
> > > On 12/9/24 11:30 AM, Amir Goldstein wrote:
> > > > On Mon, Dec 9, 2024 at 2:46 PM Christoph Hellwig <hch@infradead.org> wrote:
> > > > > 
> > > > > On Mon, Dec 09, 2024 at 09:58:58AM +0100, Amir Goldstein wrote:
> > > > > > To be clear, exporting pidfs or internal shmem via an anonymous fd is
> > > > > > probably not possible with existing userspace tools, but with all the new
> > > > > > mount_fd and magic link apis, I can never be sure what can be made possible
> > > > > > to achieve when the user holds an anonymous fd.
> > > > > > 
> > > > > > The thinking behind adding the EXPORT_OP_LOCAL_FILE_HANDLE flag
> > > > > > was that when kernfs/cgroups was added exportfs support with commit
> > > > > > aa8188253474 ("kernfs: add exportfs operations"), there was no intention
> > > > > > to export cgroupfs over nfs, only local to uses, but that was never enforced,
> > > > > > so we thought it would be good to add this restriction and backport it to
> > > > > > stable kernels.
> > > > > 
> > > > > Can you please explain what the problem with exporting these file
> > > > > systems over NFS is?  Yes, it's not going to be very useful.  But what
> > > > > is actually problematic about it?  Any why is it not problematic with
> > > > > a userland nfs server?  We really need to settle that argumet before
> > > > > deciding a flag name or polarity.
> > > > > 
> > > > 
> > > > I agree that it is not the end of the world and users do have to explicitly
> > > > use fsid= argument to be able to export cgroupfs via nfsd.
> > > > 
> > > > The idea for this patch started from the claim that Jeff wrote that cgroups
> > > > is not allowed for nfsd export, but I couldn't find where it is not allowed.
> > > > 
> > 
> > I think that must have been a wrong assumption on my part. I don't see
> > anything that specifically prevents that either. If cgroupfs is mounted
> > and you tell mountd to export it, I don't see what would prevent that.
> > 
> > To be clear, I don't see how you would trick bog-standard mountd into
> > exporting a filesystem that isn't mounted into its namespace, however.
> > Writing a replacement for mountd is always a possibilty.
> > 
> > > > I have no issue personally with leaving cgroupfs exportable via nfsd
> > > > and changing restricting only SB_NOUSER and SB_KERNMOUNT fs.
> > > > 
> > > > Jeff, Chuck, what is your opinion w.r.t exportability of cgroupfs via nfsd?
> > > 
> > > We all seem to be hard-pressed to find a usage scenario where exporting
> > > pseudo-filesystems via NFS is valuable. But maybe someone has done it
> > > and has a good reason for it.
> > > 
> > > The issue is whether such export should be consistently and actively
> > > prevented.
> > > 
> > > I'm not aware of any specific security issues with it.
> > > 
> > > 
> > 
> > I'm not either, but we are in new territory here. nfsd is a network
> > service, so it does present more of an attack surface vs. local access.
> > 
> > In general, you do have to take active steps to export a filesystem,
> > but if someone exports / with "crossmnt", everything mounted is
> > potentially accessible. That's obviously a dumb thing to do, but people
> > make mistakes, and it's possible that doing this could be part of a
> > wider exploit.
> > 
> > I tend to think it safest to make exporting via nfsd an opt-in thing on
> > a per-fs basis (along the lines of this patchset). If someone wants to
> > allow access to more "exotic" filesystems, let them argue their use-
> > case on the list first.
> 
> If we were starting from scratch, 100% agree.
> 
> The current situation is that these file systems appear to be exportable
> (and not only via NFS). The proposal is that this facility is to be
> taken away. This can easily turn into a behavior regression for someone
> if we're not careful.

So I'm happy to drop the exportfs preliminary we have now preventing
kernfs from being exported but then Christoph and you should figure out
what the security implications of allowing kernfs instances to be
exported areare because I'm not an NFS export expert.

Filesystems that fall under kernfs that are exportable by NFS as I
currently understand it are at least:

(1) sysfs
(2) cgroupfs

Has anyone ever actually tried to export the two and tested what
happens? Because I wouldn't be surprised if this ended in tears but
maybe I'm overly pessimistic.

Both (1) and (2) are rather special and don't have standard filesystem
semantics in a few places.

- cgroupfs isn't actually namespace aware. Whereas most filesystems like
  tmpfs and ramfs that are mountable inside unprivileged containers are
  multi-instance filesystems, aka allocate a new superblock per
  container cgroupfs is single-instance with a nasty implementation to
  virtualize the per-container view via cgroup namespaces. I wouldn't be
  surprised if that ends up being problematic.

- Cgroupfs has write-time permission checks as the process that is moved
  into a cgroup isn't known at open time. That has been exploitable
  before this was fixed.

- Even though it's legacy cgroup has a v1 and v2 mode where v1 is even
  more messed up than v2 including the release-agent logic which ends up
  issuing a usermode helper to call a binary when a cgroup is released.

- sysfs potentially exposes all kinds of extremly low-level information
  to a remote machine.

None of this gives me the warm and fuzzy. But that's just me.

Otherwise, I don't understand what it means that a userspace NFS server
can export kernfs instances. I don't know what that means and what the
contrast to in-kernel NFS server export is and whether that has the same
security implications. If so it's even scary that some random userspace
NFS server can just expose guts like kernfs.

But if both of you feel that this is safe to do and there aren't any
security issues lurking that have gone unnoticed simply because no one
has really ever exported sysfs or cgroupfs then by all means continue
allowing that. I'm rather skeptical.

On Tue, 2024-12-10 at 11:13 +0100, Christian Brauner wrote:
> On Mon, Dec 09, 2024 at 12:20:10PM -0500, Chuck Lever wrote:
> > On 12/9/24 12:15 PM, Jeff Layton wrote:
> > > On Mon, 2024-12-09 at 11:35 -0500, Chuck Lever wrote:
> > > > On 12/9/24 11:30 AM, Amir Goldstein wrote:
> > > > > On Mon, Dec 9, 2024 at 2:46 PM Christoph Hellwig <hch@infradead.org> wrote:
> > > > > > 
> > > > > > On Mon, Dec 09, 2024 at 09:58:58AM +0100, Amir Goldstein wrote:
> > > > > > > To be clear, exporting pidfs or internal shmem via an anonymous fd is
> > > > > > > probably not possible with existing userspace tools, but with all the new
> > > > > > > mount_fd and magic link apis, I can never be sure what can be made possible
> > > > > > > to achieve when the user holds an anonymous fd.
> > > > > > > 
> > > > > > > The thinking behind adding the EXPORT_OP_LOCAL_FILE_HANDLE flag
> > > > > > > was that when kernfs/cgroups was added exportfs support with commit
> > > > > > > aa8188253474 ("kernfs: add exportfs operations"), there was no intention
> > > > > > > to export cgroupfs over nfs, only local to uses, but that was never enforced,
> > > > > > > so we thought it would be good to add this restriction and backport it to
> > > > > > > stable kernels.
> > > > > > 
> > > > > > Can you please explain what the problem with exporting these file
> > > > > > systems over NFS is?  Yes, it's not going to be very useful.  But what
> > > > > > is actually problematic about it?  Any why is it not problematic with
> > > > > > a userland nfs server?  We really need to settle that argumet before
> > > > > > deciding a flag name or polarity.
> > > > > > 
> > > > > 
> > > > > I agree that it is not the end of the world and users do have to explicitly
> > > > > use fsid= argument to be able to export cgroupfs via nfsd.
> > > > > 
> > > > > The idea for this patch started from the claim that Jeff wrote that cgroups
> > > > > is not allowed for nfsd export, but I couldn't find where it is not allowed.
> > > > > 
> > > 
> > > I think that must have been a wrong assumption on my part. I don't see
> > > anything that specifically prevents that either. If cgroupfs is mounted
> > > and you tell mountd to export it, I don't see what would prevent that.
> > > 
> > > To be clear, I don't see how you would trick bog-standard mountd into
> > > exporting a filesystem that isn't mounted into its namespace, however.
> > > Writing a replacement for mountd is always a possibilty.
> > > 
> > > > > I have no issue personally with leaving cgroupfs exportable via nfsd
> > > > > and changing restricting only SB_NOUSER and SB_KERNMOUNT fs.
> > > > > 
> > > > > Jeff, Chuck, what is your opinion w.r.t exportability of cgroupfs via nfsd?
> > > > 
> > > > We all seem to be hard-pressed to find a usage scenario where exporting
> > > > pseudo-filesystems via NFS is valuable. But maybe someone has done it
> > > > and has a good reason for it.
> > > > 
> > > > The issue is whether such export should be consistently and actively
> > > > prevented.
> > > > 
> > > > I'm not aware of any specific security issues with it.
> > > > 
> > > > 
> > > 
> > > I'm not either, but we are in new territory here. nfsd is a network
> > > service, so it does present more of an attack surface vs. local access.
> > > 
> > > In general, you do have to take active steps to export a filesystem,
> > > but if someone exports / with "crossmnt", everything mounted is
> > > potentially accessible. That's obviously a dumb thing to do, but people
> > > make mistakes, and it's possible that doing this could be part of a
> > > wider exploit.
> > > 
> > > I tend to think it safest to make exporting via nfsd an opt-in thing on
> > > a per-fs basis (along the lines of this patchset). If someone wants to
> > > allow access to more "exotic" filesystems, let them argue their use-
> > > case on the list first.
> > 
> > If we were starting from scratch, 100% agree.
> > 
> > The current situation is that these file systems appear to be exportable
> > (and not only via NFS). The proposal is that this facility is to be
> > taken away. This can easily turn into a behavior regression for someone
> > if we're not careful.
> 
> So I'm happy to drop the exportfs preliminary we have now preventing
> kernfs from being exported but then Christoph and you should figure out
> what the security implications of allowing kernfs instances to be
> exported areare because I'm not an NFS export expert.
> 
> Filesystems that fall under kernfs that are exportable by NFS as I
> currently understand it are at least:
> 
> (1) sysfs
> (2) cgroupfs
> 
> Has anyone ever actually tried to export the two and tested what
> happens? Because I wouldn't be surprised if this ended in tears but
> maybe I'm overly pessimistic.
> 
> Both (1) and (2) are rather special and don't have standard filesystem
> semantics in a few places.
> 
> - cgroupfs isn't actually namespace aware. Whereas most filesystems like
>   tmpfs and ramfs that are mountable inside unprivileged containers are
>   multi-instance filesystems, aka allocate a new superblock per
>   container cgroupfs is single-instance with a nasty implementation to
>   virtualize the per-container view via cgroup namespaces. I wouldn't be
>   surprised if that ends up being problematic.
> 
> - Cgroupfs has write-time permission checks as the process that is moved
>   into a cgroup isn't known at open time. That has been exploitable
>   before this was fixed.
> 
> - Even though it's legacy cgroup has a v1 and v2 mode where v1 is even
>   more messed up than v2 including the release-agent logic which ends up
>   issuing a usermode helper to call a binary when a cgroup is released.
> 
> - sysfs potentially exposes all kinds of extremly low-level information
>   to a remote machine.
> 
> None of this gives me the warm and fuzzy. But that's just me.
> 
> Otherwise, I don't understand what it means that a userspace NFS server
> can export kernfs instances. I don't know what that means and what the
> contrast to in-kernel NFS server export is and whether that has the same
> security implications. If so it's even scary that some random userspace
> NFS server can just expose guts like kernfs.
> 

A userspace NFS server can export anything to which it has access. If
cgroupfs or sysfs is mounted and the server is running with appropriate
permissions then there is nothing that prevents it from making that
available. It's helpful if the filesystem can implement
name_to_handle_at() and open_by_handle_at(), but even that isn't
specifically required.

> But if both of you feel that this is safe to do and there aren't any
> security issues lurking that have gone unnoticed simply because no one
> has really ever exported sysfs or cgroupfs then by all means continue
> allowing that. I'm rather skeptical.

I'm not sure I agree that it's "safe", but in order to export kernfs or
pidfs you have to explicitly set it up to be exported. Christoph has a
good point that we don't have a specific scenario that we're trying to
prevent here.

My main thinking here is that:

1/ exporting these fstypes is not something we consider useful

2/ by forbidding this now, we prevent someone from complaining that
there is a regression later if we do find that it's problematic and
have to forbid it

Also, if we forbid this now, that might force someone who does want to
do this to articulate their use-case publicly.
-- 
Jeff Layton <jlayton@kernel.org>

On Tue, Dec 10, 2024 at 11:13:16AM +0100, Christian Brauner wrote:
> So I'm happy to drop the exportfs preliminary we have now preventing
> kernfs from being exported but then Christoph and you should figure out
> what the security implications of allowing kernfs instances to be
> exported areare because I'm not an NFS export expert.

I'm pretty sure you can do all kinds of really stupid things with it,
and very few if any useful ones.  But the litmus tests is if those are
things that only the kernel nfs server can do vs things that a userland
nfs (or other protocol) server could do the open by handle syscalls.
Because if they aren't specific to the kernel nfs server they are just
random policy for privileged actions.

On Tue, Dec 10, 2024 at 11:13:16AM +0100, Christian Brauner wrote:
> On Mon, Dec 09, 2024 at 12:20:10PM -0500, Chuck Lever wrote:
> > On 12/9/24 12:15 PM, Jeff Layton wrote:
> > > On Mon, 2024-12-09 at 11:35 -0500, Chuck Lever wrote:
> > > > On 12/9/24 11:30 AM, Amir Goldstein wrote:
> > > > > On Mon, Dec 9, 2024 at 2:46 PM Christoph Hellwig <hch@infradead.org> wrote:
> > > > > > 
> > > > > > On Mon, Dec 09, 2024 at 09:58:58AM +0100, Amir Goldstein wrote:
> > > > > > > To be clear, exporting pidfs or internal shmem via an anonymous fd is
> > > > > > > probably not possible with existing userspace tools, but with all the new
> > > > > > > mount_fd and magic link apis, I can never be sure what can be made possible
> > > > > > > to achieve when the user holds an anonymous fd.
> > > > > > > 
> > > > > > > The thinking behind adding the EXPORT_OP_LOCAL_FILE_HANDLE flag
> > > > > > > was that when kernfs/cgroups was added exportfs support with commit
> > > > > > > aa8188253474 ("kernfs: add exportfs operations"), there was no intention
> > > > > > > to export cgroupfs over nfs, only local to uses, but that was never enforced,
> > > > > > > so we thought it would be good to add this restriction and backport it to
> > > > > > > stable kernels.
> > > > > > 
> > > > > > Can you please explain what the problem with exporting these file
> > > > > > systems over NFS is?  Yes, it's not going to be very useful.  But what
> > > > > > is actually problematic about it?  Any why is it not problematic with
> > > > > > a userland nfs server?  We really need to settle that argumet before
> > > > > > deciding a flag name or polarity.
> > > > > > 
> > > > > 
> > > > > I agree that it is not the end of the world and users do have to explicitly
> > > > > use fsid= argument to be able to export cgroupfs via nfsd.
> > > > > 
> > > > > The idea for this patch started from the claim that Jeff wrote that cgroups
> > > > > is not allowed for nfsd export, but I couldn't find where it is not allowed.
> > > > > 
> > > 
> > > I think that must have been a wrong assumption on my part. I don't see
> > > anything that specifically prevents that either. If cgroupfs is mounted
> > > and you tell mountd to export it, I don't see what would prevent that.
> > > 
> > > To be clear, I don't see how you would trick bog-standard mountd into
> > > exporting a filesystem that isn't mounted into its namespace, however.
> > > Writing a replacement for mountd is always a possibilty.
> > > 
> > > > > I have no issue personally with leaving cgroupfs exportable via nfsd
> > > > > and changing restricting only SB_NOUSER and SB_KERNMOUNT fs.
> > > > > 
> > > > > Jeff, Chuck, what is your opinion w.r.t exportability of cgroupfs via nfsd?
> > > > 
> > > > We all seem to be hard-pressed to find a usage scenario where exporting
> > > > pseudo-filesystems via NFS is valuable. But maybe someone has done it
> > > > and has a good reason for it.
> > > > 
> > > > The issue is whether such export should be consistently and actively
> > > > prevented.
> > > > 
> > > > I'm not aware of any specific security issues with it.
> > > > 
> > > > 
> > > 
> > > I'm not either, but we are in new territory here. nfsd is a network
> > > service, so it does present more of an attack surface vs. local access.
> > > 
> > > In general, you do have to take active steps to export a filesystem,
> > > but if someone exports / with "crossmnt", everything mounted is
> > > potentially accessible. That's obviously a dumb thing to do, but people
> > > make mistakes, and it's possible that doing this could be part of a
> > > wider exploit.
> > > 
> > > I tend to think it safest to make exporting via nfsd an opt-in thing on
> > > a per-fs basis (along the lines of this patchset). If someone wants to
> > > allow access to more "exotic" filesystems, let them argue their use-
> > > case on the list first.
> > 
> > If we were starting from scratch, 100% agree.
> > 
> > The current situation is that these file systems appear to be exportable
> > (and not only via NFS). The proposal is that this facility is to be
> > taken away. This can easily turn into a behavior regression for someone
> > if we're not careful.
> 
> So I'm happy to drop the exportfs preliminary we have now preventing
> kernfs from being exported but then Christoph and you should figure out
> what the security implications of allowing kernfs instances to be
> exported areare because I'm not an NFS export expert.
> 
> Filesystems that fall under kernfs that are exportable by NFS as I
> currently understand it are at least:
> 
> (1) sysfs
> (2) cgroupfs
> 
> Has anyone ever actually tried to export the two and tested what
> happens? Because I wouldn't be surprised if this ended in tears but
> maybe I'm overly pessimistic.
> 
> Both (1) and (2) are rather special and don't have standard filesystem
> semantics in a few places.
> 
> - cgroupfs isn't actually namespace aware. Whereas most filesystems like
>   tmpfs and ramfs that are mountable inside unprivileged containers are
>   multi-instance filesystems, aka allocate a new superblock per
>   container cgroupfs is single-instance with a nasty implementation to
>   virtualize the per-container view via cgroup namespaces. I wouldn't be
>   surprised if that ends up being problematic.
> 
> - Cgroupfs has write-time permission checks as the process that is moved
>   into a cgroup isn't known at open time. That has been exploitable
>   before this was fixed.
> 
> - Even though it's legacy cgroup has a v1 and v2 mode where v1 is even
>   more messed up than v2 including the release-agent logic which ends up
>   issuing a usermode helper to call a binary when a cgroup is released.
> 
> - sysfs potentially exposes all kinds of extremly low-level information
>   to a remote machine.
> 
> None of this gives me the warm and fuzzy. But that's just me.
> 
> Otherwise, I don't understand what it means that a userspace NFS server
> can export kernfs instances. I don't know what that means and what the
> contrast to in-kernel NFS server export is and whether that has the same
> security implications. If so it's even scary that some random userspace
> NFS server can just expose guts like kernfs.
> 
> But if both of you feel that this is safe to do and there aren't any
> security issues lurking that have gone unnoticed simply because no one
> has really ever exported sysfs or cgroupfs then by all means continue
> allowing that. I'm rather skeptical.

Amir pointed that sysfs can't be exported as it opts out of kernfs
export_operations being set.

On Mon, Dec 09, 2024 at 09:58:58AM +0100, Amir Goldstein wrote:
> On Mon, Dec 9, 2024 at 8:49 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Sat, Dec 07, 2024 at 09:49:02AM +0100, Amir Goldstein wrote:
> > > > /* file handles can be used by a process on another node */
> > > > #define EXPORT_OP_ALLOW_REMOTE_NODES    (...)
> > >
> > > This has a sound of security which is incorrect IMO.
> > > The fact that we block nfsd export of cgroups does not prevent
> > > any type of userland file server from exporting cgroup file handles.
> >
> > So what is the purpose of the flag?  Asking for a coherent name and
> > description was the other bigger ask for me.
> >
> > > Maybe opt-out of nfsd export is a little less safer than opt-in, but
> > > 1. opt-out is and will remain the rare exception for export_operations
> > > 2. at least the flag name EXPORT_OP_LOCAL_FILE_HANDLE
> > >     is pretty clear IMO
> >
> > Even after this thread I have absolutely no idea what problem it tries
> > to solve.  Maybe that's not just the flag names fault, and not of opt-in
> > vs out, but both certainly don't help.
> >
> > > Plus, as I wrote in another email, the fact that pidfs is SB_NOUSER,
> > > so userspace is not allowed to mount it into the namespace and
> > > userland file servers cannot export the filesystem itself.
> > > That property itself (SB_NOUSER), is therefore a good enough indication
> > > to deny nfsd export of this fs.
> >
> > So check SB_NOUSER in nfsd and be done with it?
> >
> 
> That will work for the new user (pidfs).
> 
> I think SB_KERNMOUNT qualifies as well, because it signifies
> a mount that does not belong to any user's mount namespace.
> 
> For example, tmpfs (shmem) can be exported via nfs, but trying to
> export an anonymous memfd should fail.
> 
> To be clear, exporting pidfs or internal shmem via an anonymous fd is
> probably not possible with existing userspace tools, but with all the new
> mount_fd and magic link apis, I can never be sure what can be made possible
> to achieve when the user holds an anonymous fd.
> 
> The thinking behind adding the EXPORT_OP_LOCAL_FILE_HANDLE flag
> was that when kernfs/cgroups was added exportfs support with commit
> aa8188253474 ("kernfs: add exportfs operations"), there was no intention
> to export cgroupfs over nfs, only local to uses, but that was never enforced,
> so we thought it would be good to add this restriction and backport it to
> stable kernels.
> 
> [CC patch authors]
> 
> I tried to look for some property of cgroupfs that will make it not eligible
> for nfs such as the SB_KERNMOUNT and SB_NOUSER indicators, but
> as far as I can see cgroups looks like any other non-blockdev filesystem.
> 
> Maybe we were wrong about the assumption that cgroupfs should be treated
> specially and deny export cgroups over nfs??

Please don't export any of the "fake" kernel filesystems (configfs,
cgroups, sysfs, debugfs, proc, etc) over nfs please.  That way lies
madness and makes no sense.

thanks,

greg k-h

On Mon, Dec 09, 2024 at 10:16:36AM +0100, Greg KH wrote:
> > Maybe we were wrong about the assumption that cgroupfs should be treated
> > specially and deny export cgroups over nfs??
> 
> Please don't export any of the "fake" kernel filesystems (configfs,
> cgroups, sysfs, debugfs, proc, etc) over nfs please.  That way lies
> madness and makes no sense.

Umm, yes: it sounds like a pretty useless idea.  But you can do that
today with a userland nfs server, so why explicitly forbid it for
the kernel nfs server.  In either case you absolutely have to want it,
you're not going to accidentally NFS export a file system.

I'm still trying to understand what problem we're trying to solve here.

On Mon, Dec 9, 2024 at 10:16 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Dec 09, 2024 at 09:58:58AM +0100, Amir Goldstein wrote:
> > On Mon, Dec 9, 2024 at 8:49 AM Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > On Sat, Dec 07, 2024 at 09:49:02AM +0100, Amir Goldstein wrote:
> > > > > /* file handles can be used by a process on another node */
> > > > > #define EXPORT_OP_ALLOW_REMOTE_NODES    (...)
> > > >
> > > > This has a sound of security which is incorrect IMO.
> > > > The fact that we block nfsd export of cgroups does not prevent
> > > > any type of userland file server from exporting cgroup file handles.
> > >
> > > So what is the purpose of the flag?  Asking for a coherent name and
> > > description was the other bigger ask for me.
> > >
> > > > Maybe opt-out of nfsd export is a little less safer than opt-in, but
> > > > 1. opt-out is and will remain the rare exception for export_operations
> > > > 2. at least the flag name EXPORT_OP_LOCAL_FILE_HANDLE
> > > >     is pretty clear IMO
> > >
> > > Even after this thread I have absolutely no idea what problem it tries
> > > to solve.  Maybe that's not just the flag names fault, and not of opt-in
> > > vs out, but both certainly don't help.
> > >
> > > > Plus, as I wrote in another email, the fact that pidfs is SB_NOUSER,
> > > > so userspace is not allowed to mount it into the namespace and
> > > > userland file servers cannot export the filesystem itself.
> > > > That property itself (SB_NOUSER), is therefore a good enough indication
> > > > to deny nfsd export of this fs.
> > >
> > > So check SB_NOUSER in nfsd and be done with it?
> > >
> >
> > That will work for the new user (pidfs).
> >
> > I think SB_KERNMOUNT qualifies as well, because it signifies
> > a mount that does not belong to any user's mount namespace.
> >
> > For example, tmpfs (shmem) can be exported via nfs, but trying to
> > export an anonymous memfd should fail.
> >
> > To be clear, exporting pidfs or internal shmem via an anonymous fd is
> > probably not possible with existing userspace tools, but with all the new
> > mount_fd and magic link apis, I can never be sure what can be made possible
> > to achieve when the user holds an anonymous fd.
> >
> > The thinking behind adding the EXPORT_OP_LOCAL_FILE_HANDLE flag
> > was that when kernfs/cgroups was added exportfs support with commit
> > aa8188253474 ("kernfs: add exportfs operations"), there was no intention
> > to export cgroupfs over nfs, only local to uses, but that was never enforced,
> > so we thought it would be good to add this restriction and backport it to
> > stable kernels.
> >
> > [CC patch authors]
> >
> > I tried to look for some property of cgroupfs that will make it not eligible
> > for nfs such as the SB_KERNMOUNT and SB_NOUSER indicators, but
> > as far as I can see cgroups looks like any other non-blockdev filesystem.
> >
> > Maybe we were wrong about the assumption that cgroupfs should be treated
> > specially and deny export cgroups over nfs??
>
> Please don't export any of the "fake" kernel filesystems (configfs,
> cgroups, sysfs, debugfs, proc, etc) over nfs please.  That way lies
> madness and makes no sense.

Agreed. The problem is that when looking in vfs for a clear definition
of "fake" kernel filesystems, I cannot find an objective criteria, especially
for those "fake" fs that you listed.

The "pseudo" filesystems (that call init_pseudo()) which are clearly
marked with SB_NOUSER (except for nsfs)

The "internal" filesystems that use kern_mount() internal mount ns
are clearly marked with SB_KERNMOUNT.

One commonality that I found among most fs that have a known sysfs
mount point is that they use  get_tree_single() because they are "singleton"
filesystems. However, a singleton filesystem may not be fake (efivarfs,
openpromfs), so I am not sure this is a good indication for no nfs export
and in any case, this it not true for procfs, sysfs, cgroups (kernfs).

We are left with the fact that there is no clear criteria with the list of
"fake" filesystems that you mentioned and among that list, only cgroupfs
has implemented file handles, so I see two options:

1. Explicitly opt-out of nfs export for cgroups as the proposed patch set does
2. Clearly mark all "fake" filesystems, for whatever "fake" means with
    SB_<WHATEVER> and then let nfsd query the "fake" fs flag

#1 is pretty straightforward.
#2 means this discussion will go on for some time and that I may eventually
need to submit a "What is a fake filesystem?" topic to LSFMM ;)

Thanks,
Amir.

On Wed, Dec 04, 2024 at 04:38:28PM -0800, Christoph Hellwig wrote:
> On Sun, Dec 01, 2024 at 02:12:24PM +0100, Christian Brauner wrote:
> > Hey,
> > 
> > Some filesystems like kernfs and pidfs support file handles as a
> > convenience to enable the use of name_to_handle_at(2) and
> > open_by_handle_at(2) but don't want to and cannot be reliably exported.
> > Add a flag that allows them to mark their export operations accordingly
> > and make NFS check for its presence.
> > 
> > @Amir, I'll reorder the patches such that this series comes prior to the
> > pidfs file handle series. Doing it that way will mean that there's never
> > a state where pidfs supports file handles while also being exportable.
> > It's probably not a big deal but it's definitely cleaner. It also means
> > the last patch in this series to mark pidfs as non-exportable can be
> > dropped. Instead pidfs export operations will be marked as
> > non-exportable in the patch that they are added in.
> 
> Can you please invert the polarity?  Marking something as not supporting
> is always awkward.  Clearly marking it as supporting something (and
> writing down in detail what is required for that) is much better, even
> it might cause a little more churn initially.

Fine by me but let's do that as a cleanup on top, please. Especially
because we need to backport this to stable.

On Sun, Dec 1, 2024 at 2:12 PM Christian Brauner <brauner@kernel.org> wrote:
>
> Hey,
>
> Some filesystems like kernfs and pidfs support file handles as a
> convenience to enable the use of name_to_handle_at(2) and
> open_by_handle_at(2) but don't want to and cannot be reliably exported.
> Add a flag that allows them to mark their export operations accordingly
> and make NFS check for its presence.
>
> @Amir, I'll reorder the patches such that this series comes prior to the
> pidfs file handle series. Doing it that way will mean that there's never
> a state where pidfs supports file handles while also being exportable.
> It's probably not a big deal but it's definitely cleaner. It also means
> the last patch in this series to mark pidfs as non-exportable can be
> dropped. Instead pidfs export operations will be marked as
> non-exportable in the patch that they are added in.

Yeh, looks good!

Apart from missing update to exporting.rst

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

Thanks,
Amir.

On Sun, 2024-12-01 at 14:12 +0100, Christian Brauner wrote:
> Hey,
> 
> Some filesystems like kernfs and pidfs support file handles as a
> convenience to enable the use of name_to_handle_at(2) and
> open_by_handle_at(2) but don't want to and cannot be reliably exported.
> Add a flag that allows them to mark their export operations accordingly
> and make NFS check for its presence.
> 
> @Amir, I'll reorder the patches such that this series comes prior to the
> pidfs file handle series. Doing it that way will mean that there's never
> a state where pidfs supports file handles while also being exportable.
> It's probably not a big deal but it's definitely cleaner. It also means
> the last patch in this series to mark pidfs as non-exportable can be
> dropped. Instead pidfs export operations will be marked as
> non-exportable in the patch that they are added in.
> 
> Thanks!
> Christian
> 
> ---
> Christian Brauner (4):
>       exportfs: add flag to indicate local file handles
>       kernfs: restrict to local file handles
>       ovl: restrict to exportable file handles
>       pidfs: restrict to local file handles
> 
>  fs/kernfs/mount.c        | 1 +
>  fs/nfsd/export.c         | 8 +++++++-
>  fs/overlayfs/util.c      | 7 ++++++-
>  fs/pidfs.c               | 1 +
>  include/linux/exportfs.h | 1 +
>  5 files changed, 16 insertions(+), 2 deletions(-)
> ---
> base-commit: 74e20c5946ab3f8ad959ea34f63f21e157d3ebae
> change-id: 20241201-work-exportfs-cd49bee773c5
> 

I've been following the pidfs filehandle discussion and this is exactly
what I was thinking we needed: a way to explicitly label certain
fstypes as unexportable via nfsd.

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

> On Dec 1, 2024, at 8:28 AM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Sun, 2024-12-01 at 14:12 +0100, Christian Brauner wrote:
>> Hey,
>> 
>> Some filesystems like kernfs and pidfs support file handles as a
>> convenience to enable the use of name_to_handle_at(2) and
>> open_by_handle_at(2) but don't want to and cannot be reliably exported.
>> Add a flag that allows them to mark their export operations accordingly
>> and make NFS check for its presence.
>> 
>> @Amir, I'll reorder the patches such that this series comes prior to the
>> pidfs file handle series. Doing it that way will mean that there's never
>> a state where pidfs supports file handles while also being exportable.
>> It's probably not a big deal but it's definitely cleaner. It also means
>> the last patch in this series to mark pidfs as non-exportable can be
>> dropped. Instead pidfs export operations will be marked as
>> non-exportable in the patch that they are added in.
>> 
>> Thanks!
>> Christian
>> 
>> ---
>> Christian Brauner (4):
>>      exportfs: add flag to indicate local file handles
>>      kernfs: restrict to local file handles
>>      ovl: restrict to exportable file handles
>>      pidfs: restrict to local file handles
>> 
>> fs/kernfs/mount.c        | 1 +
>> fs/nfsd/export.c         | 8 +++++++-
>> fs/overlayfs/util.c      | 7 ++++++-
>> fs/pidfs.c               | 1 +
>> include/linux/exportfs.h | 1 +
>> 5 files changed, 16 insertions(+), 2 deletions(-)
>> ---
>> base-commit: 74e20c5946ab3f8ad959ea34f63f21e157d3ebae
>> change-id: 20241201-work-exportfs-cd49bee773c5
>> 
> 
> I've been following the pidfs filehandle discussion and this is exactly
> what I was thinking we needed: a way to explicitly label certain
> fstypes as unexportable via nfsd.
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> 

Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>

Though, I wonder if a similar but separate prohibition
mechanism might be necessary for other in-kernel network
file system server implementations (eg, ksmbd).


--
Chuck Lever

> Though, I wonder if a similar but separate prohibition
> mechanism might be necessary for other in-kernel network
> file system server implementations (eg, ksmbd).

Oh hm, interesting question.
I have no idea how ksmbd or 9p "exports" work. I really hope they don't
allow exporting arbitrary pseudo-fses.

On Tue, 2024-12-03 at 10:08 +0100, Christian Brauner wrote:
> > Though, I wonder if a similar but separate prohibition
> > mechanism might be necessary for other in-kernel network
> > file system server implementations (eg, ksmbd).
> 
> Oh hm, interesting question.
> I have no idea how ksmbd or 9p "exports" work. I really hope they don't
> allow exporting arbitrary pseudo-fses.

SMB is path-based so there's no worry about filehandles there. It looks
like ksmbd keeps a set of ksmbd_share_config objects, that are
configured by userland. If someone deliberately shares stuff under
/proc, then I guess they get to keep all of the pieces. ;)

9P does use filehandles, but there is no in-kernel server, so far.
Wedson had one in development at one point [1], but I haven't heard
anything about it in a while.

[1]: https://kangrejos.com/Async%20Rust%20and%209p%20server.pdf
-- 
Jeff Layton <jlayton@kernel.org>