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

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Eric Dumazet <edumazet@google.com> wrote:

>On Wed, Nov 9, 2022 at 12:39 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>>
>> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>> >
>> >
>> >On 11/8/22 17:40, Hangbin Liu wrote:
>> >> Currently, we get icmp6hdr via function icmp6_hdr(), which needs the skb
>> >> transport header to be set first. But there is no rule to ask driver set
>> >> transport header before netif_receive_skb() and bond_handle_frame(). So
>> >> we will not able to get correct icmp6hdr on some drivers.
>> >>
>> >> 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")
>> >> Acked-by: Jonathan Toppins <jtoppins@redhat.com>
>> >> Reviewed-by: David Ahern <dsahern@kernel.org>
>> >> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
>> >> ---
>> >> v3: fix _hdr parameter warning reported by kernel test robot
>> >> v2: use skb_header_pointer() to get icmp6hdr as Jay suggested.
>> >> ---
>> >>   drivers/net/bonding/bond_main.c | 9 +++++++--
>> >>   1 file changed, 7 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >> index e84c49bf4d0c..2c6356232668 100644
>> >> --- a/drivers/net/bonding/bond_main.c
>> >> +++ b/drivers/net/bonding/bond_main.c
>> >> @@ -3231,12 +3231,17 @@ 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);
>> >>      struct in6_addr *saddr, *daddr;
>> >> +    const struct icmp6hdr *hdr;
>> >> +    struct icmp6hdr _hdr;
>> >>      if (skb->pkt_type == PACKET_OTHERHOST ||
>> >>          skb->pkt_type == PACKET_LOOPBACK ||
>> >> -        hdr->icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT)
>> >> +        ipv6_hdr(skb)->nexthdr != NEXTHDR_ICMP)
>> >
>> >
>> >What makes sure IPv6 header is in skb->head (linear part of the skb) ?
>>
>>         Ah, missed that; skb_header_pointer() will take care of that
>> (copying if necessary, not that it pulls the header), but it has to be
>> called first.
>>
>>         This isn't a problem new to this patch, the original code
>> doesn't pull or copy the header, either.
>
>Quite frankly I would simply use
>
>if (pskb_may_pull(skb, sizeof(struct ipv6hdr) + sizeof(struct icmp6hdr))
> instead of  skb_header_pointer()
>because chances are high we will need the whole thing in skb->head later.

	Well, it was set up this way with skb_header_pointer() instead
of pskb_may_pull() by you in de063b7040dc ("bonding: remove packet
cloning in recv_probe()") so the bonding rx_handler wouldn't change or
clone the skb.  Now, I'm not sure if we should follow your advice to go
against your advice.

	Also, we'd have to un-const the skb parameter through the call
chain from bond_handle_frame().

	-J

>>
>>         The equivalent function for ARP, bond_arp_rcv(), more or less
>> inlines skb_header_pointer(), so it doesn't have this issue.
>>
>>         -J
>>
>> >
>> >> +            goto out;
>> >> +
>> >> +    hdr = skb_header_pointer(skb, sizeof(struct ipv6hdr), sizeof(_hdr), &_hdr);
>> >> +    if (!hdr || hdr->icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT)
>> >>              goto out;
>> >>      saddr = &ipv6_hdr(skb)->saddr;

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