[PATCH] fuse: add support for explicit export disabling

[PATCH] fuse: add support for explicit export disabling

[Jingbo Xu]

open_by_handle_at(2) can fail with -ESTALE with a valid handle returned
by a previous name_to_handle_at(2) for evicted fuse inodes, which is
especially common when entry_valid_timeout is 0, e.g. when the fuse
daemon is in "cache=none" mode.

The time sequence is like:

	name_to_handle_at(2)	# succeed
	evict fuse inode
	open_by_handle_at(2)	# fail

The root cause is that, with 0 entry_valid_timeout, the dput() called in
name_to_handle_at(2) will trigger iput -> evict(), which will send
FUSE_FORGET to the daemon.  The following open_by_handle_at(2) will send
a new FUSE_LOOKUP request upon inode cache miss since the previous inode
eviction.  Then the fuse daemon may fail the FUSE_LOOKUP request with
-ENOENT as the cached metadata of the requested inode has already been
cleaned up during the previous FUSE_FORGET.  The returned -ENOENT is
treated as -ESTALE when open_by_handle_at(2) returns.

This confuses the application somehow, as open_by_handle_at(2) fails
when the previous name_to_handle_at(2) succeeds.  The returned errno is
also confusing as the requested file is not deleted and already there.
It is reasonable to fail name_to_handle_at(2) early in this case, after
which the application can fallback to open(2) to access files.

Since this issue typically appears when entry_valid_timeout is 0 which
is configured by the fuse daemon, the fuse daemon is the right person to
explicitly disable the export when required.

Also considering FUSE_EXPORT_SUPPORT actually indicates the support for
lookups of "." and "..", and there are existing fuse daemons supporting
export without FUSE_EXPORT_SUPPORT set, for compatibility, we add a new
INIT flag for such purpose.

Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
RFC: https://lore.kernel.org/all/20240123093701.94166-1-jefflexu@linux.alibaba.com/
---
 fs/fuse/inode.c           | 11 ++++++++++-
 include/uapi/linux/fuse.h |  2 ++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 2a6d44f91729..851940c0e930 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1110,6 +1110,11 @@ static struct dentry *fuse_get_parent(struct dentry *child)
 	return parent;
 }
 
+/* only for fid encoding; no support for file handle */
+static const struct export_operations fuse_fid_operations = {
+	.encode_fh	= fuse_encode_fh,
+};
+
 static const struct export_operations fuse_export_operations = {
 	.fh_to_dentry	= fuse_fh_to_dentry,
 	.fh_to_parent	= fuse_fh_to_parent,
@@ -1284,6 +1289,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 				fc->create_supp_group = 1;
 			if (flags & FUSE_DIRECT_IO_ALLOW_MMAP)
 				fc->direct_io_allow_mmap = 1;
+			if (flags & FUSE_NO_EXPORT_SUPPORT)
+				fm->sb->s_export_op = &fuse_fid_operations;
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1330,7 +1337,8 @@ void fuse_send_init(struct fuse_mount *fm)
 		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
 		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_INIT_EXT |
 		FUSE_SECURITY_CTX | FUSE_CREATE_SUPP_GROUP |
-		FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_ALLOW_MMAP;
+		FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_ALLOW_MMAP |
+		FUSE_NO_EXPORT_SUPPORT;
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		flags |= FUSE_MAP_ALIGNMENT;
@@ -1527,6 +1535,7 @@ static int fuse_fill_super_submount(struct super_block *sb,
 	sb->s_bdi = bdi_get(parent_sb->s_bdi);
 
 	sb->s_xattr = parent_sb->s_xattr;
+	sb->s_export_op = parent_sb->s_export_op;
 	sb->s_time_gran = parent_sb->s_time_gran;
 	sb->s_blocksize = parent_sb->s_blocksize;
 	sb->s_blocksize_bits = parent_sb->s_blocksize_bits;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index e7418d15fe39..8274c0cd5d61 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -410,6 +410,7 @@ struct fuse_file_lock {
  *			symlink and mknod (single group that matches parent)
  * FUSE_HAS_EXPIRE_ONLY: kernel supports expiry-only entry invalidation
  * FUSE_DIRECT_IO_ALLOW_MMAP: allow shared mmap in FOPEN_DIRECT_IO mode.
+ * FUSE_NO_EXPORT_SUPPORT: explicitly disable export support
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -449,6 +450,7 @@ struct fuse_file_lock {
 #define FUSE_CREATE_SUPP_GROUP	(1ULL << 34)
 #define FUSE_HAS_EXPIRE_ONLY	(1ULL << 35)
 #define FUSE_DIRECT_IO_ALLOW_MMAP (1ULL << 36)
+#define FUSE_NO_EXPORT_SUPPORT	(1ULL << 37)
 
 /* Obsolete alias for FUSE_DIRECT_IO_ALLOW_MMAP */
 #define FUSE_DIRECT_IO_RELAX	FUSE_DIRECT_IO_ALLOW_MMAP
-- 
2.19.1.6.gb485710b

Re: [PATCH] fuse: add support for explicit export disabling

[Miklos Szeredi]

On Wed, 24 Jan 2024 at 12:30, Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>
> open_by_handle_at(2) can fail with -ESTALE with a valid handle returned
> by a previous name_to_handle_at(2) for evicted fuse inodes, which is
> especially common when entry_valid_timeout is 0, e.g. when the fuse
> daemon is in "cache=none" mode.
>
> The time sequence is like:
>
>         name_to_handle_at(2)    # succeed
>         evict fuse inode
>         open_by_handle_at(2)    # fail
>
> The root cause is that, with 0 entry_valid_timeout, the dput() called in
> name_to_handle_at(2) will trigger iput -> evict(), which will send
> FUSE_FORGET to the daemon.  The following open_by_handle_at(2) will send
> a new FUSE_LOOKUP request upon inode cache miss since the previous inode
> eviction.  Then the fuse daemon may fail the FUSE_LOOKUP request with
> -ENOENT as the cached metadata of the requested inode has already been
> cleaned up during the previous FUSE_FORGET.  The returned -ENOENT is
> treated as -ESTALE when open_by_handle_at(2) returns.
>
> This confuses the application somehow, as open_by_handle_at(2) fails
> when the previous name_to_handle_at(2) succeeds.  The returned errno is
> also confusing as the requested file is not deleted and already there.
> It is reasonable to fail name_to_handle_at(2) early in this case, after
> which the application can fallback to open(2) to access files.
>
> Since this issue typically appears when entry_valid_timeout is 0 which
> is configured by the fuse daemon, the fuse daemon is the right person to
> explicitly disable the export when required.
>
> Also considering FUSE_EXPORT_SUPPORT actually indicates the support for
> lookups of "." and "..", and there are existing fuse daemons supporting
> export without FUSE_EXPORT_SUPPORT set, for compatibility, we add a new
> INIT flag for such purpose.

This looks good overall.

>
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> ---
> RFC: https://lore.kernel.org/all/20240123093701.94166-1-jefflexu@linux.alibaba.com/
> ---
>  fs/fuse/inode.c           | 11 ++++++++++-
>  include/uapi/linux/fuse.h |  2 ++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 2a6d44f91729..851940c0e930 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1110,6 +1110,11 @@ static struct dentry *fuse_get_parent(struct dentry *child)
>         return parent;
>  }
>
> +/* only for fid encoding; no support for file handle */
> +static const struct export_operations fuse_fid_operations = {

Nit: I'd call this fuse_no_export_operations (or something else that
emphasizes the fact that this is only for encoding and not for full
export support).

Thanks,
Miklos

Re: [PATCH] fuse: add support for explicit export disabling

[Jingbo Xu]

On 1/24/24 8:16 PM, Miklos Szeredi wrote:
> On Wed, 24 Jan 2024 at 12:30, Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>
>> open_by_handle_at(2) can fail with -ESTALE with a valid handle returned
>> by a previous name_to_handle_at(2) for evicted fuse inodes, which is
>> especially common when entry_valid_timeout is 0, e.g. when the fuse
>> daemon is in "cache=none" mode.
>>
>> The time sequence is like:
>>
>>         name_to_handle_at(2)    # succeed
>>         evict fuse inode
>>         open_by_handle_at(2)    # fail
>>
>> The root cause is that, with 0 entry_valid_timeout, the dput() called in
>> name_to_handle_at(2) will trigger iput -> evict(), which will send
>> FUSE_FORGET to the daemon.  The following open_by_handle_at(2) will send
>> a new FUSE_LOOKUP request upon inode cache miss since the previous inode
>> eviction.  Then the fuse daemon may fail the FUSE_LOOKUP request with
>> -ENOENT as the cached metadata of the requested inode has already been
>> cleaned up during the previous FUSE_FORGET.  The returned -ENOENT is
>> treated as -ESTALE when open_by_handle_at(2) returns.
>>
>> This confuses the application somehow, as open_by_handle_at(2) fails
>> when the previous name_to_handle_at(2) succeeds.  The returned errno is
>> also confusing as the requested file is not deleted and already there.
>> It is reasonable to fail name_to_handle_at(2) early in this case, after
>> which the application can fallback to open(2) to access files.
>>
>> Since this issue typically appears when entry_valid_timeout is 0 which
>> is configured by the fuse daemon, the fuse daemon is the right person to
>> explicitly disable the export when required.
>>
>> Also considering FUSE_EXPORT_SUPPORT actually indicates the support for
>> lookups of "." and "..", and there are existing fuse daemons supporting
>> export without FUSE_EXPORT_SUPPORT set, for compatibility, we add a new
>> INIT flag for such purpose.
> 
> This looks good overall.
> 
>>
>> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>> ---
>> RFC: https://lore.kernel.org/all/20240123093701.94166-1-jefflexu@linux.alibaba.com/
>> ---
>>  fs/fuse/inode.c           | 11 ++++++++++-
>>  include/uapi/linux/fuse.h |  2 ++
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>> index 2a6d44f91729..851940c0e930 100644
>> --- a/fs/fuse/inode.c
>> +++ b/fs/fuse/inode.c
>> @@ -1110,6 +1110,11 @@ static struct dentry *fuse_get_parent(struct dentry *child)
>>         return parent;
>>  }
>>
>> +/* only for fid encoding; no support for file handle */
>> +static const struct export_operations fuse_fid_operations = {
> 
> Nit: I'd call this fuse_no_export_operations (or something else that
> emphasizes the fact that this is only for encoding and not for full
> export support).

OK I will rename it to fuse_no_export_operations.

By the way do I need to bump and update the minor version of FUSE protocol?


-- 
Thanks,
Jingbo

Re: [PATCH] fuse: add support for explicit export disabling

[Miklos Szeredi]

On Wed, 24 Jan 2024 at 13:50, Jingbo Xu <jefflexu@linux.alibaba.com> wrote:

> OK I will rename it to fuse_no_export_operations.
>
> By the way do I need to bump and update the minor version of FUSE protocol?

It's not strictly necessary since the feature is negotiated with the
FUSE_NO_EXPORT_SUPPORT flag.

Despite that we do usually bump the minor version once per kernel
release if there were any changes to the API.

Thanks,
Miklos

Re: [PATCH] fuse: add support for explicit export disabling

[Jingbo Xu]

On 1/24/24 9:04 PM, Miklos Szeredi wrote:
> On Wed, 24 Jan 2024 at 13:50, Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
> 
>> OK I will rename it to fuse_no_export_operations.
>>
>> By the way do I need to bump and update the minor version of FUSE protocol?
> 
> It's not strictly necessary since the feature is negotiated with the
> FUSE_NO_EXPORT_SUPPORT flag.
> 
> Despite that we do usually bump the minor version once per kernel
> release if there were any changes to the API.


Got it.  Many thanks.

-- 
Thanks,
Jingbo

Re: [PATCH] fuse: add support for explicit export disabling

[Amir Goldstein]

On Wed, Jan 24, 2024 at 2:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 24 Jan 2024 at 12:30, Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
> >
> > open_by_handle_at(2) can fail with -ESTALE with a valid handle returned
> > by a previous name_to_handle_at(2) for evicted fuse inodes, which is
> > especially common when entry_valid_timeout is 0, e.g. when the fuse
> > daemon is in "cache=none" mode.
> >
> > The time sequence is like:
> >
> >         name_to_handle_at(2)    # succeed
> >         evict fuse inode
> >         open_by_handle_at(2)    # fail
> >
> > The root cause is that, with 0 entry_valid_timeout, the dput() called in
> > name_to_handle_at(2) will trigger iput -> evict(), which will send
> > FUSE_FORGET to the daemon.  The following open_by_handle_at(2) will send
> > a new FUSE_LOOKUP request upon inode cache miss since the previous inode
> > eviction.  Then the fuse daemon may fail the FUSE_LOOKUP request with
> > -ENOENT as the cached metadata of the requested inode has already been
> > cleaned up during the previous FUSE_FORGET.  The returned -ENOENT is
> > treated as -ESTALE when open_by_handle_at(2) returns.
> >
> > This confuses the application somehow, as open_by_handle_at(2) fails
> > when the previous name_to_handle_at(2) succeeds.  The returned errno is
> > also confusing as the requested file is not deleted and already there.
> > It is reasonable to fail name_to_handle_at(2) early in this case, after
> > which the application can fallback to open(2) to access files.
> >
> > Since this issue typically appears when entry_valid_timeout is 0 which
> > is configured by the fuse daemon, the fuse daemon is the right person to
> > explicitly disable the export when required.
> >
> > Also considering FUSE_EXPORT_SUPPORT actually indicates the support for
> > lookups of "." and "..", and there are existing fuse daemons supporting
> > export without FUSE_EXPORT_SUPPORT set, for compatibility, we add a new
> > INIT flag for such purpose.
>
> This looks good overall.
>
> >
> > Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> > ---
> > RFC: https://lore.kernel.org/all/20240123093701.94166-1-jefflexu@linux.alibaba.com/
> > ---
> >  fs/fuse/inode.c           | 11 ++++++++++-
> >  include/uapi/linux/fuse.h |  2 ++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 2a6d44f91729..851940c0e930 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1110,6 +1110,11 @@ static struct dentry *fuse_get_parent(struct dentry *child)
> >         return parent;
> >  }
> >
> > +/* only for fid encoding; no support for file handle */
> > +static const struct export_operations fuse_fid_operations = {
>
> Nit: I'd call this fuse_no_export_operations (or something else that
> emphasizes the fact that this is only for encoding and not for full
> export support).

Not that I really care what the name is, but overlayfs already has
ovl_export_fid_operations and the name aspires from AT_HANDLE_FID,
which is already documented in man pages.

How about fuse_export_fid_operations?

Thanks,
Amir.

Re: [PATCH] fuse: add support for explicit export disabling

[Miklos Szeredi]

On Wed, 24 Jan 2024 at 15:11, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Jan 24, 2024 at 2:17 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Wed, 24 Jan 2024 at 12:30, Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
> > >
> > > open_by_handle_at(2) can fail with -ESTALE with a valid handle returned
> > > by a previous name_to_handle_at(2) for evicted fuse inodes, which is
> > > especially common when entry_valid_timeout is 0, e.g. when the fuse
> > > daemon is in "cache=none" mode.
> > >
> > > The time sequence is like:
> > >
> > >         name_to_handle_at(2)    # succeed
> > >         evict fuse inode
> > >         open_by_handle_at(2)    # fail
> > >
> > > The root cause is that, with 0 entry_valid_timeout, the dput() called in
> > > name_to_handle_at(2) will trigger iput -> evict(), which will send
> > > FUSE_FORGET to the daemon.  The following open_by_handle_at(2) will send
> > > a new FUSE_LOOKUP request upon inode cache miss since the previous inode
> > > eviction.  Then the fuse daemon may fail the FUSE_LOOKUP request with
> > > -ENOENT as the cached metadata of the requested inode has already been
> > > cleaned up during the previous FUSE_FORGET.  The returned -ENOENT is
> > > treated as -ESTALE when open_by_handle_at(2) returns.
> > >
> > > This confuses the application somehow, as open_by_handle_at(2) fails
> > > when the previous name_to_handle_at(2) succeeds.  The returned errno is
> > > also confusing as the requested file is not deleted and already there.
> > > It is reasonable to fail name_to_handle_at(2) early in this case, after
> > > which the application can fallback to open(2) to access files.
> > >
> > > Since this issue typically appears when entry_valid_timeout is 0 which
> > > is configured by the fuse daemon, the fuse daemon is the right person to
> > > explicitly disable the export when required.
> > >
> > > Also considering FUSE_EXPORT_SUPPORT actually indicates the support for
> > > lookups of "." and "..", and there are existing fuse daemons supporting
> > > export without FUSE_EXPORT_SUPPORT set, for compatibility, we add a new
> > > INIT flag for such purpose.
> >
> > This looks good overall.
> >
> > >
> > > Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> > > ---
> > > RFC: https://lore.kernel.org/all/20240123093701.94166-1-jefflexu@linux.alibaba.com/
> > > ---
> > >  fs/fuse/inode.c           | 11 ++++++++++-
> > >  include/uapi/linux/fuse.h |  2 ++
> > >  2 files changed, 12 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > index 2a6d44f91729..851940c0e930 100644
> > > --- a/fs/fuse/inode.c
> > > +++ b/fs/fuse/inode.c
> > > @@ -1110,6 +1110,11 @@ static struct dentry *fuse_get_parent(struct dentry *child)
> > >         return parent;
> > >  }
> > >
> > > +/* only for fid encoding; no support for file handle */
> > > +static const struct export_operations fuse_fid_operations = {
> >
> > Nit: I'd call this fuse_no_export_operations (or something else that
> > emphasizes the fact that this is only for encoding and not for full
> > export support).
>
> Not that I really care what the name is, but overlayfs already has
> ovl_export_fid_operations and the name aspires from AT_HANDLE_FID,
> which is already documented in man pages.
>
> How about fuse_export_fid_operations?

Okay, let's be consistent with overlayfs naming.

Thanks,
Miklos