On Tue, Feb 4, 2025 at 11:26 AM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
>
> Hi Bernd,
>
> On Tue, Feb 4, 2025 at 3:03 AM Bernd Schubert
> <bernd.schubert@xxxxxxxxxxx> wrote:
> >
> > Hi Joanne,
> >
> > On 2/3/25 19:50, Joanne Koong wrote:
> > > req->flags is set/tested/cleared atomically in fuse. When the FR_PENDING
> > > bit is cleared from the request flags when assigning a request to a
> > > uring entry, the fiq->lock does not need to be held.
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > > Fixes: c090c8abae4b6 ("fuse: Add io-uring sqe commit and fetch support")
> > > ---
> > > fs/fuse/dev_uring.c | 2 --
> > > 1 file changed, 2 deletions(-)
> > >
> > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> > > index ab8c26042aa8..42389d3e7235 100644
> > > --- a/fs/fuse/dev_uring.c
> > > +++ b/fs/fuse/dev_uring.c
> > > @@ -764,9 +764,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);
> >
> > I think that would have an issue in request_wait_answer(). Let's say
> >
> >
> > task-A, request_wait_answer(),
> > spin_lock(&fiq->lock);
> > /* Request is not yet in userspace, bail out */
> > if (test_bit(FR_PENDING, &req->flags)) { // ========> if passed
> > list_del(&req->list); // --> removes from the list
> >
> > task-B,
> > fuse_uring_add_req_to_ring_ent()
> > clear_bit(FR_PENDING, &req->flags);
> > ent->fuse_req = req;
> > ent->state = FRRS_FUSE_REQ;
> > list_move_tail(&ent->list, &queue->ent_w_req_queue);
> > fuse_uring_add_to_pq(ent, req); // ==> Add to list
> >
> >
> >
> > What I mean is, task-A passes the if, but is then slower than task-B. I.e.
> > task-B runs fuse_uring_add_to_pq() before task-B does the list_del.
> >
>
> Is this race condition possible given that fiq->ops->send_req() is
> called (and completed) before request_wait_answer() is called? The
> path I see is this:
>
> __fuse_simple_request()
> __fuse_request_send()
> fuse_send_one()
> fiq->ops->send_req()
> fuse_uring_queue_fuse_req()
> fuse_uring_add_req_to_ring_ent()
> clear FR_PENDING bit
> fuse_uring_add_to_pq()
> request_wait_answer()
>
> It doesn't seem like task A can call request_wait_answer() while task
> B is running fuse_uring_queue_fuse_req() on the same request while the
> request still has the FR_PENDING bit set.
>
> This case of task A running request_wait_answer() while task B is
> executing fuse_uring_add_req_to_ring_ent() can happen through
> fuse_uring_commit_fetch() -> fuse_uring_add_req_to_ring_ent(), but at
> that point the FR_PENDING flag will have already been cleared on the
> request, so this would bypass the "if (test_bit(FR_PENDING,...))"
> check in request_wait_answer().
>
> Is there something I'm missing? I think if this race condition is
> possible, then we also have a bigger problem where the request can be
> freed out in this request_wait_answer() -> if (test_bit(FR_PENDING,
> &req->flags))... case while fuse_uring_add_req_to_ring_ent() ->
> fuse_uring_add_to_pq() dereferences it still.
Oh okay so in the case where there is no ring entry available when we
send the request, then the pending bit does not get set on the request
and then a fuse_uring_commit_fetch() ->
fuse_uring_add_req_to_ring_ent() can run in parallel with the
request_wait_answer() -> if (test_bit(FR_PENDING, &req->flags))
logic.
But it looks like this race condition can happen right now in the
existing code where the last refcount on the request will get dropped
and the request freed while we're dereferencing it in
fuse_uring_add_to_pq(). I'm looking at fuse_uring_ent_assign_req()
right now - maybe we need to grab the fiq lock before we dequeue from
req_queue and then clear FR_PENDING wihile the lock is held? Gonna
think about this some more.
Thanks,
Joanne
>
>
> Thanks,
> Joanne
>
> >
> > Now the ring entry gets handled by fuse-server, comes back to fuse-client
> > and does not find the request anymore, because both tasks raced.
> > The entire ring entry will be lost - it not be used anymore for
> > the live time of the connection.
> >
> > And the other issue might be total list corruption, because two tasks might
> > access the list at the same time.
> >
> >
> > Thanks,
> > Bernd
> >
