All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: drop skbs in napi->rx_list when removing the napi context.
@ 2021-08-11 23:59 ` Nguyen Dinh Phi
  0 siblings, 0 replies; 14+ messages in thread
From: Nguyen Dinh Phi @ 2021-08-11 23:59 UTC (permalink / raw)
  To: davem, kuba, ast, daniel, hawk, john.fastabend, andrii, kafai,
	songliubraving, yhs, kpsingh, edumazet, atenart, alobakin,
	weiwan, ap420073, bjorn, memxor
  Cc: Nguyen Dinh Phi, netdev, linux-kernel, bpf, linux-kernel-mentees,
	syzbot+989efe781c74de1ddb54

The napi->rx_list is used to hold the GRO_NORMAL skbs before passing
them to the stack, these skbs only passed to stack at the flush time or
when the list's weight matches the predefined condition. In case the
rx_list contains pending skbs when we remove the napi context, we need
to clean out this list, otherwise, a memory leak will happen.

Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
Reported-by: syzbot+989efe781c74de1ddb54@syzkaller.appspotmail.com
---
 net/core/dev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index b51e41d0a7fe..319fffc62ce6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7038,6 +7038,13 @@ void __netif_napi_del(struct napi_struct *napi)
 	list_del_rcu(&napi->dev_list);
 	napi_free_frags(napi);

+	if (napi->rx_count) {
+		struct sk_buff *skb, *n;
+
+		list_for_each_entry_safe(skb, n, &napi->rx_list, list)
+			kfree_skb(skb);
+	}
+
 	flush_gro_hash(napi);
 	napi->gro_bitmask = 0;

--
2.25.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH] net: drop skbs in napi->rx_list when removing the napi context.
@ 2021-08-11 23:59 ` Nguyen Dinh Phi
  0 siblings, 0 replies; 14+ messages in thread
From: Nguyen Dinh Phi @ 2021-08-11 23:59 UTC (permalink / raw)
  To: davem, kuba, ast, daniel, hawk, john.fastabend, andrii, kafai,
	songliubraving, yhs, kpsingh, edumazet, atenart, alobakin,
	weiwan, ap420073, bjorn, memxor
  Cc: netdev, syzbot+989efe781c74de1ddb54, linux-kernel, bpf,
	linux-kernel-mentees

The napi->rx_list is used to hold the GRO_NORMAL skbs before passing
them to the stack, these skbs only passed to stack at the flush time or
when the list's weight matches the predefined condition. In case the
rx_list contains pending skbs when we remove the napi context, we need
to clean out this list, otherwise, a memory leak will happen.

Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
Reported-by: syzbot+989efe781c74de1ddb54@syzkaller.appspotmail.com
---
 net/core/dev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index b51e41d0a7fe..319fffc62ce6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7038,6 +7038,13 @@ void __netif_napi_del(struct napi_struct *napi)
 	list_del_rcu(&napi->dev_list);
 	napi_free_frags(napi);

+	if (napi->rx_count) {
+		struct sk_buff *skb, *n;
+
+		list_for_each_entry_safe(skb, n, &napi->rx_list, list)
+			kfree_skb(skb);
+	}
+
 	flush_gro_hash(napi);
 	napi->gro_bitmask = 0;

--
2.25.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: drop skbs in napi->rx_list when removing the napi context.
  2021-08-11 23:59 ` Nguyen Dinh Phi
  (?)
@ 2021-08-12  4:50 ` Hillf Danton
  2021-08-12  5:19     ` syzbot
  -1 siblings, 1 reply; 14+ messages in thread
From: Hillf Danton @ 2021-08-12  4:50 UTC (permalink / raw)
  To: Nguyen Dinh Phi
  Cc: syzkaller-bugs, netdev, linux-kernel-mentees, linux-kernel,
	syzbot+989efe781c74de1ddb54

On Thu, 12 Aug 2021 07:59:59 +0800 Nguyen Dinh Phi wrote:
> 
> The napi->rx_list is used to hold the GRO_NORMAL skbs before passing
> them to the stack, these skbs only passed to stack at the flush time or
> when the list's weight matches the predefined condition. In case the
> rx_list contains pending skbs when we remove the napi context, we need
> to clean out this list, otherwise, a memory leak will happen.

Thanks for your fix.

#syz test git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master


Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
Reported-by: syzbot+989efe781c74de1ddb54@syzkaller.appspotmail.com
---
 net/core/dev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index b51e41d0a7fe..319fffc62ce6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7038,6 +7038,13 @@ void __netif_napi_del(struct napi_struct *napi)
 	list_del_rcu(&napi->dev_list);
 	napi_free_frags(napi);

+	if (napi->rx_count) {
+		struct sk_buff *skb, *n;
+
+		list_for_each_entry_safe(skb, n, &napi->rx_list, list)
+			kfree_skb(skb);
+	}
+
 	flush_gro_hash(napi);
 	napi->gro_bitmask = 0;

--
2.25.1
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [syzbot] memory leak in packet_sendmsg
  2021-08-12  4:50 ` Hillf Danton
@ 2021-08-12  5:19     ` syzbot
  0 siblings, 0 replies; 14+ messages in thread
From: syzbot @ 2021-08-12  5:19 UTC (permalink / raw)
  To: hdanton, linux-kernel-mentees, linux-kernel, netdev, phind.uet,
	syzkaller-bugs

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

failed to apply patch:
checking file net/core/dev.c
patch: **** unexpected end of file in patch



Tested on:

commit:         761c6d7e Merge tag 'arc-5.14-rc6' of git://git.kernel...
git tree:       upstream
dashboard link: https://syzkaller.appspot.com/bug?extid=989efe781c74de1ddb54
compiler:       
patch:          https://syzkaller.appspot.com/x/patch.diff?x=147a5779300000


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [syzbot] memory leak in packet_sendmsg
@ 2021-08-12  5:19     ` syzbot
  0 siblings, 0 replies; 14+ messages in thread
From: syzbot @ 2021-08-12  5:19 UTC (permalink / raw)
  To: hdanton, linux-kernel-mentees, linux-kernel, netdev, phind.uet,
	syzkaller-bugs

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

failed to apply patch:
checking file net/core/dev.c
patch: **** unexpected end of file in patch



Tested on:

commit:         761c6d7e Merge tag 'arc-5.14-rc6' of git://git.kernel...
git tree:       upstream
dashboard link: https://syzkaller.appspot.com/bug?extid=989efe781c74de1ddb54
compiler:       
patch:          https://syzkaller.appspot.com/x/patch.diff?x=147a5779300000

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: drop skbs in napi->rx_list when removing the napi context.
  2021-08-11 23:59 ` Nguyen Dinh Phi
@ 2021-08-12  7:07   ` Eric Dumazet via Linux-kernel-mentees
  -1 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2021-08-12  7:07 UTC (permalink / raw)
  To: Nguyen Dinh Phi
  Cc: David Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	kpsingh, Antoine Tenart, Alexander Lobakin, Wei Wang, Taehee Yoo,
	Björn Töpel, memxor, netdev, LKML, bpf,
	linux-kernel-mentees, syzbot+989efe781c74de1ddb54

On Thu, Aug 12, 2021 at 2:00 AM Nguyen Dinh Phi <phind.uet@gmail.com> wrote:
>
> The napi->rx_list is used to hold the GRO_NORMAL skbs before passing
> them to the stack, these skbs only passed to stack at the flush time or
> when the list's weight matches the predefined condition. In case the
> rx_list contains pending skbs when we remove the napi context, we need
> to clean out this list, otherwise, a memory leak will happen.
>
> Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
> Reported-by: syzbot+989efe781c74de1ddb54@syzkaller.appspotmail.com

Thank you for working on this.

Please add a Fixes: tag, otherwise you are asking maintainers and
stable teams to find the original bug,
while you are in a much better position, since you spent time on
fixing the issue.

Also I object to this fix.

If packets have been stored temporarily in GRO, they should be
released at some point,
normally at the end of a napi poll.

By released, I mean that these packets should reach the upper stack,
instead of being dropped without
any notification.

It seems a call to gro_normal_list() is missing somewhere.

Can you find where ?

Thanks !

> ---
>  net/core/dev.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b51e41d0a7fe..319fffc62ce6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7038,6 +7038,13 @@ void __netif_napi_del(struct napi_struct *napi)
>         list_del_rcu(&napi->dev_list);
>         napi_free_frags(napi);
>
> +       if (napi->rx_count) {
> +               struct sk_buff *skb, *n;
> +
> +               list_for_each_entry_safe(skb, n, &napi->rx_list, list)
> +                       kfree_skb(skb);
> +       }
> +
>         flush_gro_hash(napi);
>         napi->gro_bitmask = 0;
>
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: drop skbs in napi->rx_list when removing the napi context.
@ 2021-08-12  7:07   ` Eric Dumazet via Linux-kernel-mentees
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet via Linux-kernel-mentees @ 2021-08-12  7:07 UTC (permalink / raw)
  To: Nguyen Dinh Phi
  Cc: Song Liu, Alexander Lobakin, syzbot+989efe781c74de1ddb54,
	Alexei Starovoitov, Wei Wang, Daniel Borkmann, John Fastabend,
	Andrii Nakryiko, Yonghong Song, linux-kernel-mentees,
	Jesper Dangaard Brouer, kpsingh, memxor, Jakub Kicinski, netdev,
	Antoine Tenart, LKML, David Miller, Björn Töpel, bpf,
	Martin KaFai Lau, Taehee Yoo

On Thu, Aug 12, 2021 at 2:00 AM Nguyen Dinh Phi <phind.uet@gmail.com> wrote:
>
> The napi->rx_list is used to hold the GRO_NORMAL skbs before passing
> them to the stack, these skbs only passed to stack at the flush time or
> when the list's weight matches the predefined condition. In case the
> rx_list contains pending skbs when we remove the napi context, we need
> to clean out this list, otherwise, a memory leak will happen.
>
> Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>
> Reported-by: syzbot+989efe781c74de1ddb54@syzkaller.appspotmail.com

Thank you for working on this.

Please add a Fixes: tag, otherwise you are asking maintainers and
stable teams to find the original bug,
while you are in a much better position, since you spent time on
fixing the issue.

Also I object to this fix.

If packets have been stored temporarily in GRO, they should be
released at some point,
normally at the end of a napi poll.

By released, I mean that these packets should reach the upper stack,
instead of being dropped without
any notification.

It seems a call to gro_normal_list() is missing somewhere.

Can you find where ?

Thanks !

> ---
>  net/core/dev.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b51e41d0a7fe..319fffc62ce6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7038,6 +7038,13 @@ void __netif_napi_del(struct napi_struct *napi)
>         list_del_rcu(&napi->dev_list);
>         napi_free_frags(napi);
>
> +       if (napi->rx_count) {
> +               struct sk_buff *skb, *n;
> +
> +               list_for_each_entry_safe(skb, n, &napi->rx_list, list)
> +                       kfree_skb(skb);
> +       }
> +
>         flush_gro_hash(napi);
>         napi->gro_bitmask = 0;
>
> --
> 2.25.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: drop skbs in napi->rx_list when removing the napi context.
  2021-08-12  7:07   ` Eric Dumazet via Linux-kernel-mentees
@ 2021-08-12 19:17     ` Phi Nguyen
  -1 siblings, 0 replies; 14+ messages in thread
From: Phi Nguyen @ 2021-08-12 19:17 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	kpsingh, Antoine Tenart, Alexander Lobakin, Wei Wang, Taehee Yoo,
	Björn Töpel, memxor, netdev, LKML, bpf,
	linux-kernel-mentees, syzbot+989efe781c74de1ddb54

On 8/12/2021 3:07 PM, Eric Dumazet wrote:
> Also I object to this fix.
> 
> If packets have been stored temporarily in GRO, they should be
> released at some point,
> normally at the end of a napi poll.
> 
> By released, I mean that these packets should reach the upper stack,
> instead of being dropped without
> any notification.
> 
> It seems a call to gro_normal_list() is missing somewhere.
> 
> Can you find where ?
> 
> Thanks !
> H Eric,

I think the location that should have a call to gro_normal_list() is 
__netif_napi_del(). Let say, if the driver call a function that lead to 
gro_normal_one(), and add a skb to the rx_list while the napi poll is 
not scheduled, and the driver remove the napi context before a napi poll 
could be triggered, then the added skb will be lost.

Actually, this was the first solution that I tried with syzbot (It 
passed the test too).
Best regards,
Phi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: drop skbs in napi->rx_list when removing the napi context.
@ 2021-08-12 19:17     ` Phi Nguyen
  0 siblings, 0 replies; 14+ messages in thread
From: Phi Nguyen @ 2021-08-12 19:17 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Song Liu, Alexander Lobakin, syzbot+989efe781c74de1ddb54,
	Alexei Starovoitov, Wei Wang, Daniel Borkmann, John Fastabend,
	Andrii Nakryiko, Yonghong Song, linux-kernel-mentees,
	Jesper Dangaard Brouer, kpsingh, memxor, Jakub Kicinski, netdev,
	Antoine Tenart, LKML, David Miller, Björn Töpel, bpf,
	Martin KaFai Lau, Taehee Yoo

On 8/12/2021 3:07 PM, Eric Dumazet wrote:
> Also I object to this fix.
> 
> If packets have been stored temporarily in GRO, they should be
> released at some point,
> normally at the end of a napi poll.
> 
> By released, I mean that these packets should reach the upper stack,
> instead of being dropped without
> any notification.
> 
> It seems a call to gro_normal_list() is missing somewhere.
> 
> Can you find where ?
> 
> Thanks !
> H Eric,

I think the location that should have a call to gro_normal_list() is 
__netif_napi_del(). Let say, if the driver call a function that lead to 
gro_normal_one(), and add a skb to the rx_list while the napi poll is 
not scheduled, and the driver remove the napi context before a napi poll 
could be triggered, then the added skb will be lost.

Actually, this was the first solution that I tried with syzbot (It 
passed the test too).
Best regards,
Phi
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: drop skbs in napi->rx_list when removing the napi context.
  2021-08-12 19:17     ` Phi Nguyen
@ 2021-08-13 10:16       ` Eric Dumazet
  -1 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2021-08-13 10:16 UTC (permalink / raw)
  To: Phi Nguyen, Eric Dumazet
  Cc: David Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	kpsingh, Antoine Tenart, Alexander Lobakin, Wei Wang, Taehee Yoo,
	Björn Töpel, memxor, netdev, LKML, bpf,
	linux-kernel-mentees, syzbot+989efe781c74de1ddb54,
	Mahesh Bandewar



On 8/12/21 9:17 PM, Phi Nguyen wrote:
> On 8/12/2021 3:07 PM, Eric Dumazet wrote:
>> Also I object to this fix.
>>
>> If packets have been stored temporarily in GRO, they should be
>> released at some point,
>> normally at the end of a napi poll.
>>
>> By released, I mean that these packets should reach the upper stack,
>> instead of being dropped without
>> any notification.
>>
>> It seems a call to gro_normal_list() is missing somewhere.
>>
>> Can you find where ?
>>
>> Thanks !
>> H Eric,
> 
> I think the location that should have a call to gro_normal_list() is __netif_napi_del(). Let say, if the driver call a function that lead to gro_normal_one(), and add a skb to the rx_list while the napi poll is not scheduled, and the driver remove the napi context before a napi poll could be triggered, then the added skb will be lost.
> 
> Actually, this was the first solution that I tried with syzbot (It passed the test too).
> Best regards,
> Phi

I think the real bug is in drivers/net/tun.c

It can call napi_gro_frags() and I do not see corresponding napi_complete()

This seems completely bogus.

Your patch only works around one the many bugs caused by 
commit 90e33d45940793def6f773b2d528e9f3c84ffdc7 tun: enable napi_gro_frags() for TUN/TAP driver

I suggest not adding your patch, because we should fix the root cause.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: drop skbs in napi->rx_list when removing the napi context.
@ 2021-08-13 10:16       ` Eric Dumazet
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2021-08-13 10:16 UTC (permalink / raw)
  To: Phi Nguyen, Eric Dumazet
  Cc: Song Liu, Alexander Lobakin, syzbot+989efe781c74de1ddb54,
	Alexei Starovoitov, Wei Wang, Mahesh Bandewar, Daniel Borkmann,
	John Fastabend, Andrii Nakryiko, Yonghong Song,
	linux-kernel-mentees, Jesper Dangaard Brouer, kpsingh, memxor,
	Jakub Kicinski, netdev, Antoine Tenart, LKML, David Miller,
	Björn Töpel, bpf, Martin KaFai Lau, Taehee Yoo



On 8/12/21 9:17 PM, Phi Nguyen wrote:
> On 8/12/2021 3:07 PM, Eric Dumazet wrote:
>> Also I object to this fix.
>>
>> If packets have been stored temporarily in GRO, they should be
>> released at some point,
>> normally at the end of a napi poll.
>>
>> By released, I mean that these packets should reach the upper stack,
>> instead of being dropped without
>> any notification.
>>
>> It seems a call to gro_normal_list() is missing somewhere.
>>
>> Can you find where ?
>>
>> Thanks !
>> H Eric,
> 
> I think the location that should have a call to gro_normal_list() is __netif_napi_del(). Let say, if the driver call a function that lead to gro_normal_one(), and add a skb to the rx_list while the napi poll is not scheduled, and the driver remove the napi context before a napi poll could be triggered, then the added skb will be lost.
> 
> Actually, this was the first solution that I tried with syzbot (It passed the test too).
> Best regards,
> Phi

I think the real bug is in drivers/net/tun.c

It can call napi_gro_frags() and I do not see corresponding napi_complete()

This seems completely bogus.

Your patch only works around one the many bugs caused by 
commit 90e33d45940793def6f773b2d528e9f3c84ffdc7 tun: enable napi_gro_frags() for TUN/TAP driver

I suggest not adding your patch, because we should fix the root cause.

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: drop skbs in napi->rx_list when removing the napi context.
  2021-08-13 10:16       ` Eric Dumazet
@ 2021-08-13 10:51         ` Nguyen Dinh Phi
  -1 siblings, 0 replies; 14+ messages in thread
From: Nguyen Dinh Phi @ 2021-08-13 10:51 UTC (permalink / raw)
  To: Eric Dumazet, Eric Dumazet
  Cc: David Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Jesper Dangaard Brouer, John Fastabend,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	kpsingh, Antoine Tenart, Alexander Lobakin, Wei Wang, Taehee Yoo,
	Björn Töpel, memxor, netdev, LKML, bpf,
	linux-kernel-mentees, syzbot+989efe781c74de1ddb54,
	Mahesh Bandewar

On August 13, 2021 6:16:34 PM GMT+08:00, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>On 8/12/21 9:17 PM, Phi Nguyen wrote:
>> On 8/12/2021 3:07 PM, Eric Dumazet wrote:
>>> Also I object to this fix.
>>>
>>> If packets have been stored temporarily in GRO, they should be
>>> released at some point,
>>> normally at the end of a napi poll.
>>>
>>> By released, I mean that these packets should reach the upper stack,
>>> instead of being dropped without
>>> any notification.
>>>
>>> It seems a call to gro_normal_list() is missing somewhere.
>>>
>>> Can you find where ?
>>>
>>> Thanks !
>>> H Eric,
>> 
>> I think the location that should have a call to gro_normal_list() is
>__netif_napi_del(). Let say, if the driver call a function that lead to
>gro_normal_one(), and add a skb to the rx_list while the napi poll is
>not scheduled, and the driver remove the napi context before a napi
>poll could be triggered, then the added skb will be lost.
>> 
>> Actually, this was the first solution that I tried with syzbot (It
>passed the test too).
>> Best regards,
>> Phi
>
>I think the real bug is in drivers/net/tun.c
>
>It can call napi_gro_frags() and I do not see corresponding
>napi_complete()
>
>This seems completely bogus.
>
>Your patch only works around one the many bugs caused by 
>commit 90e33d45940793def6f773b2d528e9f3c84ffdc7 tun: enable
>napi_gro_frags() for TUN/TAP driver
>
>I suggest not adding your patch, because we should fix the root cause.

Yes, it truely comes from tun driver. But it was there before listified RX added (that time the skb will be put to stack immediately), that why I made the fix in dev.c. I thought I should change the later commit.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] net: drop skbs in napi->rx_list when removing the napi context.
@ 2021-08-13 10:51         ` Nguyen Dinh Phi
  0 siblings, 0 replies; 14+ messages in thread
From: Nguyen Dinh Phi @ 2021-08-13 10:51 UTC (permalink / raw)
  To: Eric Dumazet, Eric Dumazet
  Cc: Song Liu, Alexander Lobakin, syzbot+989efe781c74de1ddb54,
	Alexei Starovoitov, Wei Wang, Mahesh Bandewar, Daniel Borkmann,
	John Fastabend, Andrii Nakryiko, Yonghong Song,
	linux-kernel-mentees, Jesper Dangaard Brouer, kpsingh, memxor,
	Jakub Kicinski, netdev, Antoine Tenart, LKML, David Miller,
	Björn Töpel, bpf, Martin KaFai Lau, Taehee Yoo

On August 13, 2021 6:16:34 PM GMT+08:00, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>On 8/12/21 9:17 PM, Phi Nguyen wrote:
>> On 8/12/2021 3:07 PM, Eric Dumazet wrote:
>>> Also I object to this fix.
>>>
>>> If packets have been stored temporarily in GRO, they should be
>>> released at some point,
>>> normally at the end of a napi poll.
>>>
>>> By released, I mean that these packets should reach the upper stack,
>>> instead of being dropped without
>>> any notification.
>>>
>>> It seems a call to gro_normal_list() is missing somewhere.
>>>
>>> Can you find where ?
>>>
>>> Thanks !
>>> H Eric,
>> 
>> I think the location that should have a call to gro_normal_list() is
>__netif_napi_del(). Let say, if the driver call a function that lead to
>gro_normal_one(), and add a skb to the rx_list while the napi poll is
>not scheduled, and the driver remove the napi context before a napi
>poll could be triggered, then the added skb will be lost.
>> 
>> Actually, this was the first solution that I tried with syzbot (It
>passed the test too).
>> Best regards,
>> Phi
>
>I think the real bug is in drivers/net/tun.c
>
>It can call napi_gro_frags() and I do not see corresponding
>napi_complete()
>
>This seems completely bogus.
>
>Your patch only works around one the many bugs caused by 
>commit 90e33d45940793def6f773b2d528e9f3c84ffdc7 tun: enable
>napi_gro_frags() for TUN/TAP driver
>
>I suggest not adding your patch, because we should fix the root cause.

Yes, it truely comes from tun driver. But it was there before listified RX added (that time the skb will be put to stack immediately), that why I made the fix in dev.c. I thought I should change the later commit. 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [syzbot] memory leak in packet_sendmsg
@ 2021-07-30 21:08 syzbot
  0 siblings, 0 replies; 14+ messages in thread
From: syzbot @ 2021-07-30 21:08 UTC (permalink / raw)
  To: andrii, ast, bpf, daniel, davem, edumazet, john.fastabend, kafai,
	kpsingh, kuba, linux-kernel, netdev, songliubraving,
	syzkaller-bugs, tannerlove, willemb, xie.he.0141, yhs

Hello,

syzbot found the following issue on:

HEAD commit:    ff1176468d36 Linux 5.14-rc3
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=15057fa2300000
kernel config:  https://syzkaller.appspot.com/x/.config?x=4ebfe83ba9ca8666
dashboard link: https://syzkaller.appspot.com/bug?extid=989efe781c74de1ddb54
compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.1
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16e54382300000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+989efe781c74de1ddb54@syzkaller.appspotmail.com

2021/07/26 20:48:07 executed programs: 1
2021/07/26 20:48:13 executed programs: 3
2021/07/26 20:48:19 executed programs: 5
BUG: memory leak
unreferenced object 0xffff88810f41be00 (size 232):
  comm "dhclient", pid 4908, jiffies 4294938558 (age 1092.590s)
  hex dump (first 32 bytes):
    a0 6c 13 19 81 88 ff ff a0 6c 13 19 81 88 ff ff  .l.......l......
    00 00 83 1a 81 88 ff ff 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff836e1e8f>] __alloc_skb+0x20f/0x280 net/core/skbuff.c:414
    [<ffffffff836ec6ba>] alloc_skb include/linux/skbuff.h:1112 [inline]
    [<ffffffff836ec6ba>] alloc_skb_with_frags+0x6a/0x2b0 net/core/skbuff.c:6019
    [<ffffffff836d9fa3>] sock_alloc_send_pskb+0x353/0x3c0 net/core/sock.c:2461
    [<ffffffff83bf47a2>] packet_alloc_skb net/packet/af_packet.c:2864 [inline]
    [<ffffffff83bf47a2>] packet_snd net/packet/af_packet.c:2959 [inline]
    [<ffffffff83bf47a2>] packet_sendmsg+0xbd2/0x2500 net/packet/af_packet.c:3044
    [<ffffffff836d0b46>] sock_sendmsg_nosec net/socket.c:703 [inline]
    [<ffffffff836d0b46>] sock_sendmsg+0x56/0x80 net/socket.c:723
    [<ffffffff836d0c67>] sock_write_iter+0xf7/0x180 net/socket.c:1056
    [<ffffffff81564527>] call_write_iter include/linux/fs.h:2114 [inline]
    [<ffffffff81564527>] new_sync_write+0x1d7/0x2b0 fs/read_write.c:518
    [<ffffffff81567ba1>] vfs_write+0x351/0x400 fs/read_write.c:605
    [<ffffffff81567f1b>] ksys_write+0x12b/0x160 fs/read_write.c:658
    [<ffffffff843b18b5>] do_syscall_x64 arch/x86/entry/common.c:50 [inline]
    [<ffffffff843b18b5>] do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
    [<ffffffff84400068>] entry_SYSCALL_64_after_hwframe+0x44/0xae

BUG: memory leak
unreferenced object 0xffff8881019ce500 (size 232):
  comm "kworker/1:1", pid 35, jiffies 4294938559 (age 1092.580s)
  hex dump (first 32 bytes):
    a0 d4 28 19 81 88 ff ff a0 d4 28 19 81 88 ff ff  ..(.......(.....
    00 00 cb 03 81 88 ff ff 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff836e1e8f>] __alloc_skb+0x20f/0x280 net/core/skbuff.c:414
    [<ffffffff836ec6ba>] alloc_skb include/linux/skbuff.h:1112 [inline]
    [<ffffffff836ec6ba>] alloc_skb_with_frags+0x6a/0x2b0 net/core/skbuff.c:6019
    [<ffffffff836d9fa3>] sock_alloc_send_pskb+0x353/0x3c0 net/core/sock.c:2461
    [<ffffffff83b812d4>] mld_newpack+0x84/0x200 net/ipv6/mcast.c:1751
    [<ffffffff83b814f3>] add_grhead+0xa3/0xc0 net/ipv6/mcast.c:1854
    [<ffffffff83b82196>] add_grec+0x7b6/0x820 net/ipv6/mcast.c:1992
    [<ffffffff83b84643>] mld_send_cr net/ipv6/mcast.c:2118 [inline]
    [<ffffffff83b84643>] mld_ifc_work+0x273/0x750 net/ipv6/mcast.c:2655
    [<ffffffff81262669>] process_one_work+0x2c9/0x610 kernel/workqueue.c:2276
    [<ffffffff81262f59>] worker_thread+0x59/0x5d0 kernel/workqueue.c:2422
    [<ffffffff8126c3b8>] kthread+0x188/0x1d0 kernel/kthread.c:319
    [<ffffffff810022cf>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295

BUG: memory leak
unreferenced object 0xffff88810f41b300 (size 232):
  comm "kworker/1:1", pid 35, jiffies 4294938624 (age 1091.930s)
  hex dump (first 32 bytes):
    a0 ac 3f 19 81 88 ff ff a0 ac 3f 19 81 88 ff ff  ..?.......?.....
    00 00 cb 03 81 88 ff ff 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff836e1e8f>] __alloc_skb+0x20f/0x280 net/core/skbuff.c:414
    [<ffffffff83b6d076>] alloc_skb include/linux/skbuff.h:1112 [inline]
    [<ffffffff83b6d076>] ndisc_alloc_skb+0x56/0xe0 net/ipv6/ndisc.c:420
    [<ffffffff83b7183a>] ndisc_send_ns+0xba/0x2f0 net/ipv6/ndisc.c:626
    [<ffffffff83b48b13>] addrconf_dad_work+0x643/0x900 net/ipv6/addrconf.c:4119
    [<ffffffff81262669>] process_one_work+0x2c9/0x610 kernel/workqueue.c:2276
    [<ffffffff81262f59>] worker_thread+0x59/0x5d0 kernel/workqueue.c:2422
    [<ffffffff8126c3b8>] kthread+0x188/0x1d0 kernel/kthread.c:319
    [<ffffffff810022cf>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295

BUG: memory leak
unreferenced object 0xffff88810dd97600 (size 232):
  comm "softirq", pid 0, jiffies 4294938659 (age 1091.580s)
  hex dump (first 32 bytes):
    a0 fc fb 16 81 88 ff ff a0 fc fb 16 81 88 ff ff  ................
    00 c0 84 03 81 88 ff ff 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<ffffffff836e1e8f>] __alloc_skb+0x20f/0x280 net/core/skbuff.c:414
    [<ffffffff839f1aff>] alloc_skb include/linux/skbuff.h:1112 [inline]
    [<ffffffff839f1aff>] __ip_append_data+0x12cf/0x1510 net/ipv4/ip_output.c:1109
    [<ffffffff839f429d>] ip_append_data net/ipv4/ip_output.c:1327 [inline]
    [<ffffffff839f429d>] ip_append_data net/ipv4/ip_output.c:1306 [inline]
    [<ffffffff839f429d>] ip_send_unicast_reply+0x33d/0x550 net/ipv4/ip_output.c:1718
    [<ffffffff83a33e6f>] tcp_v4_send_reset+0x3df/0x980 net/ipv4/tcp_ipv4.c:818
    [<ffffffff83a37442>] tcp_v4_rcv+0xf22/0x1620 net/ipv4/tcp_ipv4.c:2116
    [<ffffffff839e99b2>] ip_protocol_deliver_rcu+0x22/0x2c0 net/ipv4/ip_input.c:204
    [<ffffffff839e9cc1>] ip_local_deliver_finish+0x71/0x90 net/ipv4/ip_input.c:231
    [<ffffffff839e9e33>] NF_HOOK include/linux/netfilter.h:307 [inline]
    [<ffffffff839e9e33>] NF_HOOK include/linux/netfilter.h:301 [inline]
    [<ffffffff839e9e33>] ip_local_deliver+0x153/0x160 net/ipv4/ip_input.c:252
    [<ffffffff839e9016>] dst_input include/net/dst.h:458 [inline]
    [<ffffffff839e9016>] ip_sublist_rcv_finish+0x76/0x90 net/ipv4/ip_input.c:551
    [<ffffffff839e9723>] ip_list_rcv_finish net/ipv4/ip_input.c:601 [inline]
    [<ffffffff839e9723>] ip_sublist_rcv+0x293/0x340 net/ipv4/ip_input.c:609
    [<ffffffff839ea126>] ip_list_rcv+0x1c6/0x1f0 net/ipv4/ip_input.c:644
    [<ffffffff83713f01>] __netif_receive_skb_list_ptype net/core/dev.c:5541 [inline]
    [<ffffffff83713f01>] __netif_receive_skb_list_core+0x2b1/0x360 net/core/dev.c:5589
    [<ffffffff83714305>] __netif_receive_skb_list net/core/dev.c:5641 [inline]
    [<ffffffff83714305>] netif_receive_skb_list_internal+0x355/0x4a0 net/core/dev.c:5751
    [<ffffffff83715d52>] gro_normal_list net/core/dev.c:5905 [inline]
    [<ffffffff83715d52>] gro_normal_list net/core/dev.c:5901 [inline]
    [<ffffffff83715d52>] napi_complete_done+0xe2/0x2e0 net/core/dev.c:6627
    [<ffffffff828eb89d>] virtqueue_napi_complete drivers/net/virtio_net.c:337 [inline]
    [<ffffffff828eb89d>] virtnet_poll+0x52d/0x6a0 drivers/net/virtio_net.c:1546
    [<ffffffff83715f8d>] __napi_poll+0x3d/0x290 net/core/dev.c:7047



---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-08-13 10:51 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-11 23:59 [PATCH] net: drop skbs in napi->rx_list when removing the napi context Nguyen Dinh Phi
2021-08-11 23:59 ` Nguyen Dinh Phi
2021-08-12  4:50 ` Hillf Danton
2021-08-12  5:19   ` [syzbot] memory leak in packet_sendmsg syzbot
2021-08-12  5:19     ` syzbot
2021-08-12  7:07 ` [PATCH] net: drop skbs in napi->rx_list when removing the napi context Eric Dumazet
2021-08-12  7:07   ` Eric Dumazet via Linux-kernel-mentees
2021-08-12 19:17   ` Phi Nguyen
2021-08-12 19:17     ` Phi Nguyen
2021-08-13 10:16     ` Eric Dumazet
2021-08-13 10:16       ` Eric Dumazet
2021-08-13 10:51       ` Nguyen Dinh Phi
2021-08-13 10:51         ` Nguyen Dinh Phi
  -- strict thread matches above, loose matches on Subject: below --
2021-07-30 21:08 [syzbot] memory leak in packet_sendmsg syzbot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.