Re: [PATCH net] bonding: fix skb_under_panic in bond_ns_send() over stacked VLANs

[Jay Vosburgh <jv@jvosburgh.net>]

Xiang Mei (Microsoft) <xmei5@asu.edu> wrote:

>bond_ns_send() builds an IPv6 Neighbor Solicitation with
>ndisc_ns_create(), which reserves exactly LL_RESERVED_SPACE(dev) +
>sizeof(struct ipv6hdr) of headroom for the later ip6_nd_hdr() push.
>bond_handle_vlan() then inserts the collected VLAN tags into the skb;
>each inner tag consumes VLAN_HLEN of that headroom via skb_push(). With
>enough stacked VLAN devices between the bond and the ns_ip6_target, the
>reserved IPv6 headroom is exhausted, so the subsequent
>ndisc_send_skb() -> ip6_nd_hdr() -> skb_push(sizeof(struct ipv6hdr))
>underflows past skb->head and hits skb_under_panic().

	How many stacked VLANs is "enough"?  I'm guessing that it's
somewhat device dependent, as LL_RESERVED_SPACE includes
dev->needed_headroom, but as a ballpark here, was it more in the realm
of 3, or 30?

	Also, why doesn't the skb_push called via bond_handle_vlan ->
vlan_insert_tag_set_proto -> vlan_insert_tag -> vlan_insert_inner_tag ->
__vlan_inser_inner_tag trigger the skb_under_panic?

	Does adding one or two more nested VLANs move the panic into the
above call path?

>Restore the required headroom with skb_cow_head() after VLAN insertion
>and before handing the skb to ndisc_send_skb(); drop the probe on
>allocation failure. For paths that did not exhaust the headroom this is
>a no-op, so previously working configurations are unaffected.
>
>  skbuff: skb_under_panic: len:84 put:40 head:... data:... tail:0x50 end:0x180 dev:veth0
>  kernel BUG at net/core/skbuff.c:214!

	The above text doesn't seem to match with current net-next:

        pr_emerg("%s: text:%px len:%d put:%d head:%px data:%px tail:%#lx end:%#l
x dev:%s\n",

	I presume you replaced the "head:" and "data:" values with
"..."; did you also edit out the "text:" that should precede "len:"?

	I'm wondering because without the actual values, it's not clear
how far beyond head the data pointer went, and thus whether adding more
VLANs would move the failure into the VLAN tag code.

	-J

>  Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
>  Workqueue: bond0 bond_arp_monitor
>  RIP: 0010:skb_panic+0x142/0x230
>  Call Trace:
>   skb_push (net/core/skbuff.c:224)
>   ndisc_send_skb (net/ipv6/ndisc.c:454 net/ipv6/ndisc.c:506)
>   bond_ns_send (drivers/net/bonding/bond_main.c:3255)
>   bond_ns_send_all (drivers/net/bonding/bond_main.c:3313)
>   bond_arp_monitor (drivers/net/bonding/bond_main.c:3458)
>   process_one_work (kernel/workqueue.c:3322)
>   worker_thread (kernel/workqueue.c:3405)
>   kthread (kernel/kthread.c:436)
>  Kernel panic - not syncing: Fatal exception
>
>Fixes: 4e24be018eb9 ("bonding: add new parameter ns_targets")
>Reported-by: AutonomousCodeSecurity@microsoft.com
>Signed-off-by: Xiang Mei (Microsoft) <xmei5@asu.edu>
>---
> drivers/net/bonding/bond_main.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index e044fc733b8c..3ac3418c9498 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3251,6 +3251,10 @@ static void bond_ns_send(struct slave *slave, const struct in6_addr *daddr,
> 
> 	addrconf_addr_solict_mult(daddr, &mcaddr);
> 	if (bond_handle_vlan(slave, tags, skb)) {
>+		if (skb_cow_head(skb, sizeof(struct ipv6hdr))) {
>+			kfree_skb(skb);
>+			return;
>+		}
> 		slave_update_last_tx(slave);
> 		ndisc_send_skb(skb, &mcaddr, saddr);
> 	}
>-- 
>2.43.0
>

---
	-Jay Vosburgh, jv@jvosburgh.net