[PATCH] io_uring: fix invalid handler for double apoll

[PATCH] io_uring: fix invalid handler for double apoll

[Pavel Begunkov]

io_poll_double_wake() is called for both: poll requests and as apoll
(internal poll to make rw and other requests), hence when it calls
__io_async_wake() it should use a right callback depending on the
current poll type.

Cc: stable@vger.kernel.org # v5.8+
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 20781e939940..b2d72bd18fcf 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4927,6 +4927,8 @@ static void io_poll_task_func(struct callback_head *cb)
 	percpu_ref_put(&ctx->refs);
 }
 
+static void io_async_task_func(struct callback_head *cb);
+
 static int io_poll_double_wake(struct wait_queue_entry *wait, unsigned mode,
 			       int sync, void *key)
 {
@@ -4950,8 +4952,12 @@ static int io_poll_double_wake(struct wait_queue_entry *wait, unsigned mode,
 		/* make sure double remove sees this as being gone */
 		wait->private = NULL;
 		spin_unlock(&poll->head->lock);
-		if (!done)
-			__io_async_wake(req, poll, mask, io_poll_task_func);
+		if (!done) {
+			if (req->opcode == IORING_OP_POLL_ADD)
+				__io_async_wake(req, poll, mask, io_poll_task_func);
+			else
+				__io_async_wake(req, poll, mask, io_async_task_func);
+		}
 	}
 	refcount_dec(&req->refs);
 	return 1;
-- 
2.24.0

Re: [PATCH] io_uring: fix invalid handler for double apoll

[Jens Axboe]

On 10/25/20 8:26 AM, Pavel Begunkov wrote:
> io_poll_double_wake() is called for both: poll requests and as apoll
> (internal poll to make rw and other requests), hence when it calls
> __io_async_wake() it should use a right callback depending on the
> current poll type.

Can we do something like this instead? Untested...


diff --git a/fs/io_uring.c b/fs/io_uring.c
index b42dfa0243bf..a0147c0e5320 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4978,7 +4978,7 @@ static int io_poll_double_wake(struct wait_queue_entry *wait, unsigned mode,
 		wait->private = NULL;
 		spin_unlock(&poll->head->lock);
 		if (!done)
-			__io_async_wake(req, poll, mask, io_poll_task_func);
+			poll->wait.func(wait, mode, sync, key);
 	}
 	refcount_dec(&req->refs);
 	return 1;

-- 
Jens Axboe

Re: [PATCH] io_uring: fix invalid handler for double apoll

[Pavel Begunkov]

On 25/10/2020 15:53, Jens Axboe wrote:
> On 10/25/20 8:26 AM, Pavel Begunkov wrote:
>> io_poll_double_wake() is called for both: poll requests and as apoll
>> (internal poll to make rw and other requests), hence when it calls
>> __io_async_wake() it should use a right callback depending on the
>> current poll type.
> 
> Can we do something like this instead? Untested...

It should work, but looks less comprehensible. Though, it'll need
some refactoring in the future either way.

> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index b42dfa0243bf..a0147c0e5320 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -4978,7 +4978,7 @@ static int io_poll_double_wake(struct wait_queue_entry *wait, unsigned mode,
>  		wait->private = NULL;
>  		spin_unlock(&poll->head->lock);
>  		if (!done)
> -			__io_async_wake(req, poll, mask, io_poll_task_func);
> +			poll->wait.func(wait, mode, sync, key);
>  	}
>  	refcount_dec(&req->refs);
>  	return 1;
> 

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix invalid handler for double apoll

[Jens Axboe]

On 10/25/20 10:24 AM, Pavel Begunkov wrote:
> On 25/10/2020 15:53, Jens Axboe wrote:
>> On 10/25/20 8:26 AM, Pavel Begunkov wrote:
>>> io_poll_double_wake() is called for both: poll requests and as apoll
>>> (internal poll to make rw and other requests), hence when it calls
>>> __io_async_wake() it should use a right callback depending on the
>>> current poll type.
>>
>> Can we do something like this instead? Untested...
> 
> It should work, but looks less comprehensible. Though, it'll need

Not sure I agree, with a comment it'd be nicer imho:

/* call appropriate handler for this request type */
poll->wait.func(wait, mode, sync, key);

instead of having to manually dig at the opcode to figure out which one
to use.

-- 
Jens Axboe

Re: [PATCH] io_uring: fix invalid handler for double apoll

[Pavel Begunkov]

On 25/10/2020 18:42, Jens Axboe wrote:
> On 10/25/20 10:24 AM, Pavel Begunkov wrote:
>> On 25/10/2020 15:53, Jens Axboe wrote:
>>> On 10/25/20 8:26 AM, Pavel Begunkov wrote:
>>>> io_poll_double_wake() is called for both: poll requests and as apoll
>>>> (internal poll to make rw and other requests), hence when it calls
>>>> __io_async_wake() it should use a right callback depending on the
>>>> current poll type.
>>>
>>> Can we do something like this instead? Untested...
>>
>> It should work, but looks less comprehensible. Though, it'll need
> 
> Not sure I agree, with a comment it'd be nicer im ho:

I don't really care enough to start a bikeshedding, let's get yours
tested and merged.

> 
> /* call appropriate handler for this request type */
> poll->wait.func(wait, mode, sync, key);
> 
> instead of having to manually dig at the opcode to figure out which one
> to use.
> 

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix invalid handler for double apoll

[Jens Axboe]

On 10/25/20 1:01 PM, Pavel Begunkov wrote:
> On 25/10/2020 18:42, Jens Axboe wrote:
>> On 10/25/20 10:24 AM, Pavel Begunkov wrote:
>>> On 25/10/2020 15:53, Jens Axboe wrote:
>>>> On 10/25/20 8:26 AM, Pavel Begunkov wrote:
>>>>> io_poll_double_wake() is called for both: poll requests and as apoll
>>>>> (internal poll to make rw and other requests), hence when it calls
>>>>> __io_async_wake() it should use a right callback depending on the
>>>>> current poll type.
>>>>
>>>> Can we do something like this instead? Untested...
>>>
>>> It should work, but looks less comprehensible. Though, it'll need
>>
>> Not sure I agree, with a comment it'd be nicer im ho:
> 
> I don't really care enough to start a bikeshedding, let's get yours
> tested and merged.

Not really bikeshedding I think, we're not debating names of
functions :-)

My approach would need conditional clearing of ->private as well,
as far as I can tell. I'll give it a spin.

-- 
Jens Axboe

Re: [PATCH] io_uring: fix invalid handler for double apoll

[Pavel Begunkov]

On 25/10/2020 19:18, Jens Axboe wrote:
> On 10/25/20 1:01 PM, Pavel Begunkov wrote:
>> On 25/10/2020 18:42, Jens Axboe wrote:
>>> On 10/25/20 10:24 AM, Pavel Begunkov wrote:
>>>> On 25/10/2020 15:53, Jens Axboe wrote:
>>>>> On 10/25/20 8:26 AM, Pavel Begunkov wrote:
>>>>>> io_poll_double_wake() is called for both: poll requests and as apoll
>>>>>> (internal poll to make rw and other requests), hence when it calls
>>>>>> __io_async_wake() it should use a right callback depending on the
>>>>>> current poll type.
>>>>>
>>>>> Can we do something like this instead? Untested...
>>>>
>>>> It should work, but looks less comprehensible. Though, it'll need
>>>
>>> Not sure I agree, with a comment it'd be nicer im ho:
>>
>> I don't really care enough to start a bikeshedding, let's get yours
>> tested and merged.
> 
> Not really bikeshedding I think, we're not debating names of
> functions :-)

It's just not so important, and it even may get removed in a month,
who knows.

> 
> My approach would need conditional clearing of ->private as well,
> as far as I can tell. I'll give it a spin.

Maybe

- poll->wait.func(wait, mode, sync, key);
+ poll->wait.func(&poll->wait, mode, sync, key);

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix invalid handler for double apoll

[Jens Axboe]

On 10/25/20 1:32 PM, Pavel Begunkov wrote:
> On 25/10/2020 19:18, Jens Axboe wrote:
>> On 10/25/20 1:01 PM, Pavel Begunkov wrote:
>>> On 25/10/2020 18:42, Jens Axboe wrote:
>>>> On 10/25/20 10:24 AM, Pavel Begunkov wrote:
>>>>> On 25/10/2020 15:53, Jens Axboe wrote:
>>>>>> On 10/25/20 8:26 AM, Pavel Begunkov wrote:
>>>>>>> io_poll_double_wake() is called for both: poll requests and as apoll
>>>>>>> (internal poll to make rw and other requests), hence when it calls
>>>>>>> __io_async_wake() it should use a right callback depending on the
>>>>>>> current poll type.
>>>>>>
>>>>>> Can we do something like this instead? Untested...
>>>>>
>>>>> It should work, but looks less comprehensible. Though, it'll need
>>>>
>>>> Not sure I agree, with a comment it'd be nicer im ho:
>>>
>>> I don't really care enough to start a bikeshedding, let's get yours
>>> tested and merged.
>>
>> Not really bikeshedding I think, we're not debating names of
>> functions :-)
> 
> It's just not so important, and it even may get removed in a month,
> who knows.

Well it might not or it might take longer, still nice to have the
simplest fix...

>> My approach would need conditional clearing of ->private as well,
>> as far as I can tell. I'll give it a spin.
> 
> Maybe
> 
> - poll->wait.func(wait, mode, sync, key);
> + poll->wait.func(&poll->wait, mode, sync, key);

Ah yeah, that looks better.

-- 
Jens Axboe

Re: [PATCH] io_uring: fix invalid handler for double apoll

[Pavel Begunkov]

On 25/10/2020 19:44, Jens Axboe wrote:
> On 10/25/20 1:32 PM, Pavel Begunkov wrote:
>> On 25/10/2020 19:18, Jens Axboe wrote:
>>> On 10/25/20 1:01 PM, Pavel Begunkov wrote:
>>>> On 25/10/2020 18:42, Jens Axboe wrote:
>>>>> On 10/25/20 10:24 AM, Pavel Begunkov wrote:
>>>>>> On 25/10/2020 15:53, Jens Axboe wrote:
>>>>>>> On 10/25/20 8:26 AM, Pavel Begunkov wrote:
>>>>>>>> io_poll_double_wake() is called for both: poll requests and as apoll
>>>>>>>> (internal poll to make rw and other requests), hence when it calls
>>>>>>>> __io_async_wake() it should use a right callback depending on the
>>>>>>>> current poll type.
>>>>>>>
>>>>>>> Can we do something like this instead? Untested...
>>>>>>
>>>>>> It should work, but looks less comprehensible. Though, it'll need
>>>>>
>>>>> Not sure I agree, with a comment it'd be nicer im ho:
>>>>
>>>> I don't really care enough to start a bikeshedding, let's get yours
>>>> tested and merged.
>>>
>>> Not really bikeshedding I think, we're not debating names of
>>> functions :-)
>>
>> It's just not so important, and it even may get removed in a month,
>> who knows.
> 
> Well it might not or it might take longer, still nice to have the
> simplest fix...

Ok, then TLDR: my reasoning is that with poll->wait.func(), a person
who reads it needs to
a) go look up what's poll (assigned depending on the opcode),
b) then find out which wait.func callback it set. And it's not
as easy to associate __io_arm_poll_handler() call sites with cases
if you haven't seen this code before.
c) then go through io_{async,poll}_wake to finally find out that they
do almost the same thing, that's calling __io_async_wake().

I'm familiar with the code structure, but IMHO it's harder to
understand, if I'd be looking for the first time.

> 
>>> My approach would need conditional clearing of ->private as well,
>>> as far as I can tell. I'll give it a spin.
>>
>> Maybe
>>
>> - poll->wait.func(wait, mode, sync, key);
>> + poll->wait.func(&poll->wait, mode, sync, key);
> 
> Ah yeah, that looks better.
> 

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix invalid handler for double apoll

[Jens Axboe]

On 10/25/20 1:54 PM, Pavel Begunkov wrote:
> On 25/10/2020 19:44, Jens Axboe wrote:
>> On 10/25/20 1:32 PM, Pavel Begunkov wrote:
>>> On 25/10/2020 19:18, Jens Axboe wrote:
>>>> On 10/25/20 1:01 PM, Pavel Begunkov wrote:
>>>>> On 25/10/2020 18:42, Jens Axboe wrote:
>>>>>> On 10/25/20 10:24 AM, Pavel Begunkov wrote:
>>>>>>> On 25/10/2020 15:53, Jens Axboe wrote:
>>>>>>>> On 10/25/20 8:26 AM, Pavel Begunkov wrote:
>>>>>>>>> io_poll_double_wake() is called for both: poll requests and as apoll
>>>>>>>>> (internal poll to make rw and other requests), hence when it calls
>>>>>>>>> __io_async_wake() it should use a right callback depending on the
>>>>>>>>> current poll type.
>>>>>>>>
>>>>>>>> Can we do something like this instead? Untested...
>>>>>>>
>>>>>>> It should work, but looks less comprehensible. Though, it'll need
>>>>>>
>>>>>> Not sure I agree, with a comment it'd be nicer im ho:
>>>>>
>>>>> I don't really care enough to start a bikeshedding, let's get yours
>>>>> tested and merged.
>>>>
>>>> Not really bikeshedding I think, we're not debating names of
>>>> functions :-)
>>>
>>> It's just not so important, and it even may get removed in a month,
>>> who knows.
>>
>> Well it might not or it might take longer, still nice to have the
>> simplest fix...
> 
> Ok, then TLDR: my reasoning is that with poll->wait.func(), a person
> who reads it needs to
> a) go look up what's poll (assigned depending on the opcode),
> b) then find out which wait.func callback it set. And it's not
> as easy to associate __io_arm_poll_handler() call sites with cases
> if you haven't seen this code before.
> c) then go through io_{async,poll}_wake to finally find out that they
> do almost the same thing, that's calling __io_async_wake().
> 
> I'm familiar with the code structure, but IMHO it's harder to
> understand, if I'd be looking for the first time.

I guess we'll just have to agree to disagree on that one. Yes, you'd
have to find where it's assigned to see what happens, which is a
downside. The benefit would be that even if these things change in the
future, the callback func would always be right, so you'd never have to
touch that code again. c) is the same for both.

-- 
Jens Axboe