[PATCH net] mld: fix panic in mld_newpack()

[PATCH net] mld: fix panic in mld_newpack()

[Taehee Yoo]

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

Re: [PATCH net] mld: fix panic in mld_newpack()

[Cong Wang]

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.

Re: [PATCH net] mld: fix panic in mld_newpack()

[Taehee Yoo]

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!

Re: [PATCH net] mld: fix panic in mld_newpack()

[Cong Wang]

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.

Re: [PATCH net] mld: fix panic in mld_newpack()

[Taehee Yoo]

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