| Message ID | 20190503114721.10502-1-edumazet@google.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] ip6: fix skb leak in ip6frag_expire_frag_queue() | expand |
Le 03/05/2019 à 13:47, Eric Dumazet a écrit : > Since ip6frag_expire_frag_queue() now pulls the head skb > from frag queue, we should no longer use skb_get(), since > this leads to an skb leak. > > Stefan Bader initially reported a problem in 4.4.stable [1] caused > by the skb_get(), so this patch should also fix this issue. > > 296583.091021] kernel BUG at /build/linux-6VmqmP/linux-4.4.0/net/core/skbuff.c:1207! > [296583.091734] Call Trace: > [296583.091749] [<ffffffff81740e50>] __pskb_pull_tail+0x50/0x350 > [296583.091764] [<ffffffff8183939a>] _decode_session6+0x26a/0x400 > [296583.091779] [<ffffffff817ec719>] __xfrm_decode_session+0x39/0x50 > [296583.091795] [<ffffffff818239d0>] icmpv6_route_lookup+0xf0/0x1c0 > [296583.091809] [<ffffffff81824421>] icmp6_send+0x5e1/0x940 > [296583.091823] [<ffffffff81753238>] ? __netif_receive_skb+0x18/0x60 > [296583.091838] [<ffffffff817532b2>] ? netif_receive_skb_internal+0x32/0xa0 > [296583.091858] [<ffffffffc0199f74>] ? ixgbe_clean_rx_irq+0x594/0xac0 [ixgbe] > [296583.091876] [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6] > [296583.091893] [<ffffffff8183d431>] icmpv6_send+0x21/0x30 > [296583.091906] [<ffffffff8182b500>] ip6_expire_frag_queue+0xe0/0x120 > [296583.091921] [<ffffffffc04eb27f>] nf_ct_frag6_expire+0x1f/0x30 [nf_defrag_ipv6] > [296583.091938] [<ffffffff810f3b57>] call_timer_fn+0x37/0x140 > [296583.091951] [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6] > [296583.091968] [<ffffffff810f5464>] run_timer_softirq+0x234/0x330 > [296583.091982] [<ffffffff8108a339>] __do_softirq+0x109/0x2b0 > > Fixes: d4289fcc9b16 ("net: IP6 defrag: use rbtrees for IPv6 defrag") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Stfan Bader <stefan.bader@canonical.com> nit: the 'e' is missing in Stefan ;-)
On Fri, May 3, 2019 at 10:55 AM Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > > Le 03/05/2019 à 13:47, Eric Dumazet a écrit : > > Since ip6frag_expire_frag_queue() now pulls the head skb > > from frag queue, we should no longer use skb_get(), since > > this leads to an skb leak. > > > > Stefan Bader initially reported a problem in 4.4.stable [1] caused > > by the skb_get(), so this patch should also fix this issue. > > > > 296583.091021] kernel BUG at /build/linux-6VmqmP/linux-4.4.0/net/core/skbuff.c:1207! > > [296583.091734] Call Trace: > > [296583.091749] [<ffffffff81740e50>] __pskb_pull_tail+0x50/0x350 > > [296583.091764] [<ffffffff8183939a>] _decode_session6+0x26a/0x400 > > [296583.091779] [<ffffffff817ec719>] __xfrm_decode_session+0x39/0x50 > > [296583.091795] [<ffffffff818239d0>] icmpv6_route_lookup+0xf0/0x1c0 > > [296583.091809] [<ffffffff81824421>] icmp6_send+0x5e1/0x940 > > [296583.091823] [<ffffffff81753238>] ? __netif_receive_skb+0x18/0x60 > > [296583.091838] [<ffffffff817532b2>] ? netif_receive_skb_internal+0x32/0xa0 > > [296583.091858] [<ffffffffc0199f74>] ? ixgbe_clean_rx_irq+0x594/0xac0 [ixgbe] > > [296583.091876] [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6] > > [296583.091893] [<ffffffff8183d431>] icmpv6_send+0x21/0x30 > > [296583.091906] [<ffffffff8182b500>] ip6_expire_frag_queue+0xe0/0x120 > > [296583.091921] [<ffffffffc04eb27f>] nf_ct_frag6_expire+0x1f/0x30 [nf_defrag_ipv6] > > [296583.091938] [<ffffffff810f3b57>] call_timer_fn+0x37/0x140 > > [296583.091951] [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6] > > [296583.091968] [<ffffffff810f5464>] run_timer_softirq+0x234/0x330 > > [296583.091982] [<ffffffff8108a339>] __do_softirq+0x109/0x2b0 > > > > Fixes: d4289fcc9b16 ("net: IP6 defrag: use rbtrees for IPv6 defrag") > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Reported-by: Stfan Bader <stefan.bader@canonical.com> > nit: the 'e' is missing in Stefan ;-) Indeed, copy/paste error, thanks.
On Fri, May 3, 2019 at 4:47 AM Eric Dumazet <edumazet@google.com> wrote: > > Since ip6frag_expire_frag_queue() now pulls the head skb > from frag queue, we should no longer use skb_get(), since > this leads to an skb leak. > > Stefan Bader initially reported a problem in 4.4.stable [1] caused > by the skb_get(), so this patch should also fix this issue. > > 296583.091021] kernel BUG at /build/linux-6VmqmP/linux-4.4.0/net/core/skbuff.c:1207! > [296583.091734] Call Trace: > [296583.091749] [<ffffffff81740e50>] __pskb_pull_tail+0x50/0x350 > [296583.091764] [<ffffffff8183939a>] _decode_session6+0x26a/0x400 > [296583.091779] [<ffffffff817ec719>] __xfrm_decode_session+0x39/0x50 > [296583.091795] [<ffffffff818239d0>] icmpv6_route_lookup+0xf0/0x1c0 > [296583.091809] [<ffffffff81824421>] icmp6_send+0x5e1/0x940 > [296583.091823] [<ffffffff81753238>] ? __netif_receive_skb+0x18/0x60 > [296583.091838] [<ffffffff817532b2>] ? netif_receive_skb_internal+0x32/0xa0 > [296583.091858] [<ffffffffc0199f74>] ? ixgbe_clean_rx_irq+0x594/0xac0 [ixgbe] > [296583.091876] [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6] > [296583.091893] [<ffffffff8183d431>] icmpv6_send+0x21/0x30 > [296583.091906] [<ffffffff8182b500>] ip6_expire_frag_queue+0xe0/0x120 > [296583.091921] [<ffffffffc04eb27f>] nf_ct_frag6_expire+0x1f/0x30 [nf_defrag_ipv6] > [296583.091938] [<ffffffff810f3b57>] call_timer_fn+0x37/0x140 > [296583.091951] [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6] > [296583.091968] [<ffffffff810f5464>] run_timer_softirq+0x234/0x330 > [296583.091982] [<ffffffff8108a339>] __do_softirq+0x109/0x2b0 > > Fixes: d4289fcc9b16 ("net: IP6 defrag: use rbtrees for IPv6 defrag") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Stfan Bader <stefan.bader@canonical.com> > Cc: Peter Oskolkov <posk@google.com> > Cc: Florian Westphal <fw@strlen.de> > --- > include/net/ipv6_frag.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/include/net/ipv6_frag.h b/include/net/ipv6_frag.h > index 28aa9b30aeceac9a86ee6754e4b5809be115e947..1f77fb4dc79df6bc4e41d6d2f4d49ace32082ca4 100644 > --- a/include/net/ipv6_frag.h > +++ b/include/net/ipv6_frag.h > @@ -94,7 +94,6 @@ ip6frag_expire_frag_queue(struct net *net, struct frag_queue *fq) > goto out; > > head->dev = dev; > - skb_get(head); This skb_get was introduced by commit 05c0b86b9696802fd0ce5676a92a63f1b455bdf3 "ipv6: frags: rewrite ip6_expire_frag_queue()", and the rbtree patch is not in 4.4, where the bug is reported at. Shouldn't the "Fixes" tag also reference the original patch? > spin_unlock(&fq->q.lock); > > icmpv6_send(head, ICMPV6_TIME_EXCEED, ICMPV6_EXC_FRAGTIME, 0); > -- > 2.21.0.1020.gf2820cf01a-goog >
On Fri, May 3, 2019 at 11:33 AM Peter Oskolkov <posk@google.com> wrote: > > This skb_get was introduced by commit 05c0b86b9696802fd0ce5676a92a63f1b455bdf3 > "ipv6: frags: rewrite ip6_expire_frag_queue()", and the rbtree patch > is not in 4.4, where the bug is reported at. > Shouldn't the "Fixes" tag also reference the original patch? No, this bug really fixes a memory leak. Fact that it also fixes the XFRM issue is secondary, since all your patches are being backported in stable trees anyway for other reasons. There is no need to list all commits and give a complete context for a bug fix like this one, this would be quite noisy.
On Fri, May 3, 2019 at 8:52 AM Eric Dumazet <edumazet@google.com> wrote: > > On Fri, May 3, 2019 at 11:33 AM Peter Oskolkov <posk@google.com> wrote: > > > > This skb_get was introduced by commit 05c0b86b9696802fd0ce5676a92a63f1b455bdf3 > > "ipv6: frags: rewrite ip6_expire_frag_queue()", and the rbtree patch > > is not in 4.4, where the bug is reported at. > > Shouldn't the "Fixes" tag also reference the original patch? > > No, this bug really fixes a memory leak. > > Fact that it also fixes the XFRM issue is secondary, since all your > patches are being backported in stable > trees anyway for other reasons. There are no plans to backport rbtree patches to 4.4 and earlier at the moment, afaik. > > There is no need to list all commits and give a complete context for a > bug fix like this one, > this would be quite noisy.
On 5/3/19 11:58 AM, Peter Oskolkov wrote: > On Fri, May 3, 2019 at 8:52 AM Eric Dumazet <edumazet@google.com> wrote: >> >> On Fri, May 3, 2019 at 11:33 AM Peter Oskolkov <posk@google.com> wrote: >>> >>> This skb_get was introduced by commit 05c0b86b9696802fd0ce5676a92a63f1b455bdf3 >>> "ipv6: frags: rewrite ip6_expire_frag_queue()", and the rbtree patch >>> is not in 4.4, where the bug is reported at. >>> Shouldn't the "Fixes" tag also reference the original patch? >> >> No, this bug really fixes a memory leak. >> >> Fact that it also fixes the XFRM issue is secondary, since all your >> patches are being backported in stable >> trees anyway for other reasons. > > There are no plans to backport rbtree patches to 4.4 and earlier at > the moment, afaik. > No problem, I mentioned to Stefan what needs to be done. (removing the head skb, removing the skb_get())
diff --git a/include/net/ipv6_frag.h b/include/net/ipv6_frag.h index 28aa9b30aeceac9a86ee6754e4b5809be115e947..1f77fb4dc79df6bc4e41d6d2f4d49ace32082ca4 100644 --- a/include/net/ipv6_frag.h +++ b/include/net/ipv6_frag.h @@ -94,7 +94,6 @@ ip6frag_expire_frag_queue(struct net *net, struct frag_queue *fq) goto out; head->dev = dev; - skb_get(head); spin_unlock(&fq->q.lock); icmpv6_send(head, ICMPV6_TIME_EXCEED, ICMPV6_EXC_FRAGTIME, 0);
Since ip6frag_expire_frag_queue() now pulls the head skb from frag queue, we should no longer use skb_get(), since this leads to an skb leak. Stefan Bader initially reported a problem in 4.4.stable [1] caused by the skb_get(), so this patch should also fix this issue. 296583.091021] kernel BUG at /build/linux-6VmqmP/linux-4.4.0/net/core/skbuff.c:1207! [296583.091734] Call Trace: [296583.091749] [<ffffffff81740e50>] __pskb_pull_tail+0x50/0x350 [296583.091764] [<ffffffff8183939a>] _decode_session6+0x26a/0x400 [296583.091779] [<ffffffff817ec719>] __xfrm_decode_session+0x39/0x50 [296583.091795] [<ffffffff818239d0>] icmpv6_route_lookup+0xf0/0x1c0 [296583.091809] [<ffffffff81824421>] icmp6_send+0x5e1/0x940 [296583.091823] [<ffffffff81753238>] ? __netif_receive_skb+0x18/0x60 [296583.091838] [<ffffffff817532b2>] ? netif_receive_skb_internal+0x32/0xa0 [296583.091858] [<ffffffffc0199f74>] ? ixgbe_clean_rx_irq+0x594/0xac0 [ixgbe] [296583.091876] [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6] [296583.091893] [<ffffffff8183d431>] icmpv6_send+0x21/0x30 [296583.091906] [<ffffffff8182b500>] ip6_expire_frag_queue+0xe0/0x120 [296583.091921] [<ffffffffc04eb27f>] nf_ct_frag6_expire+0x1f/0x30 [nf_defrag_ipv6] [296583.091938] [<ffffffff810f3b57>] call_timer_fn+0x37/0x140 [296583.091951] [<ffffffffc04eb260>] ? nf_ct_net_exit+0x50/0x50 [nf_defrag_ipv6] [296583.091968] [<ffffffff810f5464>] run_timer_softirq+0x234/0x330 [296583.091982] [<ffffffff8108a339>] __do_softirq+0x109/0x2b0 Fixes: d4289fcc9b16 ("net: IP6 defrag: use rbtrees for IPv6 defrag") Signed-off-by: Eric Dumazet <edumazet@google.com> Reported-by: Stfan Bader <stefan.bader@canonical.com> Cc: Peter Oskolkov <posk@google.com> Cc: Florian Westphal <fw@strlen.de> --- include/net/ipv6_frag.h | 1 - 1 file changed, 1 deletion(-)