Re: Still a pretty bad time on 5.4.6 with fuse_request_end.

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, Feb 12, 2020 at 08:36:11PM +0100, Miklos Szeredi wrote:
> On Wed, Feb 12, 2020 at 10:38 AM Michael Stapelberg
> <michael+lkml@stapelberg.ch> wrote:
> >
> > Unfortunately not: when I change the code like so:
> >
> >     bool async;
> >     uint32_t opcode_early = req->args->opcode;
> >
> >     if (test_and_set_bit(FR_FINISHED, &req->flags))
> >         goto put_request;
> >
> >     async = req->args->end;
> >
> > …gdb only reports:
> >
> > (gdb) bt
> > #0  0x000000a700000001 in ?? ()
> > #1  0xffffffff8137fc99 in fuse_copy_finish (cs=0x20000ffffffff) at
> > fs/fuse/dev.c:681
> > Backtrace stopped: previous frame inner to this frame (corrupt stack?)
> >
> > But maybe that’s a hint in and of itself?
> 
> Yep, it's a stack use after return bug.   Attached patch should fix
> it, though I haven't tested it.

I think I have noticed couple of crashes in fuse_request_end() while
it was trying to call req->args->end() and I suspect this patch might
fix the issue.

Just that I have not been able to reproduce it reliably to be able test
it.

Vivek

> 
> Thanks,
> Miklos

> ---
>  fs/fuse/dev.c    |    6 +++---
>  fs/fuse/fuse_i.h |    2 ++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -276,12 +276,10 @@ static void flush_bg_queue(struct fuse_c
>  void fuse_request_end(struct fuse_conn *fc, struct fuse_req *req)
>  {
>  	struct fuse_iqueue *fiq = &fc->iq;
> -	bool async;
>  
>  	if (test_and_set_bit(FR_FINISHED, &req->flags))
>  		goto put_request;
>  
> -	async = req->args->end;
>  	/*
>  	 * test_and_set_bit() implies smp_mb() between bit
>  	 * changing and below intr_entry check. Pairs with
> @@ -324,7 +322,7 @@ void fuse_request_end(struct fuse_conn *
>  		wake_up(&req->waitq);
>  	}
>  
> -	if (async)
> +	if (test_bit(FR_ASYNC, &req->flags))
>  		req->args->end(fc, req->args, req->out.h.error);
>  put_request:
>  	fuse_put_request(fc, req);
> @@ -471,6 +469,8 @@ static void fuse_args_to_req(struct fuse
>  	req->in.h.opcode = args->opcode;
>  	req->in.h.nodeid = args->nodeid;
>  	req->args = args;
> +	if (args->end)
> +		set_bit(FR_ASYNC, &req->flags);
>  }
>  
>  ssize_t fuse_simple_request(struct fuse_conn *fc, struct fuse_args *args)
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -301,6 +301,7 @@ struct fuse_io_priv {
>   * FR_SENT:		request is in userspace, waiting for an answer
>   * FR_FINISHED:		request is finished
>   * FR_PRIVATE:		request is on private list
> + * FR_ASYNC:		request is asynchronous
>   */
>  enum fuse_req_flag {
>  	FR_ISREPLY,
> @@ -314,6 +315,7 @@ enum fuse_req_flag {
>  	FR_SENT,
>  	FR_FINISHED,
>  	FR_PRIVATE,
> +	FR_ASYNC,
>  };
>  
>  /**