On Tue, Mar 18, 2025 at 3:38 AM Bernd Schubert
<bernd.schubert@xxxxxxxxxxx> wrote:
>
>
>
> On 3/18/25 01:30, Joanne Koong wrote:
> > There is a race condition leading to a kernel crash from a null
> > dereference when attemping to access fc->lock in
> > fuse_uring_create_queue(). fc may be NULL in the case where another
> > thread is creating the uring in fuse_uring_create() and has set
> > fc->ring but has not yet set ring->fc when fuse_uring_create_queue()
> > reads ring->fc. There is another race condition as well where in
> > fuse_uring_register(), ring->nr_queues may still be 0 and not yet set
> > to the new value when we compare qid against it.
> >
> > This fix sets fc->ring only after ring->fc and ring->nr_queues have been
> > set, which guarantees now that ring->fc is a proper pointer when any
> > queues are created and ring->nr_queues reflects the right number of
> > queues if ring is not NULL. We must use smp_store_release() and
> > smp_load_acquire() semantics to ensure the ordering will remain correct
> > where fc->ring is assigned only after ring->fc and ring->nr_queues have
> > been assigned.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > Fixes: 24fe962c86f5 ("fuse: {io-uring} Handle SQEs - register commands")
> >
> > ---
> >
> > Changes between v2 -> v3:
> > * v2 implementation still has race condition for ring->nr_queues
> > *link to v2: https://lore.kernel.org/linux-fsdevel/20250314205033.762641-1-joannelkoong@xxxxxxxxx/
> >
> > Changes between v1 -> v2:
> > * v1 implementation may be reordered by compiler (Bernd)
> > * link to v1: https://lore.kernel.org/linux-fsdevel/20250314191334.215741-1-joannelkoong@xxxxxxxxx/
> >
> > ---
> > fs/fuse/dev_uring.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> > index ab8c26042aa8..97e6d31479e0 100644
> > --- a/fs/fuse/dev_uring.c
> > +++ b/fs/fuse/dev_uring.c
> > @@ -235,11 +235,11 @@ static struct fuse_ring *fuse_uring_create(struct fuse_conn *fc)
> >
> > init_waitqueue_head(&ring->stop_waitq);
> >
> > - fc->ring = ring;
> > ring->nr_queues = nr_queues;
> > ring->fc = fc;
> > ring->max_payload_sz = max_payload_size;
> > atomic_set(&ring->queue_refs, 0);
> > + smp_store_release(&fc->ring, ring);
> >
> > spin_unlock(&fc->lock);
> > return ring;
> > @@ -1068,7 +1068,7 @@ static int fuse_uring_register(struct io_uring_cmd *cmd,
> > unsigned int issue_flags, struct fuse_conn *fc)
> > {
> > const struct fuse_uring_cmd_req *cmd_req = io_uring_sqe_cmd(cmd->sqe);
> > - struct fuse_ring *ring = fc->ring;
> > + struct fuse_ring *ring = smp_load_acquire(&fc->ring);
> > struct fuse_ring_queue *queue;
> > struct fuse_ring_ent *ent;
> > int err;
>
> I was actually debating with myself that smp_load_acquire() on Friday.
> I think we do not need it, because if the ring is not found, it will
> go into the spin lock. But it does not hurt either and it might
> cleaner to have a pair of smp_store_release() and smp_load_acquire().
>
I think the problem is that if we don't have it, then ring may be
non-null but the ring->nr_queues value we read in "if (qid >=
ring->nr_queues)" might still be the stale value, which would make the
uring registration fail with -EINVAL.
Thanks,
Joanne
>
> Reviewed-by: Bernd Schubert <bschubert@xxxxxxx>
