diff mbox series

[net] mld: fix panic in mld_newpack()

Message ID 20201223165250.14505-1-ap420073@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net] mld: fix panic in mld_newpack() | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers warning 2 maintainers not CCed: yoshfuji@linux-ipv6.org kuznet@ms2.inr.ac.ru
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 11 this patch: 11
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 10 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 11 this patch: 11
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Taehee Yoo Dec. 23, 2020, 4:52 p.m. UTC
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(-)

Comments

Cong Wang Dec. 26, 2020, 7:27 p.m. UTC | #1
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.
Taehee Yoo Dec. 27, 2020, 2:40 p.m. UTC | #2
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!
Cong Wang Dec. 27, 2020, 7:24 p.m. UTC | #3
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.
Taehee Yoo Dec. 28, 2020, 2:20 a.m. UTC | #4
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
diff mbox series

Patch

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;