Re: [PATCH v2] fuse: return -ECONNABORTED on /dev/fuse read after abort

[Miklos Szeredi <miklos@szeredi.hu>]

On Fri, Nov 3, 2017 at 4:04 PM, Szymon Lukasz <noh4hss@gmail.com> wrote:
> Return -ECONNABORTED when userspace tries to read from /dev/fuse after
> the fuse connection was aborted via sysfs.
> The patch increases FUSE_KERNEL_MINOR_VERSION. However -ECONNABORTED is
> always returned, regardless of the minor version sent by userspace in
> FUSE_INIT.

I worry about that.

I'd rather have this enabled with a capability flag negotiated in INIT.

>
> Info why this patch might be useful:
> https://github.com/libfuse/libfuse/issues/122
>
> Signed-off-by: Szymon Lukasz <noh4hss@gmail.com>
> ---
>  fs/fuse/control.c         |  2 +-
>  fs/fuse/dev.c             | 13 ++++++++-----
>  fs/fuse/fuse_i.h          |  8 +++++++-
>  fs/fuse/inode.c           |  4 ++--
>  include/uapi/linux/fuse.h |  5 ++++-
>  5 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/fs/fuse/control.c b/fs/fuse/control.c
> index b9ea99c5b5b3..eeda05a7c882 100644
> --- a/fs/fuse/control.c
> +++ b/fs/fuse/control.c
> @@ -35,7 +35,7 @@ static ssize_t fuse_conn_abort_write(struct file *file, const char __user *buf,
>  {
>         struct fuse_conn *fc = fuse_ctl_file_conn_get(file);
>         if (fc) {
> -               fuse_abort_conn(fc);
> +               fuse_abort_conn(fc, 1);
>                 fuse_conn_put(fc);
>         }
>         return count;
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 13c65dd2d37d..b51da52b92b9 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -1234,9 +1234,10 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
>         if (err)
>                 goto err_unlock;
>
> -       err = -ENODEV;
> -       if (!fiq->connected)
> +       if (!fiq->connected) {
> +               err = fiq->aborted ? -ECONNABORTED : -ENODEV;
>                 goto err_unlock;
> +       }
>
>         if (!list_empty(&fiq->interrupts)) {
>                 req = list_entry(fiq->interrupts.next, struct fuse_req,
> @@ -1287,7 +1288,7 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
>         spin_lock(&fpq->lock);
>         clear_bit(FR_LOCKED, &req->flags);
>         if (!fpq->connected) {
> -               err = -ENODEV;
> +               err = fpq->aborted ? -ECONNABORTED : -ENODEV;
>                 goto out_end;
>         }
>         if (err) {
> @@ -2076,7 +2077,7 @@ static void end_polls(struct fuse_conn *fc)
>   * is OK, the request will in that case be removed from the list before we touch
>   * it.
>   */
> -void fuse_abort_conn(struct fuse_conn *fc)
> +void fuse_abort_conn(struct fuse_conn *fc, int is_abort)
>  {
>         struct fuse_iqueue *fiq = &fc->iq;
>
> @@ -2095,6 +2096,7 @@ void fuse_abort_conn(struct fuse_conn *fc)
>
>                         spin_lock(&fpq->lock);
>                         fpq->connected = 0;
> +                       fpq->aborted = is_abort;
>                         list_for_each_entry_safe(req, next, &fpq->io, list) {
>                                 req->out.h.error = -ECONNABORTED;
>                                 spin_lock(&req->waitq.lock);
> @@ -2113,6 +2115,7 @@ void fuse_abort_conn(struct fuse_conn *fc)
>
>                 spin_lock(&fiq->waitq.lock);
>                 fiq->connected = 0;
> +               fiq->aborted = is_abort;
>                 list_splice_init(&fiq->pending, &to_end2);
>                 list_for_each_entry(req, &to_end2, list)
>                         clear_bit(FR_PENDING, &req->flags);
> @@ -2151,7 +2154,7 @@ int fuse_dev_release(struct inode *inode, struct file *file)
>                 /* Are we the last open device? */
>                 if (atomic_dec_and_test(&fc->dev_count)) {
>                         WARN_ON(fc->iq.fasync != NULL);
> -                       fuse_abort_conn(fc);
> +                       fuse_abort_conn(fc, 0);
>                 }
>                 fuse_dev_free(fud);
>         }
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index d5773ca67ad2..650e72be4174 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -387,6 +387,9 @@ struct fuse_iqueue {
>         /** Connection established */
>         unsigned connected;
>
> +       /** Connection aborted via sysfs */
> +       int aborted;
> +

Does it need to be a per-queue granularity?   Isn't a single bool in
fuse_conn sufficient?

Thanks,
Miklos


>         /** Readers of the connection are waiting on this */
>         wait_queue_head_t waitq;
>
> @@ -414,6 +417,9 @@ struct fuse_pqueue {
>         /** Connection established */
>         unsigned connected;
>
> +       /** Connection aborted via sysfs */
> +       int aborted;
> +
>         /** Lock protecting accessess to  members of this structure */
>         spinlock_t lock;
>
> @@ -851,7 +857,7 @@ void fuse_request_send_background_locked(struct fuse_conn *fc,
>                                          struct fuse_req *req);
>
>  /* Abort all requests */
> -void fuse_abort_conn(struct fuse_conn *fc);
> +void fuse_abort_conn(struct fuse_conn *fc, int is_abort);
>
>  /**
>   * Invalidate inode attributes
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 94a745acaef8..8cc9de4ec16d 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -371,7 +371,7 @@ void fuse_unlock_inode(struct inode *inode)
>
>  static void fuse_umount_begin(struct super_block *sb)
>  {
> -       fuse_abort_conn(get_fuse_conn_super(sb));
> +       fuse_abort_conn(get_fuse_conn_super(sb), 0);
>  }
>
>  static void fuse_send_destroy(struct fuse_conn *fc)
> @@ -393,7 +393,7 @@ static void fuse_put_super(struct super_block *sb)
>
>         fuse_send_destroy(fc);
>
> -       fuse_abort_conn(fc);
> +       fuse_abort_conn(fc, 0);
>         mutex_lock(&fuse_mutex);
>         list_del(&fc->entry);
>         fuse_ctl_remove_conn(fc);
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 42fa977e3b14..4025a5599b2b 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -112,6 +112,9 @@
>   *  7.26
>   *  - add FUSE_HANDLE_KILLPRIV
>   *  - add FUSE_POSIX_ACL
> + *
> + *  7.27
> + *  - reading from /dev/fuse after sysfs abort returns -ECONNABORTED
>   */
>
>  #ifndef _LINUX_FUSE_H
> @@ -147,7 +150,7 @@
>  #define FUSE_KERNEL_VERSION 7
>
>  /** Minor version number of this interface */
> -#define FUSE_KERNEL_MINOR_VERSION 26
> +#define FUSE_KERNEL_MINOR_VERSION 27
>
>  /** The node ID of the root inode */
>  #define FUSE_ROOT_ID 1
> --
> 2.15.0
>