[PATCH] io_uring: consistently use rcu semantics with sqpoll thread

[PATCH] io_uring: consistently use rcu semantics with sqpoll thread

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

It is already dereferenced with rcu read protection, so it needs to be
annotated as such, and consistently use rcu helpers for access and
assignment.

Fixes: ac0b8b327a5677d ("io_uring: fix use-after-free of sq->thread in __io_uring_show_fdinfo()")
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 io_uring/sqpoll.c | 16 +++++++++-------
 io_uring/sqpoll.h |  2 +-
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
index 0625a421626f4..7c6d7de05e0a0 100644
--- a/io_uring/sqpoll.c
+++ b/io_uring/sqpoll.c
@@ -30,7 +30,7 @@ enum {
 void io_sq_thread_unpark(struct io_sq_data *sqd)
 	__releases(&sqd->lock)
 {
-	WARN_ON_ONCE(sqd->thread == current);
+	WARN_ON_ONCE(rcu_access_pointer(sqd->thread) == current);
 
 	/*
 	 * Do the dance but not conditional clear_bit() because it'd race with
@@ -46,24 +46,24 @@ void io_sq_thread_unpark(struct io_sq_data *sqd)
 void io_sq_thread_park(struct io_sq_data *sqd)
 	__acquires(&sqd->lock)
 {
-	WARN_ON_ONCE(data_race(sqd->thread) == current);
+	WARN_ON_ONCE(data_race(rcu_access_pointer(sqd->thread)) == current);
 
 	atomic_inc(&sqd->park_pending);
 	set_bit(IO_SQ_THREAD_SHOULD_PARK, &sqd->state);
 	mutex_lock(&sqd->lock);
 	if (sqd->thread)
-		wake_up_process(sqd->thread);
+		wake_up_process(rcu_access_pointer(sqd->thread));
 }
 
 void io_sq_thread_stop(struct io_sq_data *sqd)
 {
-	WARN_ON_ONCE(sqd->thread == current);
+	WARN_ON_ONCE(rcu_access_pointer(sqd->thread) == current);
 	WARN_ON_ONCE(test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state));
 
 	set_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state);
 	mutex_lock(&sqd->lock);
 	if (sqd->thread)
-		wake_up_process(sqd->thread);
+		wake_up_process(rcu_access_pointer(sqd->thread));
 	mutex_unlock(&sqd->lock);
 	wait_for_completion(&sqd->exited);
 }
@@ -486,7 +486,7 @@ __cold int io_sq_offload_create(struct io_ring_ctx *ctx,
 			goto err_sqpoll;
 		}
 
-		sqd->thread = tsk;
+		rcu_assign_pointer(sqd->thread, tsk);
 		task_to_put = get_task_struct(tsk);
 		ret = io_uring_alloc_task_context(tsk, ctx);
 		wake_up_new_task(tsk);
@@ -517,7 +517,9 @@ __cold int io_sqpoll_wq_cpu_affinity(struct io_ring_ctx *ctx,
 		io_sq_thread_park(sqd);
 		/* Don't set affinity for a dying thread */
 		if (sqd->thread)
-			ret = io_wq_cpu_affinity(sqd->thread->io_uring, mask);
+			ret = io_wq_cpu_affinity(
+				rcu_access_pointer(sqd->thread)->io_uring,
+				mask);
 		io_sq_thread_unpark(sqd);
 	}
 
diff --git a/io_uring/sqpoll.h b/io_uring/sqpoll.h
index 4171666b1cf4c..43f69d3cf2959 100644
--- a/io_uring/sqpoll.h
+++ b/io_uring/sqpoll.h
@@ -8,7 +8,7 @@ struct io_sq_data {
 	/* ctx's that are using this sqd */
 	struct list_head	ctx_list;
 
-	struct task_struct	*thread;
+	struct task_struct __rcu *thread;
 	struct wait_queue_head	wait;
 
 	unsigned		sq_thread_idle;
-- 
2.47.1

Re: [PATCH] io_uring: consistently use rcu semantics with sqpoll thread

[Jens Axboe]

On 6/10/25 1:30 PM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> It is already dereferenced with rcu read protection, so it needs to be
> annotated as such, and consistently use rcu helpers for access and
> assignment.

There are some bits in io_uring.c that access it, which probably need
some attention too I think. One of them a bit trickier.

-- 
Jens Axboe

Re: [PATCH] io_uring: consistently use rcu semantics with sqpoll thread

[Keith Busch]

On Tue, Jun 10, 2025 at 02:04:41PM -0600, Jens Axboe wrote:
> On 6/10/25 1:30 PM, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > It is already dereferenced with rcu read protection, so it needs to be
> > annotated as such, and consistently use rcu helpers for access and
> > assignment.
> 
> There are some bits in io_uring.c that access it, which probably need
> some attention too I think. One of them a bit trickier.

Oh, sure is. I just ran 'make C=1' on the originally affected files, but
should have ran it on all of io_uring/.

I think the below should clear up the new warnings. I think it's safe to
hold the rcu read lock for the tricky one as io_wq_cancel_cb() doesn't
appear to make any blocking calls.

---
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index cf759c172083c..c6502197eb6b2 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -2906,10 +2906,12 @@ static __cold void io_ring_exit_work(struct work_struct *work)
 			struct task_struct *tsk;
 
 			io_sq_thread_park(sqd);
-			tsk = sqd->thread;
+			rcu_read_lock();
+			tsk = rcu_dereference(sqd->thread);
 			if (tsk && tsk->io_uring && tsk->io_uring->io_wq)
 				io_wq_cancel_cb(tsk->io_uring->io_wq,
 						io_cancel_ctx_cb, ctx, true);
+			rcu_read_unlock();
 			io_sq_thread_unpark(sqd);
 		}
 
@@ -3142,7 +3144,7 @@ __cold void io_uring_cancel_generic(bool cancel_all, struct io_sq_data *sqd)
 	s64 inflight;
 	DEFINE_WAIT(wait);
 
-	WARN_ON_ONCE(sqd && sqd->thread != current);
+	WARN_ON_ONCE(sqd && rcu_access_pointer(sqd->thread) != current);
 
 	if (!current->io_uring)
 		return;
--

Re: [PATCH] io_uring: consistently use rcu semantics with sqpoll thread

[Jens Axboe]

On 6/10/25 2:20 PM, Keith Busch wrote:
> On Tue, Jun 10, 2025 at 02:04:41PM -0600, Jens Axboe wrote:
>> On 6/10/25 1:30 PM, Keith Busch wrote:
>>> From: Keith Busch <kbusch@kernel.org>
>>>
>>> It is already dereferenced with rcu read protection, so it needs to be
>>> annotated as such, and consistently use rcu helpers for access and
>>> assignment.
>>
>> There are some bits in io_uring.c that access it, which probably need
>> some attention too I think. One of them a bit trickier.
> 
> Oh, sure is. I just ran 'make C=1' on the originally affected files, but
> should have ran it on all of io_uring/.
> 
> I think the below should clear up the new warnings. I think it's safe to
> hold the rcu read lock for the tricky one as io_wq_cancel_cb() doesn't
> appear to make any blocking calls.

It _probably_ is, but that's entirely untested. Right now it looks fine,
for a variety of reasons like submitting work that's marked cancel
should not be doing anything with it really. But it doesn't feel me with
joy, particularly as only the somewhat uncommon SQPOLL is the one that
will do it.

The io_sq_thread_park() parts in the patch also look broken, as an
rcu_access_pointer() is being passed into wake_up_process(). It should
all be fine, but it's now a case of instrumentation actively making the
code more confusing to read.

I think we might be better off leaving the sparse warnings and doing a
proper io_sq_data accessor thing for this, rather than try and paper
over the sparse warnings.

-- 
Jens Axboe

Re: [PATCH] io_uring: consistently use rcu semantics with sqpoll thread

[Jens Axboe]

On 6/10/25 2:45 PM, Jens Axboe wrote:
> On 6/10/25 2:20 PM, Keith Busch wrote:
>> On Tue, Jun 10, 2025 at 02:04:41PM -0600, Jens Axboe wrote:
>>> On 6/10/25 1:30 PM, Keith Busch wrote:
>>>> From: Keith Busch <kbusch@kernel.org>
>>>>
>>>> It is already dereferenced with rcu read protection, so it needs to be
>>>> annotated as such, and consistently use rcu helpers for access and
>>>> assignment.
>>>
>>> There are some bits in io_uring.c that access it, which probably need
>>> some attention too I think. One of them a bit trickier.
>>
>> Oh, sure is. I just ran 'make C=1' on the originally affected files, but
>> should have ran it on all of io_uring/.
>>
>> I think the below should clear up the new warnings. I think it's safe to
>> hold the rcu read lock for the tricky one as io_wq_cancel_cb() doesn't
>> appear to make any blocking calls.
> 
> It _probably_ is, but that's entirely untested. Right now it looks fine,
> for a variety of reasons like submitting work that's marked cancel
> should not be doing anything with it really. But it doesn't feel me with
> joy, particularly as only the somewhat uncommon SQPOLL is the one that
> will do it.
> 
> The io_sq_thread_park() parts in the patch also look broken, as an
> rcu_access_pointer() is being passed into wake_up_process(). It should
> all be fine, but it's now a case of instrumentation actively making the
> code more confusing to read.
> 
> I think we might be better off leaving the sparse warnings and doing a
> proper io_sq_data accessor thing for this, rather than try and paper
> over the sparse warnings.

Ah we can probably get by with just a bit more work, something like
this per section should do it.

diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
index 0625a421626f..8852e104ed68 100644
--- a/io_uring/sqpoll.c
+++ b/io_uring/sqpoll.c
@@ -46,13 +46,18 @@ void io_sq_thread_unpark(struct io_sq_data *sqd)
 void io_sq_thread_park(struct io_sq_data *sqd)
 	__acquires(&sqd->lock)
 {
-	WARN_ON_ONCE(data_race(sqd->thread) == current);
+	struct task_struct *tsk;
 
 	atomic_inc(&sqd->park_pending);
 	set_bit(IO_SQ_THREAD_SHOULD_PARK, &sqd->state);
 	mutex_lock(&sqd->lock);
-	if (sqd->thread)
-		wake_up_process(sqd->thread);
+	rcu_read_lock();
+	tsk = rcu_dereference(sqd->thread);
+	if (tsk) {
+		WARN_ON_ONCE(tsk == current);
+		wake_up_process(tsk);
+	}
+	rcu_read_unlock();
 }
 
 void io_sq_thread_stop(struct io_sq_data *sqd)

-- 
Jens Axboe

Re: [PATCH] io_uring: consistently use rcu semantics with sqpoll thread

[Keith Busch]

On Tue, Jun 10, 2025 at 02:52:28PM -0600, Jens Axboe wrote:
> Ah we can probably get by with just a bit more work, something like
> this per section should do it.

Nice, and it clears up the data_race() usage with the assignment and
access both under the sqd mutex.
 
> diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
> index 0625a421626f..8852e104ed68 100644
> --- a/io_uring/sqpoll.c
> +++ b/io_uring/sqpoll.c
> @@ -46,13 +46,18 @@ void io_sq_thread_unpark(struct io_sq_data *sqd)
>  void io_sq_thread_park(struct io_sq_data *sqd)
>  	__acquires(&sqd->lock)
>  {
> -	WARN_ON_ONCE(data_race(sqd->thread) == current);
> +	struct task_struct *tsk;
>  
>  	atomic_inc(&sqd->park_pending);
>  	set_bit(IO_SQ_THREAD_SHOULD_PARK, &sqd->state);
>  	mutex_lock(&sqd->lock);
> -	if (sqd->thread)
> -		wake_up_process(sqd->thread);
> +	rcu_read_lock();
> +	tsk = rcu_dereference(sqd->thread);
> +	if (tsk) {
> +		WARN_ON_ONCE(tsk == current);
> +		wake_up_process(tsk);
> +	}
> +	rcu_read_unlock();
>  }
>  
>  void io_sq_thread_stop(struct io_sq_data *sqd)
> 
> -- 
> Jens Axboe