[PATCH net] bonding: fix ICMPv6 header handling when receiving IPv6 messages

[PATCH net] bonding: fix ICMPv6 header handling when receiving IPv6 messages

[Hangbin Liu]

Some drivers, like bnx2x, will call ipv6_gro_receive() and set skb
IPv6 transport header before bond_handle_frame(). But some other drivers,
like be2net, will not call ipv6_gro_receive() and skb transport header
is not set properly. Thus we can't use icmp6_hdr(skb) to get the icmp6
header directly when dealing with IPv6 messages.

Fix this by checking the skb length manually and getting icmp6 header based
on the IPv6 header offset.

Reported-by: Liang Li <liali@redhat.com>
Fixes: 4e24be018eb9 ("bonding: add new parameter ns_targets")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e84c49bf4d0c..08b5f512f5fb 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3231,12 +3231,23 @@ static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
 		       struct slave *slave)
 {
 	struct slave *curr_active_slave, *curr_arp_slave;
-	struct icmp6hdr *hdr = icmp6_hdr(skb);
+	const struct icmp6hdr *icmp6_hdr;
 	struct in6_addr *saddr, *daddr;
+	const struct ipv6hdr *hdr;
+	u16 pkt_len;
+
+	/* Check if the length is enough manually as we can't use pskb_may_pull */
+	hdr = ipv6_hdr(skb);
+	pkt_len = ntohs(hdr->payload_len);
+	if (hdr->nexthdr != NEXTHDR_ICMP || pkt_len < sizeof(*icmp6_hdr) ||
+	    skb_headlen(skb) < sizeof(*hdr) + pkt_len)
+		goto out;
+
+	icmp6_hdr = (const struct icmp6hdr *)(skb->data + sizeof(*hdr));
 
 	if (skb->pkt_type == PACKET_OTHERHOST ||
 	    skb->pkt_type == PACKET_LOOPBACK ||
-	    hdr->icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT)
+	    icmp6_hdr->icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT)
 		goto out;
 
 	saddr = &ipv6_hdr(skb)->saddr;
-- 
2.38.1

Re: [PATCH net] bonding: fix ICMPv6 header handling when receiving IPv6 messages

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>Some drivers, like bnx2x, will call ipv6_gro_receive() and set skb
>IPv6 transport header before bond_handle_frame(). But some other drivers,
>like be2net, will not call ipv6_gro_receive() and skb transport header
>is not set properly. Thus we can't use icmp6_hdr(skb) to get the icmp6
>header directly when dealing with IPv6 messages.

	I don't understand this explanation, as ipv6_gro_receive() isn't
called directly by the device drivers, but from within the GRO
processing, e.g., by dev_gro_receive().

	Could you explain how the call paths actually differ?  

	-J

>Fix this by checking the skb length manually and getting icmp6 header based
>on the IPv6 header offset.
>
>Reported-by: Liang Li <liali@redhat.com>
>Fixes: 4e24be018eb9 ("bonding: add new parameter ns_targets")
>Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>---
> drivers/net/bonding/bond_main.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index e84c49bf4d0c..08b5f512f5fb 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3231,12 +3231,23 @@ static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
> 		       struct slave *slave)
> {
> 	struct slave *curr_active_slave, *curr_arp_slave;
>-	struct icmp6hdr *hdr = icmp6_hdr(skb);
>+	const struct icmp6hdr *icmp6_hdr;
> 	struct in6_addr *saddr, *daddr;
>+	const struct ipv6hdr *hdr;
>+	u16 pkt_len;
>+
>+	/* Check if the length is enough manually as we can't use pskb_may_pull */
>+	hdr = ipv6_hdr(skb);
>+	pkt_len = ntohs(hdr->payload_len);
>+	if (hdr->nexthdr != NEXTHDR_ICMP || pkt_len < sizeof(*icmp6_hdr) ||
>+	    skb_headlen(skb) < sizeof(*hdr) + pkt_len)
>+		goto out;
>+
>+	icmp6_hdr = (const struct icmp6hdr *)(skb->data + sizeof(*hdr));
> 
> 	if (skb->pkt_type == PACKET_OTHERHOST ||
> 	    skb->pkt_type == PACKET_LOOPBACK ||
>-	    hdr->icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT)
>+	    icmp6_hdr->icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT)
> 		goto out;
> 
> 	saddr = &ipv6_hdr(skb)->saddr;
>-- 
>2.38.1
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] bonding: fix ICMPv6 header handling when receiving IPv6 messages

[Hangbin Liu]

On Tue, Nov 01, 2022 at 11:12:43AM +0100, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> >Some drivers, like bnx2x, will call ipv6_gro_receive() and set skb
> >IPv6 transport header before bond_handle_frame(). But some other drivers,
> >like be2net, will not call ipv6_gro_receive() and skb transport header
> >is not set properly. Thus we can't use icmp6_hdr(skb) to get the icmp6
> >header directly when dealing with IPv6 messages.
> 
> 	I don't understand this explanation, as ipv6_gro_receive() isn't
> called directly by the device drivers, but from within the GRO
> processing, e.g., by dev_gro_receive().
> 
> 	Could you explain how the call paths actually differ?  

Er..Yes, it's a little weird.

I checked if the transport header is set before  __netif_receive_skb_core().
The bnx2x driver set it while be2net does not. So the transport header is reset
in __netif_receive_skb_core() with be2net.

I also found ipv6_gro_receive() is called before bond_handle_frame() when
receive NA message. Not sure which path it go through. I'm not very familiar
with driver part. But I can do more investigating.

Thanks
Hangbin

Re: [PATCH net] bonding: fix ICMPv6 header handling when receiving IPv6 messages

[Hangbin Liu]

On Tue, Nov 01, 2022 at 09:39:22PM +0800, Hangbin Liu wrote:
> > 	I don't understand this explanation, as ipv6_gro_receive() isn't
> > called directly by the device drivers, but from within the GRO
> > processing, e.g., by dev_gro_receive().
> > 
> > 	Could you explain how the call paths actually differ?  
> 
> Er..Yes, it's a little weird.
> 
> I checked if the transport header is set before  __netif_receive_skb_core().
> The bnx2x driver set it while be2net does not. So the transport header is reset
> in __netif_receive_skb_core() with be2net.
> 
> I also found ipv6_gro_receive() is called before bond_handle_frame() when
> receive NA message. Not sure which path it go through. I'm not very familiar
> with driver part. But I can do more investigating.

With dump_stack(), it shows bnx2x do calls ipv6_gro_receive().
PS: I only dump the stack when receive NA.

[   65.537605]  dump_stack_lvl+0x34/0x48
[   65.541695]  ipv6_gro_receive.cold+0x1b/0x3d
[   65.546453]  dev_gro_receive+0x16c/0x380
[   65.550831]  napi_gro_receive+0x64/0x210
[   65.555206]  bnx2x_rx_int+0x44c/0x820 [bnx2x]
[   65.560100]  bnx2x_poll+0xe5/0x1d0 [bnx2x]
[   65.564687]  __napi_poll+0x2c/0x160
[   65.568579]  net_rx_action+0x296/0x350

Thanks
Hangbin

Re: [PATCH net] bonding: fix ICMPv6 header handling when receiving IPv6 messages

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Tue, Nov 01, 2022 at 09:39:22PM +0800, Hangbin Liu wrote:
>> > 	I don't understand this explanation, as ipv6_gro_receive() isn't
>> > called directly by the device drivers, but from within the GRO
>> > processing, e.g., by dev_gro_receive().
>> > 
>> > 	Could you explain how the call paths actually differ?  
>> 
>> Er..Yes, it's a little weird.
>> 
>> I checked if the transport header is set before  __netif_receive_skb_core().
>> The bnx2x driver set it while be2net does not. So the transport header is reset
>> in __netif_receive_skb_core() with be2net.
>> 
>> I also found ipv6_gro_receive() is called before bond_handle_frame() when
>> receive NA message. Not sure which path it go through. I'm not very familiar
>> with driver part. But I can do more investigating.

	I suspect that what you're seeing is caused by bnx2x calling
skb_set_transport_header() in bnx2x_gro_ipv6_csum() to explicitly set
the transport header for IPv6, and benet having no equivalent call.  If
benet were to set the transport header, I think it would happen in
be_rx_compl_process_gro().

	__netif_receive_skb_core() calls skb_reset_transport_header() if
the transport header isn't set, but I presume that doesn't do the right
thing for ICMPv6.

	I don't believe there's any expectation that drivers must set
the transport header at this point, so I tentatively think that what
your patch is trying to do is reasonable.

	Briefly looking at the patch, the commit message needs updating,
and I'm curious to know why pskb_may_pull can't be used.

	-J

>With dump_stack(), it shows bnx2x do calls ipv6_gro_receive().
>PS: I only dump the stack when receive NA.
>
>[   65.537605]  dump_stack_lvl+0x34/0x48
>[   65.541695]  ipv6_gro_receive.cold+0x1b/0x3d
>[   65.546453]  dev_gro_receive+0x16c/0x380
>[   65.550831]  napi_gro_receive+0x64/0x210
>[   65.555206]  bnx2x_rx_int+0x44c/0x820 [bnx2x]
>[   65.560100]  bnx2x_poll+0xe5/0x1d0 [bnx2x]
>[   65.564687]  __napi_poll+0x2c/0x160
>[   65.568579]  net_rx_action+0x296/0x350
>
>Thanks
>Hangbin

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] bonding: fix ICMPv6 header handling when receiving IPv6 messages

[Hangbin Liu]

On Thu, Nov 03, 2022 at 05:03:59PM +0100, Jay Vosburgh wrote:
> 	Briefly looking at the patch, the commit message needs updating,
> and I'm curious to know why pskb_may_pull can't be used.

Oh, forgot to reply this. pskb_may_pull() need "struct sk_buff *skb" but we
defined "const struct sk_buff *skb" in bond_na_rcv().

Thanks
Hangbin

Re: [PATCH net] bonding: fix ICMPv6 header handling when receiving IPv6 messages

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

>On Thu, Nov 03, 2022 at 05:03:59PM +0100, Jay Vosburgh wrote:
>> 	Briefly looking at the patch, the commit message needs updating,
>> and I'm curious to know why pskb_may_pull can't be used.
>
>Oh, forgot to reply this. pskb_may_pull() need "struct sk_buff *skb" but we
>defined "const struct sk_buff *skb" in bond_na_rcv().

	Perhaps you could use skb_header_pointer(), similarly to what is
done in bond_3ad_lacpdu_recv() or rlb_arp_recv()?

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] bonding: fix ICMPv6 header handling when receiving IPv6 messages

[Hangbin Liu]

On Fri, Nov 04, 2022 at 09:18:52AM +0100, Jay Vosburgh wrote:
> Hangbin Liu <liuhangbin@gmail.com> wrote:
> 
> >On Thu, Nov 03, 2022 at 05:03:59PM +0100, Jay Vosburgh wrote:
> >> 	Briefly looking at the patch, the commit message needs updating,
> >> and I'm curious to know why pskb_may_pull can't be used.
> >
> >Oh, forgot to reply this. pskb_may_pull() need "struct sk_buff *skb" but we
> >defined "const struct sk_buff *skb" in bond_na_rcv().
> 
> 	Perhaps you could use skb_header_pointer(), similarly to what is
> done in bond_3ad_lacpdu_recv() or rlb_arp_recv()?

Cool, Thanks Jay. This saves my lumbering checking.

Cheers
Hangbin