Re: [PATCH 5/7] fuse: Introduce generic fuse_copy_aborted()

[Miklos Szeredi <miklos@szeredi.hu>]

On Tue, Jan 15, 2019 at 11:19 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> There is no a reason to set individual FR_ABORTED state
> for every request, since fuse_abort_conn() aborts all
> unlocked requests at once. FR_ABORTED bit just makes
> fuse_copy_aborted() to end some of requests, which are
> in the middle of fuse_dev_do_read() and fuse_dev_do_write(),
> but this is not a big deal. These functions may abort
> the requests themselves.
>
> The patch kills lock_request and unlock_request(),
> and introduces generic fuse_copy_aborted() to use
> instead of them. This allows next patches to kill
> FR_ABORTED, FR_LOCKED and FR_PRIVATE, and simplify
> fuse dev read/write function.

Patch is no good: you assume that fuse_copy_args() called from
fuse_dev_do_*() will finish in finite time.  That's not the case if
page fault on userspace buffer is hung (see e.g.
Documentation/filesystems/fuse.txt #Tricky deadlock).

You *are* right that this is a big wart in the current implementation,
and would be really nice to get rid of.  Certainly doable, just need
to make sure all buffers stored in the request are properly refcounted
and have lifetime bound to that of the request, i.e. all synchronous
requests (not background) can be aborted asynchronously, the caller of
the request can return in this case with an abort, yet the request
processing (copy to/from userspace) can finish without encountering
already freed memory.

The fuse_simple_request() thing was a first step in this direction, so
that one's pretty easy to deal with: copy arguments to a temporary
buffer that is freed only when the request itself is freed...

Requests which have pages need special attention.   A ref is taken for
those pages, so they may be OK, but in some cases they may not be.

Thanks,
Miklos