Re: [PATCH 08/10] io_uring/cancel: drop sqd->lock while SQPOLL waits

[Jens Axboe <axboe@kernel.dk>]

On 10/29/26 7:25 PM, Karl Mehltretter wrote:
>> diff --git a/io_uring/cancel.c b/io_uring/cancel.c
>> index 6f7a6cf356c2..79b5c133319b 100644
>> --- a/io_uring/cancel.c
>> +++ b/io_uring/cancel.c
>> @@ -703,7 +703,16 @@ __cold void io_uring_cancel_generic(bool cancel_all, struct io_sq_data *sqd)
>>  		}
>>  
>>  		if (loop) {
>> +			/*
>> +			 * A request that runs in io-wq and doesn't end makes
>> +			 * each round come here. Don't hold sqd->lock for that
>> +			 * long either.
>> +			 */
>> +			if (sqd)
>> +				mutex_unlock(&sqd->lock);
>>  			cond_resched();
>> +			if (sqd)
>> +				mutex_lock(&sqd->lock);
>>  			continue;
>>  		}
> 
> Doesn't dropping sqd->lock here allow IORING_SETUP_ATTACH_WQ to attach
> to an SQPOLL thread that is already exiting?
> 
> io_sq_offload_create() checks !sqd->thread under sqd->lock to avoid
> attaching to a dying thread:
> 
> 	/* don't attach to a dying SQPOLL thread, would be racy */
> 	ret = (attached && !sqd->thread) ? -ENXIO : 0;
> 
> Previously, once io_sq_thread() left its main loop, it held sqd->lock
> across io_uring_cancel_generic() until it cleared sqd->thread. With this
> unlock, io_sq_offload_create() can take sqd->lock while cancellation is
> waiting, see sqd->thread still set, add the new ctx to sqd->ctx_list,
> and succeed. io_sq_thread() then finishes canceling, clears sqd->thread,
> and exits without ever servicing the new ring.
> 
> When testing this race in a VM, io_uring_setup() succeeds instead of
> failing with -ENXIO, and the new ring immediately fails with -EOWNERDEAD
> on io_uring_enter().
> 
> Maybe io_sq_thread() could set an exiting bit in sqd->state before
> calling io_uring_cancel_generic(), and io_sq_offload_create() could
> check that alongside !sqd->thread?

This is narrower than it looks. The stop bit is only set once the last
ring has dropped the sqd, at which point there's no fd left to attach
through. So the only exit that can be caught is the one on a fatal
signal, and since that's group wide and attaching requires the same
thread group, the attacher is also dying. That window isn't new, between
an attach and the thread noticing the signal, which is what -EOWNERDEAD
is there for.

But no reason not to make the check see it. I've queued up the below,
what do you think? It sets IO_SQ_THREAD_SHOULD_STOP in io_sq_thread()
before it cancels, and tests that bit in io_sq_offload_create()
alongside sqd->thread. Then no new state needed.

diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
index 49c52284ecdd..c833e776320d 100644
--- a/io_uring/sqpoll.c
+++ b/io_uring/sqpoll.c
@@ -433,6 +433,12 @@ static int io_sq_thread(void *data)
 	if (io_sq_tw_pending())
 		io_sq_tw(UINT_MAX);
 
+	/*
+	 * Cancelation drops sqd->lock while it waits, and a ring could attach
+	 * to a thread that is on its way out. A fatal signal doesn't set the
+	 * bit, so set it here for io_sq_offload_create() to see.
+	 */
+	set_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state);
 	io_uring_cancel_generic(true, sqd);
 	rcu_assign_pointer(sqd->thread, NULL);
 	put_task_struct(current);
@@ -502,7 +508,10 @@ __cold int io_sq_offload_create(struct io_ring_ctx *ctx,
 		list_add(&ctx->sqd_list, &sqd->ctx_list);
 		io_sqd_update_thread_idle(sqd);
 		/* don't attach to a dying SQPOLL thread, would be racy */
-		ret = (attached && !sqd->thread) ? -ENXIO : 0;
+		ret = 0;
+		if (attached && (!sqd->thread ||
+		    test_bit(IO_SQ_THREAD_SHOULD_STOP, &sqd->state)))
+			ret = -ENXIO;
 		io_sq_thread_unpark(sqd);
 
 		if (ret < 0)

-- 
Jens Axboe