On 5/1/19 8:32 AM, Jeff Moyer wrote:
> Jens Axboe <axboe@xxxxxxxxx> writes:
>
>> On 5/1/19 5:56 AM, Jeff Moyer wrote:
>>> Shenghui Wang <shhuiw@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>>>> ---
>>>> 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
