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@xxxxxxxxxxxxx> 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,
> };
>
> /**
