| Message ID | 20210811235959.1099333-1-phind.uet@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: drop skbs in napi->rx_list when removing the napi context. | expand |
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 >
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
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.
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.
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;
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(+) -- 2.25.1