| Message ID | 20250319-fr_pending-race-v1-1-1f832af2f51e@ddn.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: {io-uring} Avoid possible FR_PENDING related list corruption | expand |
On Wed, Mar 19, 2025 at 5:37 AM Bernd Schubert <bschubert@ddn.com> wrote: > > The request is removed from the list of pending requests, > directly after follows a __fuse_put_request() which is > likely going to destruct the request, but that is not > guaranteed, better if FR_PENDING gets cleared. I think it is guaranteed that the request will be freed. there's only one call path for request_wait_answer(): __fuse_simple_request() fuse_get_req() -> request is allocated (req refcount is 1) __fuse_request_send() __fuse_get_request() -> req refcount is 2 fuse_send_one() request_wait_answer() fuse_put_request() -> req refcount is dropped if we hit that "if (test_bit(FR_PENDING, &req->flags))" case and request_wait_answer() drops the ref, then the request will be freed (or else it's leaked). imo we don't need this patch. Thanks, Joanne > > Suggested-by: Jeff Layton <jlayton@kernel.org> > Signed-off-by: Bernd Schubert <bschubert@ddn.com> > --- > fs/fuse/dev.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index 2c3a4d09e500f98232d5d9412a012235af6bec2e..124a6744e8088474efa014a483dc6d297cf321b7 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -438,6 +438,7 @@ static void request_wait_answer(struct fuse_req *req) > /* Request is not yet in userspace, bail out */ > if (test_bit(FR_PENDING, &req->flags)) { > list_del(&req->list); > + clear_bit(FR_PENDING, &req->flags); > spin_unlock(&fiq->lock); > __fuse_put_request(req); > req->out.h.error = -EINTR; > > -- > 2.43.0 >
On 3/19/25 22:15, Joanne Koong wrote: > On Wed, Mar 19, 2025 at 5:37 AM Bernd Schubert <bschubert@ddn.com> wrote: >> >> The request is removed from the list of pending requests, >> directly after follows a __fuse_put_request() which is >> likely going to destruct the request, but that is not >> guaranteed, better if FR_PENDING gets cleared. > > I think it is guaranteed that the request will be freed. there's only > one call path for request_wait_answer(): > __fuse_simple_request() > fuse_get_req() -> request is allocated (req refcount is 1) > __fuse_request_send() > __fuse_get_request() -> req refcount is 2 > fuse_send_one() > request_wait_answer() > fuse_put_request() -> req refcount is dropped > > if we hit that "if (test_bit(FR_PENDING, &req->flags))" case and > request_wait_answer() drops the ref, then the request will be freed > (or else it's leaked). imo we don't need this patch. > I thought I had seen a path that was holding the request, but now I see that every __fuse_get_request() actually also moves the request to another list. I'm going to add a comments into the next patch explaining why FR_PENDING is not cleared. Thanks, Bernd
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 2c3a4d09e500f98232d5d9412a012235af6bec2e..124a6744e8088474efa014a483dc6d297cf321b7 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -438,6 +438,7 @@ static void request_wait_answer(struct fuse_req *req) /* Request is not yet in userspace, bail out */ if (test_bit(FR_PENDING, &req->flags)) { list_del(&req->list); + clear_bit(FR_PENDING, &req->flags); spin_unlock(&fiq->lock); __fuse_put_request(req); req->out.h.error = -EINTR;
The request is removed from the list of pending requests, directly after follows a __fuse_put_request() which is likely going to destruct the request, but that is not guaranteed, better if FR_PENDING gets cleared. Suggested-by: Jeff Layton <jlayton@kernel.org> Signed-off-by: Bernd Schubert <bschubert@ddn.com> --- fs/fuse/dev.c | 1 + 1 file changed, 1 insertion(+)