* [PATCH net] mld: fix panic in mld_newpack()
@ 2020-12-23 16:52 Taehee Yoo
2020-12-26 19:27 ` Cong Wang
0 siblings, 1 reply; 5+ messages in thread
From: Taehee Yoo @ 2020-12-23 16:52 UTC (permalink / raw)
To: davem, kuba, netdev; +Cc: ap420073, eric.dumazet
mld_newpack() doesn't allow to allocate high order page,
just order-0 allocation is allowed.
If headroom size is too large, a kernel panic could occur in skb_put().
Test commands:
ip netns add A
ip netns add B
ip link add veth0 type veth peer name veth1
ip link set veth0 netns A
ip link set veth1 netns B
ip netns exec A ip link set lo up
ip netns exec A ip link set veth0 up
ip netns exec A ip link add ip6tnl100 type ip6tnl local 2001:db8:99::1 \
remote 2001:db8:99::2
ip netns exec A ip -6 a a 2001:db8:100::1/64 dev ip6tnl100
ip netns exec A ip link set ip6tnl100 up
for i in {99..1}
do
let A=$i-1
ip netns exec A ip link add ip6tnl$i type ip6tnl local \
2001:db8:$A::1 remote 2001:db8:$A::2
ip netns exec A ip -6 a a 2001:db8:$i::1/64 dev ip6tnl$i
ip netns exec A ip link set ip6tnl$i up
done
ip netns exec A ip -6 a a 2001:db8:0::1/64 dev veth0
ip netns exec B ip link set lo up
ip netns exec B ip link set veth1 up
ip netns exec B ip link add ip6tnl100 type ip6tnl local 2001:db8:99::2 \
remote 2001:db8:99::1
ip netns exec B ip -6 a a 2001:db8:100::2/64 dev ip6tnl100
ip netns exec B ip link set ip6tnl100 up
for i in {99..1}
do
let B=$i-1
ip netns exec B ip link add ip6tnl$i type ip6tnl local \
2001:db8:$B::2 remote 2001:db8:$B::1
ip netns exec B ip -6 a a 2001:db8:$i::2/64 dev ip6tnl$i
ip netns exec B ip link set ip6tnl$i up
done
ip netns exec B ip -6 a a 2001:db8:0::2/64 dev veth1
Splat looks like:
[ 104.047694][ T104] skbuff: skb_over_panic: text:ffffffffb0c31a92 len:56 put:8 head:ffff888009609000 data:ffff888009609e90 tail:0xec8 end:0xec0 dev:ip6gre4b
[ 104.053431][ T104] ------------[ cut here ]------------
[ 104.055733][ T104] kernel BUG at net/core/skbuff.c:109!
[ 104.058014][ T104] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC KASAN PTI
[ 104.060761][ T104] CPU: 4 PID: 104 Comm: kworker/4:1 Not tainted 5.10.0+ #811
[ 104.064000][ T104] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[ 104.068077][ T104] Workqueue: ipv6_addrconf addrconf_dad_work
[ 104.070096][ T104] RIP: 0010:skb_panic+0x15d/0x15f
[ 104.072335][ T104] Code: 98 fe 4c 8b 4c 24 10 53 8b 4d 70 45 89 e0 48 c7 c7 60 8b 78 b1 41 57 41 56 41 55 48 8b 54 24 20 48 8b 74 24 28 e8 b5 40 f9 ff <0f> 0b 48 8b 6c 24 20 89 34 24 e8 08 c9 98 fe 8b 34 24 48 c7 c1 80
[ 104.079948][ T104] RSP: 0018:ffff888102557870 EFLAGS: 00010282
[ 104.082361][ T104] RAX: 0000000000000088 RBX: ffff888101c7c000 RCX: 0000000000000000
[ 104.085878][ T104] RDX: 0000000000000088 RSI: 0000000000000008 RDI: ffffed10204aaf05
[ 104.088906][ T104] RBP: ffff8881165f60c0 R08: ffffed102338018d R09: ffffed102338018d
[ 104.092111][ T104] R10: ffff888119c00c67 R11: ffffed102338018c R12: 0000000000000008
[ 104.095291][ T104] R13: ffff888009609e90 R14: 0000000000000ec8 R15: 0000000000000ec0
[ 104.098023][ T104] FS: 0000000000000000(0000) GS:ffff888119a00000(0000) knlGS:0000000000000000
[ 104.101532][ T104] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 104.103972][ T104] CR2: 000055a06421b7cc CR3: 000000010d55a002 CR4: 00000000003706e0
[ 104.107058][ T104] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 104.110048][ T104] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 104.113020][ T104] Call Trace:
[ 104.114253][ T104] ? mld_newpack+0x4d2/0x8f0
[ 104.115875][ T104] ? mld_newpack+0x4d2/0x8f0
[ 104.117389][ T104] skb_put.cold.104+0x22/0x22
[ 104.118940][ T104] mld_newpack+0x4d2/0x8f0
[ 104.120389][ T104] ? ip6_mc_hdr.isra.25.constprop.47+0x600/0x600
[ 104.122466][ T104] ? register_lock_class+0x1910/0x1910
[ 104.124256][ T104] ? mark_lock.part.46+0xef/0x1c20
[ 104.125925][ T104] add_grhead.isra.32+0x280/0x380
[ 104.127574][ T104] add_grec+0xb13/0xdc0
[ 104.128952][ T104] ? rcu_read_lock_bh_held+0xa0/0xa0
[ ... ]
Allowing high order page allocation could fix this problem.
Fixes: 72e09ad107e7 ("ipv6: avoid high order allocations")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
net/ipv6/mcast.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 6c8604390266..2cab0c563214 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -1601,10 +1601,7 @@ static struct sk_buff *mld_newpack(struct inet6_dev *idev, unsigned int mtu)
IPV6_TLV_PADN, 0 };
/* we assume size > sizeof(ra) here */
- /* limit our allocations to order-0 page */
- size = min_t(int, size, SKB_MAX_ORDER(0, 0));
skb = sock_alloc_send_skb(sk, size, 1, &err);
-
if (!skb)
return NULL;
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] mld: fix panic in mld_newpack()
2020-12-23 16:52 [PATCH net] mld: fix panic in mld_newpack() Taehee Yoo
@ 2020-12-26 19:27 ` Cong Wang
2020-12-27 14:40 ` Taehee Yoo
0 siblings, 1 reply; 5+ messages in thread
From: Cong Wang @ 2020-12-26 19:27 UTC (permalink / raw)
To: Taehee Yoo
Cc: David Miller, Jakub Kicinski, Linux Kernel Network Developers,
Eric Dumazet
On Wed, Dec 23, 2020 at 8:55 AM Taehee Yoo <ap420073@gmail.com> wrote:
>
> mld_newpack() doesn't allow to allocate high order page,
> just order-0 allocation is allowed.
> If headroom size is too large, a kernel panic could occur in skb_put().
...
> Allowing high order page allocation could fix this problem.
>
> Fixes: 72e09ad107e7 ("ipv6: avoid high order allocations")
So you just revert this commit which fixes another issue. ;)
How about changing timers to delayed works so that we can
make both sides happy? It is certainly much more work, but
looks worthy of it.
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] mld: fix panic in mld_newpack()
2020-12-26 19:27 ` Cong Wang
@ 2020-12-27 14:40 ` Taehee Yoo
2020-12-27 19:24 ` Cong Wang
0 siblings, 1 reply; 5+ messages in thread
From: Taehee Yoo @ 2020-12-27 14:40 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Jakub Kicinski, Linux Kernel Network Developers,
Eric Dumazet
On Sun, 27 Dec 2020 at 04:27, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
Hi Cong,
Thank you so much for the review!
> On Wed, Dec 23, 2020 at 8:55 AM Taehee Yoo <ap420073@gmail.com> wrote:
> >
> > mld_newpack() doesn't allow to allocate high order page,
> > just order-0 allocation is allowed.
> > If headroom size is too large, a kernel panic could occur in skb_put().
> ...
> > Allowing high order page allocation could fix this problem.
> >
> > Fixes: 72e09ad107e7 ("ipv6: avoid high order allocations")
>
> So you just revert this commit which fixes another issue. ;)
>
Yes, This patch is actually to revert 72e09ad107e7 commit.
But I found conflict while I do "git revert". so I just sent a normal patch :)
> How about changing timers to delayed works so that we can
> make both sides happy? It is certainly much more work, but
> looks worthy of it.
>
Thank you so much for your advice!
But I'm so sorry I didn't understand some points.
1. you said "both side" and I understand these as follows:
a) failure of allocation because of a high order and it is fixed
by 72e09ad107e7
b) kernel panic because of 72e09ad107e7
Are these two issues right?
2. So, as far as I understand your mention, these timers are
good to be changed to the delayed works And these timers are mca_timer,
mc_gq_timer, mc_ifc_timer, mc_dad_timer.
Do I understand your mention correctly?
If so, what is the benefit of it?
I, unfortunately, couldn't understand the relationship between changing
timers to the delayed works and these issues.
Could you please explain the above things again?
Thank you!
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] mld: fix panic in mld_newpack()
2020-12-27 14:40 ` Taehee Yoo
@ 2020-12-27 19:24 ` Cong Wang
2020-12-28 2:20 ` Taehee Yoo
0 siblings, 1 reply; 5+ messages in thread
From: Cong Wang @ 2020-12-27 19:24 UTC (permalink / raw)
To: Taehee Yoo
Cc: David Miller, Jakub Kicinski, Linux Kernel Network Developers,
Eric Dumazet
On Sun, Dec 27, 2020 at 6:40 AM Taehee Yoo <ap420073@gmail.com> wrote:
> But I'm so sorry I didn't understand some points.
>
> 1. you said "both side" and I understand these as follows:
> a) failure of allocation because of a high order and it is fixed
> by 72e09ad107e7
> b) kernel panic because of 72e09ad107e7
> Are these two issues right?
Yes, we can't fix one by reverting the fix for the other.
>
> 2. So, as far as I understand your mention, these timers are
> good to be changed to the delayed works And these timers are mca_timer,
> mc_gq_timer, mc_ifc_timer, mc_dad_timer.
> Do I understand your mention correctly?
> If so, what is the benefit of it?
> I, unfortunately, couldn't understand the relationship between changing
> timers to the delayed works and these issues.
Because a work has process context so we can use GFP_KERNEL
allocation rather than GFP_ATOMIC, which is what commit 72e09ad107e7
addresses.
Thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] mld: fix panic in mld_newpack()
2020-12-27 19:24 ` Cong Wang
@ 2020-12-28 2:20 ` Taehee Yoo
0 siblings, 0 replies; 5+ messages in thread
From: Taehee Yoo @ 2020-12-28 2:20 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Jakub Kicinski, Linux Kernel Network Developers,
Eric Dumazet
On Mon, 28 Dec 2020 at 04:24, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sun, Dec 27, 2020 at 6:40 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > But I'm so sorry I didn't understand some points.
> >
> > 1. you said "both side" and I understand these as follows:
> > a) failure of allocation because of a high order and it is fixed
> > by 72e09ad107e7
> > b) kernel panic because of 72e09ad107e7
> > Are these two issues right?
>
> Yes, we can't fix one by reverting the fix for the other.
>
> >
> > 2. So, as far as I understand your mention, these timers are
> > good to be changed to the delayed works And these timers are mca_timer,
> > mc_gq_timer, mc_ifc_timer, mc_dad_timer.
> > Do I understand your mention correctly?
> > If so, what is the benefit of it?
> > I, unfortunately, couldn't understand the relationship between changing
> > timers to the delayed works and these issues.
>
> Because a work has process context so we can use GFP_KERNEL
> allocation rather than GFP_ATOMIC, which is what commit 72e09ad107e7
> addresses.
>
Thank you for explaining!
I now understand why you suggested it.
I will send a v2 patch which will change timers to delay works.
Thanks a lot!
Taehee Yoo
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-12-28 2:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-23 16:52 [PATCH net] mld: fix panic in mld_newpack() Taehee Yoo
2020-12-26 19:27 ` Cong Wang
2020-12-27 14:40 ` Taehee Yoo
2020-12-27 19:24 ` Cong Wang
2020-12-28 2:20 ` Taehee Yoo
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.