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 <[email protected]>
---
RFC: https://lore.kernel.org/all/[email protected]/
---
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
On Wed, 24 Jan 2024 at 12:30, Jingbo Xu <[email protected]> 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 <[email protected]>
> ---
> RFC: https://lore.kernel.org/all/[email protected]/
> ---
> 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
On 1/24/24 8:16 PM, Miklos Szeredi wrote:
> On Wed, 24 Jan 2024 at 12:30, Jingbo Xu <[email protected]> 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 <[email protected]>
>> ---
>> RFC: https://lore.kernel.org/all/[email protected]/
>> ---
>> 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
On Wed, 24 Jan 2024 at 13:50, Jingbo Xu <[email protected]> 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
On 1/24/24 9:04 PM, Miklos Szeredi wrote:
> On Wed, 24 Jan 2024 at 13:50, Jingbo Xu <[email protected]> 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
On Wed, Jan 24, 2024 at 2:17 PM Miklos Szeredi <[email protected]> wrote:
>
> On Wed, 24 Jan 2024 at 12:30, Jingbo Xu <[email protected]> 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 <[email protected]>
> > ---
> > RFC: https://lore.kernel.org/all/[email protected]/
> > ---
> > 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.
On Wed, 24 Jan 2024 at 15:11, Amir Goldstein <[email protected]> wrote:
>
> On Wed, Jan 24, 2024 at 2:17 PM Miklos Szeredi <[email protected]> wrote:
> >
> > On Wed, 24 Jan 2024 at 12:30, Jingbo Xu <[email protected]> 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 <[email protected]>
> > > ---
> > > RFC: https://lore.kernel.org/all/[email protected]/
> > > ---
> > > 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