Received-SPF: pass (google.com: domain of linux-kernel+bounces-39704-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240126072120.71867-1-jefflexu@linux.alibaba.com>
In-Reply-To: <20240126072120.71867-1-jefflexu@linux.alibaba.com>
From: Amir Goldstein <amir73il@gmail.com>
Date: Fri, 26 Jan 2024 09:33:47 +0200
Message-ID: <CAOQ4uxh2C_mh2Zgr=s8jXh0pk=UGWJxLijTWBGEL5cZfyFUzbQ@mail.gmail.com>
Subject: Re: [PATCH v2] fuse: add support for explicit export disabling
To: Jingbo Xu <jefflexu@linux.alibaba.com>
Cc: miklos@szeredi.hu, linux-fsdevel@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 26, 2024 at 9:21=E2=80=AFAM Jingbo Xu <jefflexu@linux.alibaba.c=
om> 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=3Dnone" 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>

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

> ---
> v2:
> - rename to "fuse_export_fid_operations"
> - bump FUSE_KERNEL_MINOR_VERSION
>
> v1: https://lore.kernel.org/linux-fsdevel/20240124113042.44300-1-jefflexu=
@linux.alibaba.com/
> RFC: https://lore.kernel.org/all/20240123093701.94166-1-jefflexu@linux.al=
ibaba.com/
> ---
>  fs/fuse/inode.c           | 11 ++++++++++-
>  include/uapi/linux/fuse.h |  7 ++++++-
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 2a6d44f91729..eee200308482 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1110,6 +1110,11 @@ static struct dentry *fuse_get_parent(struct dentr=
y *child)
>         return parent;
>  }
>
> +/* only for fid encoding; no support for file handle */
> +static const struct export_operations fuse_export_fid_operations =3D {
> +       .encode_fh      =3D fuse_encode_fh,
> +};
> +
>  static const struct export_operations fuse_export_operations =3D {
>         .fh_to_dentry   =3D fuse_fh_to_dentry,
>         .fh_to_parent   =3D fuse_fh_to_parent,
> @@ -1284,6 +1289,8 @@ static void process_init_reply(struct fuse_mount *f=
m, struct fuse_args *args,
>                                 fc->create_supp_group =3D 1;
>                         if (flags & FUSE_DIRECT_IO_ALLOW_MMAP)
>                                 fc->direct_io_allow_mmap =3D 1;
> +                       if (flags & FUSE_NO_EXPORT_SUPPORT)
> +                               fm->sb->s_export_op =3D &fuse_export_fid_=
operations;
>                 } else {
>                         ra_pages =3D fc->max_read / PAGE_SIZE;
>                         fc->no_lock =3D 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_E=
XT |
>                 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 |=3D FUSE_MAP_ALIGNMENT;
> @@ -1527,6 +1535,7 @@ static int fuse_fill_super_submount(struct super_bl=
ock *sb,
>         sb->s_bdi =3D bdi_get(parent_sb->s_bdi);
>
>         sb->s_xattr =3D parent_sb->s_xattr;
> +       sb->s_export_op =3D parent_sb->s_export_op;
>         sb->s_time_gran =3D parent_sb->s_time_gran;
>         sb->s_blocksize =3D parent_sb->s_blocksize;
>         sb->s_blocksize_bits =3D parent_sb->s_blocksize_bits;
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index e7418d15fe39..38d9f285a599 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -211,6 +211,9 @@
>   *  7.39
>   *  - add FUSE_DIRECT_IO_ALLOW_MMAP
>   *  - add FUSE_STATX and related structures
> + *
> + *  7.40
> + *  - add FUSE_NO_EXPORT_SUPPORT
>   */
>
>  #ifndef _LINUX_FUSE_H
> @@ -246,7 +249,7 @@
>  #define FUSE_KERNEL_VERSION 7
>
>  /** Minor version number of this interface */
> -#define FUSE_KERNEL_MINOR_VERSION 39
> +#define FUSE_KERNEL_MINOR_VERSION 40
>
>  /** The node ID of the root inode */
>  #define FUSE_ROOT_ID 1
> @@ -410,6 +413,7 @@ struct fuse_file_lock {
>   *                     symlink and mknod (single group that matches pare=
nt)
>   * 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 +453,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
>