Request for clarification about FILESYSTEM_MAX_STACK_DEPTH

Request for clarification about FILESYSTEM_MAX_STACK_DEPTH

[Allison Karlitskaya]

hi,

Here's another "would be nice to have this on the record" sort of
thing that I haven't been able to find any other public statements
about.

FILESYSTEM_MAX_STACK_DEPTH is defined in a non-public header as 2.  As
far as I can tell, there's no way to query the limit out of a running
kernel, and there's no indication if this value might ever be
increased.  Hopefully it won't be decreased.

I'm trying to write a userspace binding layer for supporting
passthrough fds in fuse and it's hard to validate user input for the
stacking depth parameter.  libfuse hardcodes some constant values (0,
1) that the user might choose, but in an adjacent comment makes
references to the "current" kernel, suggesting that it might change at
some point.  It's also sort of difficult to determine if a value is
valid by probing: choosing an invalid value simply disables
passthrough fd support, which you won't find out about until you
actually go and try to create a passthrough fd, at which point it
fails with EPERM (but which can also happen for many other reasons).

So, I guess:
 - will 2 ever change?
 - if not, can we get it in a public header?
 - if so, can we add some sort of API to get the current value?

Thanks in advance!

lis

Re: Request for clarification about FILESYSTEM_MAX_STACK_DEPTH

[Miklos Szeredi]

On Mon, 5 May 2025 at 23:02, Allison Karlitskaya <lis@redhat.com> wrote:
>
> hi,
>
> Here's another "would be nice to have this on the record" sort of
> thing that I haven't been able to find any other public statements
> about.
>
> FILESYSTEM_MAX_STACK_DEPTH is defined in a non-public header as 2.  As
> far as I can tell, there's no way to query the limit out of a running
> kernel, and there's no indication if this value might ever be
> increased.  Hopefully it won't be decreased.
>
> I'm trying to write a userspace binding layer for supporting
> passthrough fds in fuse and it's hard to validate user input for the
> stacking depth parameter.  libfuse hardcodes some constant values (0,
> 1) that the user might choose, but in an adjacent comment makes
> references to the "current" kernel, suggesting that it might change at
> some point.  It's also sort of difficult to determine if a value is
> valid by probing: choosing an invalid value simply disables
> passthrough fd support, which you won't find out about until you
> actually go and try to create a passthrough fd, at which point it
> fails with EPERM (but which can also happen for many other reasons).
>
> So, I guess:
>  - will 2 ever change?

It will never be smaller than 2, that's guaranteed.  I think there's a
good chance of increasing this in the future if there's use case for
it.

>  - if so, can we add some sort of API to get the current value?

Yes.

Thanks,
Miklos

[PATCH] fuse: add max_stack_depth to fuse_init_in

[Allison Karlitskaya]

FILESYSTEM_MAX_STACK_DEPTH is defined privately inside of the kernel,
but you need to know its value to properly implement fd passthrough on a
FUSE filesystem.  So far most users have been assuming its current value
of 2, but there's nothing that says that it won't change.

Use one of the unused fields in fuse_init_in to add a max_stack_depth
uint32_t (matching the max_stack_depth uint32_t in fuse_init_out). If
CONFIG_FUSE_PASSTHROUGH is configured then this is set to the maximum
value that the kernel will accept for the corresponding field in
fuse_init_out (ie: FILESYSTEM_MAX_STACK_DEPTH).

Let's not treat this as an ABI change: this struct is zero-initialized
and the maximum max_stack_depth is non-zero (and always will be) so
userspace can easily find out for itself if the value is present in the
struct or not.

Signed-off-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>
---
 fs/fuse/inode.c           | 4 +++-
 include/uapi/linux/fuse.h | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index fd48e8d37f2e..46fd37eec9ae 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1497,8 +1497,10 @@ void fuse_send_init(struct fuse_mount *fm)
 #endif
 	if (fm->fc->auto_submounts)
 		flags |= FUSE_SUBMOUNTS;
-	if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
+	if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH)) {
 		flags |= FUSE_PASSTHROUGH;
+		ia->in.max_stack_depth = FILESYSTEM_MAX_STACK_DEPTH;
+	}
 
 	/*
 	 * This is just an information flag for fuse server. No need to check
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 5ec43ecbceb7..eb5d77d50176 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -895,7 +895,8 @@ struct fuse_init_in {
 	uint32_t	max_readahead;
 	uint32_t	flags;
 	uint32_t	flags2;
-	uint32_t	unused[11];
+	uint32_t	max_stack_depth;
+	uint32_t	unused[10];
 };
 
 #define FUSE_COMPAT_INIT_OUT_SIZE 8
-- 
2.49.0

Re: [PATCH] fuse: add max_stack_depth to fuse_init_in

[Bernd Schubert]

On 5/14/25 14:14, Allison Karlitskaya wrote:
> FILESYSTEM_MAX_STACK_DEPTH is defined privately inside of the kernel,
> but you need to know its value to properly implement fd passthrough on a
> FUSE filesystem.  So far most users have been assuming its current value
> of 2, but there's nothing that says that it won't change.
> 
> Use one of the unused fields in fuse_init_in to add a max_stack_depth
> uint32_t (matching the max_stack_depth uint32_t in fuse_init_out). If
> CONFIG_FUSE_PASSTHROUGH is configured then this is set to the maximum
> value that the kernel will accept for the corresponding field in
> fuse_init_out (ie: FILESYSTEM_MAX_STACK_DEPTH).
> 
> Let's not treat this as an ABI change: this struct is zero-initialized
> and the maximum max_stack_depth is non-zero (and always will be) so
> userspace can easily find out for itself if the value is present in the
> struct or not.
> 
> Signed-off-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>
> ---
>   fs/fuse/inode.c           | 4 +++-
>   include/uapi/linux/fuse.h | 3 ++-
>   2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index fd48e8d37f2e..46fd37eec9ae 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1497,8 +1497,10 @@ void fuse_send_init(struct fuse_mount *fm)
>   #endif
>   	if (fm->fc->auto_submounts)
>   		flags |= FUSE_SUBMOUNTS;
> -	if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
> +	if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH)) {
>   		flags |= FUSE_PASSTHROUGH;
> +		ia->in.max_stack_depth = FILESYSTEM_MAX_STACK_DEPTH;
> +	}
>   
>   	/*
>   	 * This is just an information flag for fuse server. No need to check
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 5ec43ecbceb7..eb5d77d50176 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -895,7 +895,8 @@ struct fuse_init_in {
>   	uint32_t	max_readahead;
>   	uint32_t	flags;
>   	uint32_t	flags2;
> -	uint32_t	unused[11];
> +	uint32_t	max_stack_depth;

Objections to make this a uint8_t? In fuse_init_out it just had slipped 
through in review. (And I wonder a bit if we should post change it in 
fuse_init_out.


Thanks,
Bernd

Re: [PATCH] fuse: add max_stack_depth to fuse_init_in

[Miklos Szeredi]

On Wed, 14 May 2025 at 14:14, Allison Karlitskaya
<allison.karlitskaya@redhat.com> wrote:
> Use one of the unused fields in fuse_init_in to add a max_stack_depth
> uint32_t (matching the max_stack_depth uint32_t in fuse_init_out).

This is not a fuse-only thing.

What about making it a read-only sysctl attribute? E.g.
/proc/sys/fs/max-stack-depth.

Thanks,
Miklos

Re: [PATCH] fuse: add max_stack_depth to fuse_init_in

[Christian Brauner]

On Thu, May 15, 2025 at 10:42:30AM +0200, Miklos Szeredi wrote:
> On Wed, 14 May 2025 at 14:14, Allison Karlitskaya
> <allison.karlitskaya@redhat.com> wrote:
> > Use one of the unused fields in fuse_init_in to add a max_stack_depth
> > uint32_t (matching the max_stack_depth uint32_t in fuse_init_out).
> 
> This is not a fuse-only thing.
> 
> What about making it a read-only sysctl attribute? E.g.
> /proc/sys/fs/max-stack-depth.

Before making this a kernel wide sysctl attribute we should have actual
users that need more than two right now. IIUC, then making this an
attribute in FUSE will happen at some point anyway. For example, if
userspace wants to have a FUSE specific stacking limit that's different
from the global limit.

Re: [PATCH] fuse: add max_stack_depth to fuse_init_in

[Miklos Szeredi]

On Thu, 15 May 2025 at 12:10, Christian Brauner <brauner@kernel.org> wrote:

> Before making this a kernel wide sysctl attribute we should have actual
> users that need more than two right now. IIUC, then making this an
> attribute in FUSE will happen at some point anyway. For example, if
> userspace wants to have a FUSE specific stacking limit that's different
> from the global limit.

Okay, let's add it to fuse_init_in as uint8_t.

Thanks,
Miklos

Re: [PATCH] fuse: add max_stack_depth to fuse_init_in

[Allison Karlitskaya]

hi,

Sorry for being so late to reply to this: it's been busy.

On Fri, 16 May 2025 at 11:07, Miklos Szeredi <miklos@szeredi.hu> wrote:
> Okay, let's add it to fuse_init_in as uint8_t.

Is this to help save a few bytes?  I'm not sure it's worth it, for a
few reasons:
 - there are 11 reserved fields here, which is still quite a lot of room
 - even if we reduce this to a single byte, we need to add 3 extra
bytes of padding, which is a bit awkward.  We also only get to use
this if we have something else that fits into a u8 or u16, otherwise
it's wasted
 - we might imagine some sort of a beautiful future where the kernel
figures out a way to increase this restriction considerably (ie: >
256).  I'm not sure how that would look, but it seems foolish to not
consider it.

I'm happy to redo the patch if you're sure this is right (since I want
to update the commit message a bit anyway), but how should I call the
3 extra bytes in that case?  unused2?  reserved?

Thanks

lis

Re: [PATCH] fuse: add max_stack_depth to fuse_init_in

[Amir Goldstein]

On Mon, May 26, 2025 at 10:51 AM Allison Karlitskaya <lis@redhat.com> wrote:
>
> hi,
>
> Sorry for being so late to reply to this: it's been busy.
>
> On Fri, 16 May 2025 at 11:07, Miklos Szeredi <miklos@szeredi.hu> wrote:
> > Okay, let's add it to fuse_init_in as uint8_t.
>
> Is this to help save a few bytes?  I'm not sure it's worth it, for a
> few reasons:
>  - there are 11 reserved fields here, which is still quite a lot of room
>  - even if we reduce this to a single byte, we need to add 3 extra
> bytes of padding, which is a bit awkward.  We also only get to use
> this if we have something else that fits into a u8 or u16, otherwise
> it's wasted
>  - we might imagine some sort of a beautiful future where the kernel
> figures out a way to increase this restriction considerably (ie: >
> 256).  I'm not sure how that would look, but it seems foolish to not
> consider it.
>
> I'm happy to redo the patch if you're sure this is right (since I want
> to update the commit message a bit anyway), but how should I call the
> 3 extra bytes in that case?  unused2?  reserved?
>

Allison,

Sorry for joining so late.

I cannot help feeling that all this is really pointless.

The reality is that I don't imagine the kernel's FILESYSTEM_MAX_STACK_DEPTH
constant to grow any time soon and that for the foreseen future
the only valid values for arg->max_stack_depth are 1 or 2.

I can't remember why we did not use 0.
I must have inherited this from the Android patches or something.
As it is, with or without knowing FILESYSTEM_MAX_STACK_DEPTH
this argument is quite hard to document for userspace.

for this reason I left an explicit documentation for the only two
possible values in libfuse:

        /**
         * When FUSE_CAP_PASSTHROUGH is enabled, this is the maximum allowed
         * stacking depth of the backing files.  In current kernel, the maximum
         * allowed stack depth if FILESYSTEM_MAX_STACK_DEPTH (2), which includes
         * the FUSE passthrough layer, so the maximum stacking depth for backing
         * files is 1.
         *
         * The default is FUSE_BACKING_STACKED_UNDER (0), meaning that the
         * backing files cannot be on a stacked filesystem, but another stacked
         * filesystem can be stacked over this FUSE passthrough filesystem.
         *
         * Set this to FUSE_BACKING_STACKED_OVER (1) if backing files may be on
         * a stacked filesystem, such as overlayfs or another FUSE passthrough.
         * In this configuration, another stacked filesystem cannot be stacked
         * over this FUSE passthrough filesystem.
         */
#define FUSE_BACKING_STACKED_UNDER      (0)
#define FUSE_BACKING_STACKED_OVER       (1)

If the problem is that this is not defined in uapi/fuse.h we can define:

#define FUSE_STACKED_UNDER (1)
#define FUSE_STAKCED_OVER (2)

With the comment from libfuse.
If we ever need to pass values greater than 2 we can introduce a new
init flag for that.

What do you think?
Did I misunderstand the problem that you are trying to solve?

Thanks,
Amir.

Re: [PATCH] fuse: add max_stack_depth to fuse_init_in

[Miklos Szeredi]

On Mon, 26 May 2025 at 12:12, Amir Goldstein <amir73il@gmail.com> wrote:

> I cannot help feeling that all this is really pointless.
>
> The reality is that I don't imagine the kernel's FILESYSTEM_MAX_STACK_DEPTH
> constant to grow any time soon and that for the foreseen future

I think the reason the max was defined as 2 when we implemented this
in overlayfs is that there was no use case for more since lower layers
can be stacked within overlayfs and that leaves stacking of upper
layer, which there was some use case for.

With the introduction of fuse passthrough, I can imagine that there
will be a push to raise this.  Which needs careful review of kernel
stack usage, but I suspect that we are not yet close to the limit.

> the only valid values for arg->max_stack_depth are 1 or 2.
>
> I can't remember why we did not use 0.

Kernel uses s_stack_depth of zero for no stacking, so that's what went
into the interface.  But we also added FUSE_PASSTHROUGH init flag,
which is redundant, since a stack depth of zero also implies no
passthrough.

> If the problem is that this is not defined in uapi/fuse.h we can define:
>
> #define FUSE_STACKED_UNDER (1)
> #define FUSE_STAKCED_OVER (2)

I'm not fond if hard coding these constants as they only make sense
with a max stack depth of 2.

Thanks,
Miklos