[PATCH] io_uring: fix incorrect io_kiocb reference in io_link_skb

[PATCH] io_uring: fix incorrect io_kiocb reference in io_link_skb

[Yang Xiuwei]

From: Yang Xiuwei <yangxiuwei@kylinos.cn>

In io_link_skb function, there is a bug where prev_notif is incorrectly
assigned using 'nd' instead of 'prev_nd'. This causes the context
validation check to compare the current notification with itself instead
of comparing it with the previous notification.

Fix by using the correct prev_nd parameter when obtaining prev_notif.

Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>

diff --git a/io_uring/notif.c b/io_uring/notif.c
index 9a6f6e92d742..ea9c0116cec2 100644
--- a/io_uring/notif.c
+++ b/io_uring/notif.c
@@ -85,7 +85,7 @@ static int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg)
 		return -EEXIST;
 
 	prev_nd = container_of(prev_uarg, struct io_notif_data, uarg);
-	prev_notif = cmd_to_io_kiocb(nd);
+	prev_notif = cmd_to_io_kiocb(prev_nd);
 
 	/* make sure all noifications can be finished in the same task_work */
 	if (unlikely(notif->ctx != prev_notif->ctx ||
-- 
2.25.1

Re: [PATCH] io_uring: fix incorrect io_kiocb reference in io_link_skb

[Pavel Begunkov]

On 9/19/25 10:03, Yang Xiuwei wrote:
> From: Yang Xiuwei <yangxiuwei@kylinos.cn>
> 
> In io_link_skb function, there is a bug where prev_notif is incorrectly
> assigned using 'nd' instead of 'prev_nd'. This causes the context
> validation check to compare the current notification with itself instead
> of comparing it with the previous notification.
> 
> Fix by using the correct prev_nd parameter when obtaining prev_notif.

Good catch,

Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
Fixes: 6fe4220912d19 ("io_uring/notif: implement notification stacking")
Cc: stable@vger.kernel.org


> Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
> 
> diff --git a/io_uring/notif.c b/io_uring/notif.c
> index 9a6f6e92d742..ea9c0116cec2 100644
> --- a/io_uring/notif.c
> +++ b/io_uring/notif.c
> @@ -85,7 +85,7 @@ static int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg)
>   		return -EEXIST;
>   
>   	prev_nd = container_of(prev_uarg, struct io_notif_data, uarg);
> -	prev_notif = cmd_to_io_kiocb(nd);
> +	prev_notif = cmd_to_io_kiocb(prev_nd);
>   
>   	/* make sure all noifications can be finished in the same task_work */
>   	if (unlikely(notif->ctx != prev_notif->ctx ||

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix incorrect io_kiocb reference in io_link_skb

[David Kahurani]

On Fri, Sep 19, 2025 at 2:23 PM David Kahurani <k.kahurani@gmail.com> wrote:
>
> This is something unrelated but just bringing it up because it is in the same locality.
>
> It doesn't seem like the references(uarg->refcnt) are well accounted for io_notif_data. Any node that gets passed to 'io_tx_ubuf_complete' will gets its refcnt decremented but assuming there's a list of nodes, some of the nodes in the list will not get their reference count decremented and that will trigger the lockdep_assert in 'io_notif_tw_complete'
>
> It doesn't look that this will have any consequences beyond triggering the lockderp_assert, though.
>
> Maybe my analysis is wrong?
>
>
> On Fri, Sep 19, 2025 at 2:16 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> On 9/19/25 10:03, Yang Xiuwei wrote:
>> > From: Yang Xiuwei <yangxiuwei@kylinos.cn>
>> >
>> > In io_link_skb function, there is a bug where prev_notif is incorrectly
>> > assigned using 'nd' instead of 'prev_nd'. This causes the context
>> > validation check to compare the current notification with itself instead
>> > of comparing it with the previous notification.
>> >
>> > Fix by using the correct prev_nd parameter when obtaining prev_notif.
>>
>> Good catch,
>>
>> Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
>> Fixes: 6fe4220912d19 ("io_uring/notif: implement notification stacking")
>> Cc: stable@vger.kernel.org
>>
>>
>> > Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
>> >
>> > diff --git a/io_uring/notif.c b/io_uring/notif.c
>> > index 9a6f6e92d742..ea9c0116cec2 100644
>> > --- a/io_uring/notif.c
>> > +++ b/io_uring/notif.c
>> > @@ -85,7 +85,7 @@ static int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg)
>> >               return -EEXIST;
>> >
>> >       prev_nd = container_of(prev_uarg, struct io_notif_data, uarg);
>> > -     prev_notif = cmd_to_io_kiocb(nd);
>> > +     prev_notif = cmd_to_io_kiocb(prev_nd);
>> >
>> >       /* make sure all noifications can be finished in the same task_work */
>> >       if (unlikely(notif->ctx != prev_notif->ctx ||
>>
>> --
>> Pavel Begunkov
>>
>>

This is something unrelated but just bringing it up because it is in
the same locality.

It doesn't seem like the references(uarg->refcnt) are well accounted
for io_notif_data. Any node that gets passed to 'io_tx_ubuf_complete'
will gets it's refcnt decremented but assuming there's a list of
nodes, some of the nodes in the list will not get their reference
count decremented and that will trigger the lockdep_assert in
'io_notif_tw_complete'

It doesn't look that this will have any consequences beyond triggering
the lockderp_assert, though.

Re: [PATCH] io_uring: fix incorrect io_kiocb reference in io_link_skb

[Jens Axboe]

On Fri, 19 Sep 2025 17:03:52 +0800, Yang Xiuwei wrote:
> In io_link_skb function, there is a bug where prev_notif is incorrectly
> assigned using 'nd' instead of 'prev_nd'. This causes the context
> validation check to compare the current notification with itself instead
> of comparing it with the previous notification.
> 
> Fix by using the correct prev_nd parameter when obtaining prev_notif.
> 
> [...]

Applied, thanks!

[1/1] io_uring: fix incorrect io_kiocb reference in io_link_skb
      commit: 2c139a47eff8de24e3350dadb4c9d5e3426db826

Best regards,
-- 
Jens Axboe

Re: [PATCH] io_uring: fix incorrect io_kiocb reference in io_link_skb

[Pavel Begunkov]

On 9/19/25 12:25, David Kahurani wrote:
...>>>> Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
>>>>
>>>> diff --git a/io_uring/notif.c b/io_uring/notif.c
>>>> index 9a6f6e92d742..ea9c0116cec2 100644
>>>> --- a/io_uring/notif.c
>>>> +++ b/io_uring/notif.c
>>>> @@ -85,7 +85,7 @@ static int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg)
>>>>                return -EEXIST;
>>>>
>>>>        prev_nd = container_of(prev_uarg, struct io_notif_data, uarg);
>>>> -     prev_notif = cmd_to_io_kiocb(nd);
>>>> +     prev_notif = cmd_to_io_kiocb(prev_nd);
>>>>
>>>>        /* make sure all noifications can be finished in the same task_work */
>>>>        if (unlikely(notif->ctx != prev_notif->ctx ||
>>>
>>> --
>>> Pavel Begunkov
>>>
>>>
> 
> This is something unrelated but just bringing it up because it is in
> the same locality.
> 
> It doesn't seem like the references(uarg->refcnt) are well accounted
> for io_notif_data. Any node that gets passed to 'io_tx_ubuf_complete'
> will gets it's refcnt decremented but assuming there's a list of
> nodes, some of the nodes in the list will not get their reference
> count decremented and 

And not supposed to. Children reference the head, and the head dies
last.

> that will trigger the lockdep_assert in
> 'io_notif_tw_complete'

Did you see it trigger? If so, please attach the warning splat.

> It doesn't look that this will have any consequences beyond triggering
> the lockderp_assert, though.

-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix incorrect io_kiocb reference in io_link_skb

[David Kahurani]

On Fri, Sep 19, 2025 at 5:14 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 9/19/25 12:25, David Kahurani wrote:
> ...>>>> Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
> >>>>
> >>>> diff --git a/io_uring/notif.c b/io_uring/notif.c
> >>>> index 9a6f6e92d742..ea9c0116cec2 100644
> >>>> --- a/io_uring/notif.c
> >>>> +++ b/io_uring/notif.c
> >>>> @@ -85,7 +85,7 @@ static int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg)
> >>>>                return -EEXIST;
> >>>>
> >>>>        prev_nd = container_of(prev_uarg, struct io_notif_data, uarg);
> >>>> -     prev_notif = cmd_to_io_kiocb(nd);
> >>>> +     prev_notif = cmd_to_io_kiocb(prev_nd);
> >>>>
> >>>>        /* make sure all noifications can be finished in the same task_work */
> >>>>        if (unlikely(notif->ctx != prev_notif->ctx ||
> >>>
> >>> --
> >>> Pavel Begunkov
> >>>
> >>>
> >
> > This is something unrelated but just bringing it up because it is in
> > the same locality.
> >
> > It doesn't seem like the references(uarg->refcnt) are well accounted
> > for io_notif_data. Any node that gets passed to 'io_tx_ubuf_complete'
> > will gets it's refcnt decremented but assuming there's a list of
> > nodes, some of the nodes in the list will not get their reference
> > count decremented and
>
> And not supposed to. Children reference the head, and the head dies
> last.

I am not sure about the mechanics of this. This is only based on
analysing the code but it seems, if a child node gets completed, it
will pull all the other nodes in that link by jumping to the head
node. But, I trust that you know better :-)

What do you mean it's not supposed to? All the nodes eventually go
through 'io_notif_tw_complete' to be queued back into request queues,
if any nodes whose reference was not handled(all nodes get a reference
of 1 at allocation) goes through the method, then the warning will
trigger.

>
> > that will trigger the lockdep_assert in
> > 'io_notif_tw_complete'
>
> Did you see it trigger? If so, please attach the warning splat.
>
> > It doesn't look that this will have any consequences beyond triggering
> > the lockderp_assert, though.
>
> --
> Pavel Begunkov
>

Re: [PATCH] io_uring: fix incorrect io_kiocb reference in io_link_skb

[Pavel Begunkov]

On 9/19/25 15:28, David Kahurani wrote:
> On Fri, Sep 19, 2025 at 5:14 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> On 9/19/25 12:25, David Kahurani wrote:
>> ...>>>> Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
>>>>>>
>>>>>> diff --git a/io_uring/notif.c b/io_uring/notif.c
>>>>>> index 9a6f6e92d742..ea9c0116cec2 100644
>>>>>> --- a/io_uring/notif.c
>>>>>> +++ b/io_uring/notif.c
>>>>>> @@ -85,7 +85,7 @@ static int io_link_skb(struct sk_buff *skb, struct ubuf_info *uarg)
>>>>>>                 return -EEXIST;
>>>>>>
>>>>>>         prev_nd = container_of(prev_uarg, struct io_notif_data, uarg);
>>>>>> -     prev_notif = cmd_to_io_kiocb(nd);
>>>>>> +     prev_notif = cmd_to_io_kiocb(prev_nd);
>>>>>>
>>>>>>         /* make sure all noifications can be finished in the same task_work */
>>>>>>         if (unlikely(notif->ctx != prev_notif->ctx ||
>>>>>
>>>>> --
>>>>> Pavel Begunkov
>>>>>
>>>>>
>>>
>>> This is something unrelated but just bringing it up because it is in
>>> the same locality.
>>>
>>> It doesn't seem like the references(uarg->refcnt) are well accounted
>>> for io_notif_data. Any node that gets passed to 'io_tx_ubuf_complete'
>>> will gets it's refcnt decremented but assuming there's a list of
>>> nodes, some of the nodes in the list will not get their reference
>>> count decremented and
>>
>> And not supposed to. Children reference the head, and the head dies
>> last.
> 
> I am not sure about the mechanics of this. This is only based on
> analysing the code but it seems, if a child node gets completed, it
> will pull all the other nodes in that link by jumping to the head

It'll put its reference to the head, but nothing is going to
be destroyed until the head refs hit 0.

> node. But, I trust that you know better :-)
> 
> What do you mean it's not supposed to? All the nodes eventually go

I was saying that the head isn't supposed to put the children's
references, it goes the other way around. Children have refs to
head, and everything is destroyed once the head is put down.

> through 'io_notif_tw_complete' to be queued back into request queues,
> if any nodes whose reference was not handled(all nodes get a reference
> of 1 at allocation) goes through the method, then the warning will
> trigger.
-- 
Pavel Begunkov

Re: [PATCH] io_uring: fix incorrect io_kiocb reference in io_link_skb

[David Kahurani]

On 9/22/25 10:52, Pavel Begunkov wrote:
> On 9/19/25 15:28, David Kahurani wrote:
>> On Fri, Sep 19, 2025 at 5:14 PM Pavel Begunkov 
>> <asml.silence@gmail.com> wrote:
>>>
>>> On 9/19/25 12:25, David Kahurani wrote:
>>> ...>>>> Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
>>>>>>>
>>>>>>> diff --git a/io_uring/notif.c b/io_uring/notif.c
>>>>>>> index 9a6f6e92d742..ea9c0116cec2 100644
>>>>>>> --- a/io_uring/notif.c
>>>>>>> +++ b/io_uring/notif.c
>>>>>>> @@ -85,7 +85,7 @@ static int io_link_skb(struct sk_buff *skb, 
>>>>>>> struct ubuf_info *uarg)
>>>>>>>                 return -EEXIST;
>>>>>>>
>>>>>>>         prev_nd = container_of(prev_uarg, struct io_notif_data, 
>>>>>>> uarg);
>>>>>>> -     prev_notif = cmd_to_io_kiocb(nd);
>>>>>>> +     prev_notif = cmd_to_io_kiocb(prev_nd);
>>>>>>>
>>>>>>>         /* make sure all noifications can be finished in the 
>>>>>>> same task_work */
>>>>>>>         if (unlikely(notif->ctx != prev_notif->ctx ||
>>>>>>
>>>>>> -- 
>>>>>> Pavel Begunkov
>>>>>>
>>>>>>
>>>>
>>>> This is something unrelated but just bringing it up because it is in
>>>> the same locality.
>>>>
>>>> It doesn't seem like the references(uarg->refcnt) are well accounted
>>>> for io_notif_data. Any node that gets passed to 'io_tx_ubuf_complete'
>>>> will gets it's refcnt decremented but assuming there's a list of
>>>> nodes, some of the nodes in the list will not get their reference
>>>> count decremented and
>>>
>>> And not supposed to. Children reference the head, and the head dies
>>> last.
>>
>> I am not sure about the mechanics of this. This is only based on
>> analysing the code but it seems, if a child node gets completed, it
>> will pull all the other nodes in that link by jumping to the head
>
> It'll put its reference to the head, but nothing is going to
> be destroyed until the head refs hit 0.


I take that to mean there's some code elsewhere that also interacts with 
these references otherwise just based on this code, it seems like notifs 
always have a reference of 1

Because I don't have a stacktrace, I will leave it that.


>
>> node. But, I trust that you know better :-)
>>
>> What do you mean it's not supposed to? All the nodes eventually go
>
> I was saying that the head isn't supposed to put the children's
> references, it goes the other way around. Children have refs to
> head, and everything is destroyed once the head is put down.
>
>> through 'io_notif_tw_complete' to be queued back into request queues,
>> if any nodes whose reference was not handled(all nodes get a reference
>> of 1 at allocation) goes through the method, then the warning will
>> trigger.