Re: [PATCH] io_uring: use cpu_online() to check p->sq_thread_cpu instead of cpu_possible()

[Jens Axboe <axboe@kernel.dk>]

On 5/1/19 8:32 AM, Jeff Moyer wrote:
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> On 5/1/19 5:56 AM, Jeff Moyer wrote:
>>> Shenghui Wang <shhuiw@foxmail.com> writes:
>>>
>>>> This issue is found by running liburing/test/io_uring_setup test.
>>>>
>>>> When test run, the testcase "attempt to bind to invalid cpu" would not
>>>> pass with messages like:
>>>>    io_uring_setup(1, 0xbfc2f7c8), \
>>>> flags: IORING_SETUP_SQPOLL|IORING_SETUP_SQ_AFF, \
>>>> resv: 0x00000000 0x00000000 0x00000000 0x00000000 0x00000000, \
>>>> sq_thread_cpu: 2
>>>>    expected -1, got 3
>>>>    FAIL
>>>>
>>>> On my system, there is:
>>>>    CPU(s) possible : 0-3
>>>>    CPU(s) online   : 0-1
>>>>    CPU(s) offline  : 2-3
>>>>    CPU(s) present  : 0-1
>>>>
>>>> The sq_thread_cpu 2 is offline on my system, so the bind should fail.
>>>> But cpu_possible() will pass the check. We shouldn't be able to bind
>>>> to an offline cpu. Use cpu_online() to do the check.
>>>>
>>>> After the change, the testcase run as expected: EINVAL will be returned
>>>> for cpu offlined.
>>>>
>>>> Signed-off-by: Shenghui Wang <shhuiw@foxmail.com>
>>>> ---
>>>>  fs/io_uring.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>> index 0e9fb2cb1984..aa3d39860a1c 100644
>>>> --- a/fs/io_uring.c
>>>> +++ b/fs/io_uring.c
>>>> @@ -2241,7 +2241,7 @@ static int io_sq_offload_start(struct io_ring_ctx *ctx,
>>>>  	ctx->sqo_mm = current->mm;
>>>>  
>>>>  	ret = -EINVAL;
>>>> -	if (!cpu_possible(p->sq_thread_cpu))
>>>> +	if (!cpu_online(p->sq_thread_cpu))
>>>>  		goto err;
>>>>  
>>>>  	if (ctx->flags & IORING_SETUP_SQPOLL) {
>>>> @@ -2258,7 +2258,7 @@ static int io_sq_offload_start(struct io_ring_ctx *ctx,
>>>>  
>>>>  			cpu = array_index_nospec(p->sq_thread_cpu, NR_CPUS);
>>>>  			ret = -EINVAL;
>>>> -			if (!cpu_possible(p->sq_thread_cpu))
>>>> +			if (!cpu_online(p->sq_thread_cpu))
>>>>  				goto err;
>>>>  
>>>>  			ctx->sqo_thread = kthread_create_on_cpu(io_sq_thread,
>>>
>>> Hmm.  Why are we doing this check twice?  Oh... Jens, I think you
>>> braino'd commit 917257daa0fea.  Have a look.  You probably wanted to get
>>> rid of the first check for cpu_possible.
>>
>> Added a fixup patch the other day:
>>
>> http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=362bf8670efccebca22efda1ee5a5ee831ec5efb
> 
> @@ -2333,13 +2329,14 @@ static int io_sq_offload_start(struct io_ring_ctx *ctx,
>  			ctx->sq_thread_idle = HZ;
>  
>  		if (p->flags & IORING_SETUP_SQ_AFF) {
> -			int cpu;
> +			int cpu = p->sq_thread_cpu;
>  
> -			cpu = array_index_nospec(p->sq_thread_cpu, NR_CPUS);
>  			ret = -EINVAL;
> -			if (!cpu_possible(p->sq_thread_cpu))
> +			if (cpu >= nr_cpu_ids || !cpu_possible(cpu))
>  				goto err;
>  
> +			cpu = array_index_nospec(cpu, nr_cpu_ids);
> +
> 
> Why do you do the array_index_nospec last?  Why wouldn't that be written
> as:
> 
> 	if (p->flags & IORING_SETUP_SQ_AFF) {
> 		int cpu = array_index_nospec(p->sq_thread_cpu, nr_cpu_ids);
> 
> 		ret = -EINVAL;
> 		if (!cpu_possible(cpu))
> 			goto err;
> 
> 		ctx->sqo_thread = kthread_create_on_cpu(io_sq_thread,
> 						ctx, cpu,
> 						"io_uring-sq");
> 	} else {
> ...
> 
> That would take away some head-scratching for me.

Agree, I've cleaned it up, it was a bit of a mess.

-- 
Jens Axboe