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().
Reviewed-by: Bernd Schubert <bschubert@xxxxxxx>
