| Message ID | 20250319-fr_pending-race-v1-2-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: > > task-A (application) might be in request_wait_answer and > try to remove the request when it has FR_PENDING set. > > task-B (a fuse-server io-uring task) might handle this > request with FUSE_IO_URING_CMD_COMMIT_AND_FETCH, when > fetching the next request and accessed the req from > the pending list in fuse_uring_ent_assign_req(). > That code path was not protected by fiq->lock and so > might race with task-A. > > For scaling reasons we better don't use fiq->lock, but > add a handler to remove canceled requests from the queue. > > This also removes usage of fiq->lock from > fuse_uring_add_req_to_ring_ent() altogether, as it was > there just to protect against this race and incomplete. > > Fixes: c090c8abae4b ("fuse: Add io-uring sqe commit and fetch support") > Reported-by: Joanne Koong <joannelkoong@gmail.com> > Closes: https://lore.kernel.org/all/CAJnrk1ZgHNb78dz-yfNTpxmW7wtT88A=m-zF0ZoLXKLUHRjNTw@mail.gmail.com/ > Signed-off-by: Bernd Schubert <bschubert@ddn.com> LGTM but imo the code looks cleaner with fuse_uring_remove_pending_req() just directly calling fuse_remove_pending_req() instead of passing in "bool (*remove_fn)(struct fuse_req *req, spinlock_t *lock))" as a function arg. Signed-off-by: Joanne Koong <joannelkoong@gmail.com> > --- > fs/fuse/dev.c | 33 +++++++++++++++++++++++---------- > fs/fuse/dev_uring.c | 17 +++++++++++++---- > fs/fuse/dev_uring_i.h | 10 ++++++++++ > fs/fuse/fuse_i.h | 2 ++ > 4 files changed, 48 insertions(+), 14 deletions(-) > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index 124a6744e8088474efa014a483dc6d297cf321b7..20c82bb2313b95cdc910808ee4968804077ed05b 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -407,6 +407,21 @@ static int queue_interrupt(struct fuse_req *req) > return 0; > } > > +static bool fuse_remove_pending_req(struct fuse_req *req, spinlock_t *lock) > +{ > + spin_lock(lock); > + if (test_bit(FR_PENDING, &req->flags)) { > + list_del(&req->list); > + clear_bit(FR_PENDING, &req->flags); > + spin_unlock(lock); > + __fuse_put_request(req); > + req->out.h.error = -EINTR; > + return true; > + } > + spin_unlock(lock); > + return false; > +} > + > static void request_wait_answer(struct fuse_req *req) > { > struct fuse_conn *fc = req->fm->fc; > @@ -428,23 +443,21 @@ static void request_wait_answer(struct fuse_req *req) > } > > if (!test_bit(FR_FORCE, &req->flags)) { > + bool removed; > + > /* Only fatal signals may interrupt this */ > err = wait_event_killable(req->waitq, > test_bit(FR_FINISHED, &req->flags)); > if (!err) > return; > > - spin_lock(&fiq->lock); > - /* 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; > + if (test_bit(FR_URING, &req->flags)) > + removed = fuse_uring_remove_pending_req( > + req, fuse_remove_pending_req); > + else > + removed = fuse_remove_pending_req(req, &fiq->lock); > + if (removed) > return; > - } > - spin_unlock(&fiq->lock); > } > > /* > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c > index ebd2931b4f2acac461091b6b1f1176cde759e2d1..0d7fe8d6d2bf214b38bc90f6a7a9b4840219a81c 100644 > --- a/fs/fuse/dev_uring.c > +++ b/fs/fuse/dev_uring.c > @@ -726,8 +726,6 @@ static void fuse_uring_add_req_to_ring_ent(struct fuse_ring_ent *ent, > struct fuse_req *req) > { > struct fuse_ring_queue *queue = ent->queue; > - struct fuse_conn *fc = req->fm->fc; > - struct fuse_iqueue *fiq = &fc->iq; > > lockdep_assert_held(&queue->lock); > > @@ -737,9 +735,7 @@ static void fuse_uring_add_req_to_ring_ent(struct fuse_ring_ent *ent, > ent->state); > } > > - spin_lock(&fiq->lock); > clear_bit(FR_PENDING, &req->flags); > - spin_unlock(&fiq->lock); > ent->fuse_req = req; > ent->state = FRRS_FUSE_REQ; > list_move(&ent->list, &queue->ent_w_req_queue); > @@ -1238,6 +1234,8 @@ void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req) > if (unlikely(queue->stopped)) > goto err_unlock; > > + set_bit(FR_URING, &req->flags); > + req->ring_queue = queue; > ent = list_first_entry_or_null(&queue->ent_avail_queue, > struct fuse_ring_ent, list); > if (ent) > @@ -1276,6 +1274,8 @@ bool fuse_uring_queue_bq_req(struct fuse_req *req) > return false; > } > > + set_bit(FR_URING, &req->flags); > + req->ring_queue = queue; > list_add_tail(&req->list, &queue->fuse_req_bg_queue); > > ent = list_first_entry_or_null(&queue->ent_avail_queue, > @@ -1306,6 +1306,15 @@ bool fuse_uring_queue_bq_req(struct fuse_req *req) > return true; > } > > +bool fuse_uring_remove_pending_req(struct fuse_req *req, > + bool (*remove_fn)(struct fuse_req *req, > + spinlock_t *lock)) nit: indentation should be aligned here? > +{ > + struct fuse_ring_queue *queue = req->ring_queue; > + > + return remove_fn(req, &queue->lock); > +} > + > static const struct fuse_iqueue_ops fuse_io_uring_ops = { > /* should be send over io-uring as enhancement */ > .send_forget = fuse_dev_queue_forget, > diff --git a/fs/fuse/dev_uring_i.h b/fs/fuse/dev_uring_i.h > index 2102b3d0c1aed1105e9c1200c91e1cb497b9a597..89a1da485b0e06fc6096f9b343dc0855c5df9c0b 100644 > --- a/fs/fuse/dev_uring_i.h > +++ b/fs/fuse/dev_uring_i.h > @@ -142,6 +142,9 @@ void fuse_uring_abort_end_requests(struct fuse_ring *ring); > int fuse_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags); > void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req); > bool fuse_uring_queue_bq_req(struct fuse_req *req); > +bool fuse_uring_remove_pending_req(struct fuse_req *req, > + bool (*remove_fn)(struct fuse_req *req, > + spinlock_t *lock)); nit: indentation needs to be aligned here? > > static inline void fuse_uring_abort(struct fuse_conn *fc) > { > @@ -200,6 +203,13 @@ static inline bool fuse_uring_ready(struct fuse_conn *fc) > return false; > } > > +static inline bool fuse_uring_remove_pending_req( > + struct fuse_req *req, > + bool (*remove_fn)(struct fuse_req *req, spinlock_t *lock)) > +{ > + return false; > +} > + > #endif /* CONFIG_FUSE_IO_URING */ > > #endif /* _FS_FUSE_DEV_URING_I_H */ > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h > index fee96fe7887b30cd57b8a6bbda11447a228cf446..5428a5b5e16a880894142f0ec1176a349c9469dc 100644 > --- a/fs/fuse/fuse_i.h > +++ b/fs/fuse/fuse_i.h > @@ -392,6 +392,7 @@ enum fuse_req_flag { > FR_FINISHED, > FR_PRIVATE, > FR_ASYNC, > + FR_URING, > }; > > /** > @@ -441,6 +442,7 @@ struct fuse_req { > > #ifdef CONFIG_FUSE_IO_URING > void *ring_entry; > + void *ring_queue; > #endif > }; > > > -- > 2.43.0 >
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c index 124a6744e8088474efa014a483dc6d297cf321b7..20c82bb2313b95cdc910808ee4968804077ed05b 100644 --- a/fs/fuse/dev.c +++ b/fs/fuse/dev.c @@ -407,6 +407,21 @@ static int queue_interrupt(struct fuse_req *req) return 0; } +static bool fuse_remove_pending_req(struct fuse_req *req, spinlock_t *lock) +{ + spin_lock(lock); + if (test_bit(FR_PENDING, &req->flags)) { + list_del(&req->list); + clear_bit(FR_PENDING, &req->flags); + spin_unlock(lock); + __fuse_put_request(req); + req->out.h.error = -EINTR; + return true; + } + spin_unlock(lock); + return false; +} + static void request_wait_answer(struct fuse_req *req) { struct fuse_conn *fc = req->fm->fc; @@ -428,23 +443,21 @@ static void request_wait_answer(struct fuse_req *req) } if (!test_bit(FR_FORCE, &req->flags)) { + bool removed; + /* Only fatal signals may interrupt this */ err = wait_event_killable(req->waitq, test_bit(FR_FINISHED, &req->flags)); if (!err) return; - spin_lock(&fiq->lock); - /* 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; + if (test_bit(FR_URING, &req->flags)) + removed = fuse_uring_remove_pending_req( + req, fuse_remove_pending_req); + else + removed = fuse_remove_pending_req(req, &fiq->lock); + if (removed) return; - } - spin_unlock(&fiq->lock); } /* diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c index ebd2931b4f2acac461091b6b1f1176cde759e2d1..0d7fe8d6d2bf214b38bc90f6a7a9b4840219a81c 100644 --- a/fs/fuse/dev_uring.c +++ b/fs/fuse/dev_uring.c @@ -726,8 +726,6 @@ static void fuse_uring_add_req_to_ring_ent(struct fuse_ring_ent *ent, struct fuse_req *req) { struct fuse_ring_queue *queue = ent->queue; - struct fuse_conn *fc = req->fm->fc; - struct fuse_iqueue *fiq = &fc->iq; lockdep_assert_held(&queue->lock); @@ -737,9 +735,7 @@ static void fuse_uring_add_req_to_ring_ent(struct fuse_ring_ent *ent, ent->state); } - spin_lock(&fiq->lock); clear_bit(FR_PENDING, &req->flags); - spin_unlock(&fiq->lock); ent->fuse_req = req; ent->state = FRRS_FUSE_REQ; list_move(&ent->list, &queue->ent_w_req_queue); @@ -1238,6 +1234,8 @@ void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req) if (unlikely(queue->stopped)) goto err_unlock; + set_bit(FR_URING, &req->flags); + req->ring_queue = queue; ent = list_first_entry_or_null(&queue->ent_avail_queue, struct fuse_ring_ent, list); if (ent) @@ -1276,6 +1274,8 @@ bool fuse_uring_queue_bq_req(struct fuse_req *req) return false; } + set_bit(FR_URING, &req->flags); + req->ring_queue = queue; list_add_tail(&req->list, &queue->fuse_req_bg_queue); ent = list_first_entry_or_null(&queue->ent_avail_queue, @@ -1306,6 +1306,15 @@ bool fuse_uring_queue_bq_req(struct fuse_req *req) return true; } +bool fuse_uring_remove_pending_req(struct fuse_req *req, + bool (*remove_fn)(struct fuse_req *req, + spinlock_t *lock)) +{ + struct fuse_ring_queue *queue = req->ring_queue; + + return remove_fn(req, &queue->lock); +} + static const struct fuse_iqueue_ops fuse_io_uring_ops = { /* should be send over io-uring as enhancement */ .send_forget = fuse_dev_queue_forget, diff --git a/fs/fuse/dev_uring_i.h b/fs/fuse/dev_uring_i.h index 2102b3d0c1aed1105e9c1200c91e1cb497b9a597..89a1da485b0e06fc6096f9b343dc0855c5df9c0b 100644 --- a/fs/fuse/dev_uring_i.h +++ b/fs/fuse/dev_uring_i.h @@ -142,6 +142,9 @@ void fuse_uring_abort_end_requests(struct fuse_ring *ring); int fuse_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags); void fuse_uring_queue_fuse_req(struct fuse_iqueue *fiq, struct fuse_req *req); bool fuse_uring_queue_bq_req(struct fuse_req *req); +bool fuse_uring_remove_pending_req(struct fuse_req *req, + bool (*remove_fn)(struct fuse_req *req, + spinlock_t *lock)); static inline void fuse_uring_abort(struct fuse_conn *fc) { @@ -200,6 +203,13 @@ static inline bool fuse_uring_ready(struct fuse_conn *fc) return false; } +static inline bool fuse_uring_remove_pending_req( + struct fuse_req *req, + bool (*remove_fn)(struct fuse_req *req, spinlock_t *lock)) +{ + return false; +} + #endif /* CONFIG_FUSE_IO_URING */ #endif /* _FS_FUSE_DEV_URING_I_H */ diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index fee96fe7887b30cd57b8a6bbda11447a228cf446..5428a5b5e16a880894142f0ec1176a349c9469dc 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -392,6 +392,7 @@ enum fuse_req_flag { FR_FINISHED, FR_PRIVATE, FR_ASYNC, + FR_URING, }; /** @@ -441,6 +442,7 @@ struct fuse_req { #ifdef CONFIG_FUSE_IO_URING void *ring_entry; + void *ring_queue; #endif };
task-A (application) might be in request_wait_answer and try to remove the request when it has FR_PENDING set. task-B (a fuse-server io-uring task) might handle this request with FUSE_IO_URING_CMD_COMMIT_AND_FETCH, when fetching the next request and accessed the req from the pending list in fuse_uring_ent_assign_req(). That code path was not protected by fiq->lock and so might race with task-A. For scaling reasons we better don't use fiq->lock, but add a handler to remove canceled requests from the queue. This also removes usage of fiq->lock from fuse_uring_add_req_to_ring_ent() altogether, as it was there just to protect against this race and incomplete. Fixes: c090c8abae4b ("fuse: Add io-uring sqe commit and fetch support") Reported-by: Joanne Koong <joannelkoong@gmail.com> Closes: https://lore.kernel.org/all/CAJnrk1ZgHNb78dz-yfNTpxmW7wtT88A=m-zF0ZoLXKLUHRjNTw@mail.gmail.com/ Signed-off-by: Bernd Schubert <bschubert@ddn.com> --- fs/fuse/dev.c | 33 +++++++++++++++++++++++---------- fs/fuse/dev_uring.c | 17 +++++++++++++---- fs/fuse/dev_uring_i.h | 10 ++++++++++ fs/fuse/fuse_i.h | 2 ++ 4 files changed, 48 insertions(+), 14 deletions(-)