Re: [PATCH] fuse: fix uring race condition for null dereference of fc

[Joanne Koong <joannelkoong@xxxxxxxxx>]

On Fri, Mar 14, 2025 at 12:46 PM Bernd Schubert
<bernd.schubert@xxxxxxxxxxx> wrote:
>
> Hi Joanne,
>
> On 3/14/25 20:13, 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.
> >
> > This fix sets fc->ring only after ring->fc has been set, which
> > guarantees now that ring->fc is a proper pointer when any queues are
> > created.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > Fixes: 24fe962c86f5 ("fuse: {io-uring} Handle SQEs - register commands")
> > ---
> >  fs/fuse/dev_uring.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> > index ab8c26042aa8..618a413ef400 100644
> > --- a/fs/fuse/dev_uring.c
> > +++ b/fs/fuse/dev_uring.c
> > @@ -235,9 +235,9 @@ 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;
> > +     fc->ring = ring;
> >       ring->max_payload_sz = max_payload_size;
> >       atomic_set(&ring->queue_refs, 0);
> >
>
> oh, I  didn't get that and even KCSAN didn't complain. But I see that it
> would be possible. I'm just a bit scared that the compiler might
> re-order things on its own.
>
> What about this?

Hi Bernd,

I think an easier way then might just be

diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
index ab8c26042aa8..64f1ae308dc4 100644
--- a/fs/fuse/dev_uring.c
+++ b/fs/fuse/dev_uring.c
@@ -250,10 +250,10 @@ static struct fuse_ring
*fuse_uring_create(struct fuse_conn *fc)
        return res;
 }

-static struct fuse_ring_queue *fuse_uring_create_queue(struct fuse_ring *ring,
+static struct fuse_ring_queue *fuse_uring_create_queue(struct fuse_conn *fc,
+                                                      struct fuse_ring *ring,
                                                       int qid)
 {
-       struct fuse_conn *fc = ring->fc;
        struct fuse_ring_queue *queue;
        struct list_head *pq;

@@ -1088,7 +1088,7 @@ static int fuse_uring_register(struct io_uring_cmd *cmd,

        queue = ring->queues[qid];
        if (!queue) {
-               queue = fuse_uring_create_queue(ring, qid);
+               queue = fuse_uring_create_queue(fc, ring, qid);
                if (!queue)
                        return err;
        }

where we pass fc directly. I'll submit this as v2. i couldn't make up
my mind between the two initially :)

Thanks,
Joanne

>
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index 9d78c9f29a09..f33a7e6f5ec3 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -241,11 +241,12 @@ 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);
> +       /* Ensures initialization is visible before ring pointer */
> +       smp_store_release(&fc->ring, ring);
>
>         spin_unlock(&fc->lock);
>         return ring;
>
>
>
> Thanks,
> Bernd