| Message ID | 20221109014018.312181-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [PATCHv3,net] bonding: fix ICMPv6 header handling when receiving IPv6 messages | expand |
Hangbin Liu <liuhangbin@gmail.com> 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> Acked-by: Jay Vosburgh <jay.vosburgh@canonical.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) >+ 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; >-- >2.38.1 >
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) ? > + 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;
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. 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
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. > > 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
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
On Wed, Nov 9, 2022 at 1:23 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote: > > 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. Ah... I forgot about this, thanks for reminding me it ;) > > 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
On Wed, Nov 09, 2022 at 01:48:11PM -0800, Eric Dumazet wrote: > On Wed, Nov 9, 2022 at 1:23 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote: > > > > Eric Dumazet <edumazet@google.com> wrote: > > >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. > > Ah... I forgot about this, thanks for reminding me it ;) Hi David, The patch state[1] is Changes Requested, but I think Eric has no object on this patch now. Should I keep waiting, or re-send the patch? [1] https://patchwork.kernel.org/project/netdevbpf/patch/20221109014018.312181-1-liuhangbin@gmail.com/ Thanks Hangbin
Hangbin Liu <liuhangbin@gmail.com> wrote: >On Wed, Nov 09, 2022 at 01:48:11PM -0800, Eric Dumazet wrote: >> On Wed, Nov 9, 2022 at 1:23 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote: >> > >> > Eric Dumazet <edumazet@google.com> wrote: >> > >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. >> >> Ah... I forgot about this, thanks for reminding me it ;) > >Hi David, > >The patch state[1] is Changes Requested, but I think Eric has no object on this >patch now. Should I keep waiting, or re-send the patch? > >[1] https://patchwork.kernel.org/project/netdevbpf/patch/20221109014018.312181-1-liuhangbin@gmail.com/ The excerpt above is confirming that using skb_header_pointer() is the correct implementation to use. However, the patch needs to change to call skb_header_pointer() sooner, to insure that the IPv6 header is available. I've copied the relevant part of the discussion below: >> 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) ? The above comment is from Eric. I had also mentioned that this particular problem already existed in the code being patched. -J --- -Jay Vosburgh, jay.vosburgh@canonical.com
On Wed, Nov 16, 2022 at 07:16:14AM -0800, Jay Vosburgh wrote: > >Hi David, > > > >The patch state[1] is Changes Requested, but I think Eric has no object on this > >patch now. Should I keep waiting, or re-send the patch? > > > >[1] https://patchwork.kernel.org/project/netdevbpf/patch/20221109014018.312181-1-liuhangbin@gmail.com/ > > The excerpt above is confirming that using skb_header_pointer() > is the correct implementation to use. > > However, the patch needs to change to call skb_header_pointer() > sooner, to insure that the IPv6 header is available. I've copied the > relevant part of the discussion below: > > >> 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) ? > > The above comment is from Eric. I had also mentioned that this > particular problem already existed in the code being patched. Yes, I also saw your comments. I was thinking to fix this issue separately. i.e. in bond_rcv_validate(). With this we can check both IPv6 header and ARP header. e.g. diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 2c6356232668..ae4c30a25b76 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -3278,8 +3278,10 @@ int bond_rcv_validate(const struct sk_buff *skb, struct bonding *bond, { #if IS_ENABLED(CONFIG_IPV6) bool is_ipv6 = skb->protocol == __cpu_to_be16(ETH_P_IPV6); + struct ipv6hdr ip6_hdr; #endif bool is_arp = skb->protocol == __cpu_to_be16(ETH_P_ARP); + struct arphdr arp_hdr; slave_dbg(bond->dev, slave->dev, "%s: skb->dev %s\n", __func__, skb->dev->name); @@ -3293,10 +3295,10 @@ int bond_rcv_validate(const struct sk_buff *skb, struct bonding *bond, !slave_do_arp_validate_only(bond)) slave->last_rx = jiffies; return RX_HANDLER_ANOTHER; - } else if (is_arp) { + } else if (is_arp && skb_header_pointer(skb, 0, sizeof(arp_hdr), &arp_hdr)) { return bond_arp_rcv(skb, bond, slave); #if IS_ENABLED(CONFIG_IPV6) - } else if (is_ipv6) { + } else if (is_ipv6 && skb_header_pointer(skb, 0, sizeof(ip6_hdr), &ip6_hdr)) { return bond_na_rcv(skb, bond, slave); #endif } else { What do you think? Thanks Hangbin
Hangbin Liu <liuhangbin@gmail.com> wrote: >On Wed, Nov 16, 2022 at 07:16:14AM -0800, Jay Vosburgh wrote: [...] >> The above comment is from Eric. I had also mentioned that this >> particular problem already existed in the code being patched. > >Yes, I also saw your comments. I was thinking to fix this issue separately. >i.e. in bond_rcv_validate(). With this we can check both IPv6 header and ARP >header. e.g. > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index 2c6356232668..ae4c30a25b76 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -3278,8 +3278,10 @@ int bond_rcv_validate(const struct sk_buff *skb, struct bonding *bond, > { > #if IS_ENABLED(CONFIG_IPV6) > bool is_ipv6 = skb->protocol == __cpu_to_be16(ETH_P_IPV6); >+ struct ipv6hdr ip6_hdr; > #endif > bool is_arp = skb->protocol == __cpu_to_be16(ETH_P_ARP); >+ struct arphdr arp_hdr; > > slave_dbg(bond->dev, slave->dev, "%s: skb->dev %s\n", > __func__, skb->dev->name); >@@ -3293,10 +3295,10 @@ int bond_rcv_validate(const struct sk_buff *skb, struct bonding *bond, > !slave_do_arp_validate_only(bond)) > slave->last_rx = jiffies; > return RX_HANDLER_ANOTHER; >- } else if (is_arp) { >+ } else if (is_arp && skb_header_pointer(skb, 0, sizeof(arp_hdr), &arp_hdr)) { > return bond_arp_rcv(skb, bond, slave); > #if IS_ENABLED(CONFIG_IPV6) >- } else if (is_ipv6) { >+ } else if (is_ipv6 && skb_header_pointer(skb, 0, sizeof(ip6_hdr), &ip6_hdr)) { > return bond_na_rcv(skb, bond, slave); > #endif > } else { > >What do you think? I don't see how this solves the icmp6_hdr() / ipv6_hdr() problem in bond_na_rcv(); skb_header_pointer() doesn't do a pull, it just copies into the supplied struct (if necessary). -J --- -Jay Vosburgh, jay.vosburgh@canonical.com
On Wed, Nov 16, 2022 at 08:29:58PM -0800, Jay Vosburgh wrote: > > #if IS_ENABLED(CONFIG_IPV6) > >- } else if (is_ipv6) { > >+ } else if (is_ipv6 && skb_header_pointer(skb, 0, sizeof(ip6_hdr), &ip6_hdr)) { > > return bond_na_rcv(skb, bond, slave); > > #endif > > } else { > > > >What do you think? > > I don't see how this solves the icmp6_hdr() / ipv6_hdr() problem > in bond_na_rcv(); skb_header_pointer() doesn't do a pull, it just copies > into the supplied struct (if necessary). Hmm... Maybe I didn't get what you and Eric means. If we can copy the supplied buffer success, doesn't this make sure IPv6 header is in skb? Thanks Hangbin
On 11/17/22 00:34, Hangbin Liu wrote: > On Wed, Nov 16, 2022 at 08:29:58PM -0800, Jay Vosburgh wrote: >>> #if IS_ENABLED(CONFIG_IPV6) >>> - } else if (is_ipv6) { >>> + } else if (is_ipv6 && skb_header_pointer(skb, 0, sizeof(ip6_hdr), &ip6_hdr)) { >>> return bond_na_rcv(skb, bond, slave); >>> #endif >>> } else { >>> >>> What do you think? >> I don't see how this solves the icmp6_hdr() / ipv6_hdr() problem >> in bond_na_rcv(); skb_header_pointer() doesn't do a pull, it just copies >> into the supplied struct (if necessary). > Hmm... Maybe I didn't get what you and Eric means. If we can copy the > supplied buffer success, doesn't this make sure IPv6 header is in skb? Please try : diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 1cd4e71916f80876ca56eb778f8423aa04c80684..c4bdc707c62c4a29c3e16ec4ad523feae00447cb 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -3224,16 +3224,24 @@ 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; + struct { + struct ipv6hdr ip6; + struct icmp6hdr icmp6; + } *combined, _combined; if (skb->pkt_type == PACKET_OTHERHOST || - skb->pkt_type == PACKET_LOOPBACK || - hdr->icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT) + skb->pkt_type == PACKET_LOOPBACK) + goto out; + + combined = skb_header_pointer(skb, 0, sizeof(_combined), &_combined); + if (!combined || + combined->ip6.nexthdr != NEXTHDR_ICMP || + combined->icmp6.icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT) goto out; - saddr = &ipv6_hdr(skb)->saddr; - daddr = &ipv6_hdr(skb)->daddr; + saddr = &combined->ip6.saddr; + daddr = &combined->ip6.daddr; slave_dbg(bond->dev, slave->dev, "%s: %s/%d av %d sv %d sip %pI6c tip %pI6c\n", __func__, slave->dev->name, bond_slave_state(slave),
Hangbin Liu <liuhangbin@gmail.com> wrote: >On Wed, Nov 16, 2022 at 08:29:58PM -0800, Jay Vosburgh wrote: >> > #if IS_ENABLED(CONFIG_IPV6) >> >- } else if (is_ipv6) { >> >+ } else if (is_ipv6 && skb_header_pointer(skb, 0, sizeof(ip6_hdr), &ip6_hdr)) { >> > return bond_na_rcv(skb, bond, slave); >> > #endif >> > } else { >> > >> >What do you think? >> >> I don't see how this solves the icmp6_hdr() / ipv6_hdr() problem >> in bond_na_rcv(); skb_header_pointer() doesn't do a pull, it just copies >> into the supplied struct (if necessary). > >Hmm... Maybe I didn't get what you and Eric means. If we can copy the >supplied buffer success, doesn't this make sure IPv6 header is in skb? The header is in the skb, but it may not be in the linear part of the skb, i.e., the header is wholly or partially in a skb frag, not in the area covered by skb->data ... skb->tail. The various *_hdr() macros only look in the linear area, not the frags, and don't check to see if the linear area contains the entire header. skb_header_pointer() is smart enough to check, and if the requested data is entirely within the linear area, it returns a pointer to there; if not, it copies from the frags into the supplied struct and returns a pointer to that. What it doesn't do is a pull (move data from a frag into the linear area), so merely calling skb_header_pointer() doesn't affect the layout of what's in the skb (which is the point, bonding uses it here to avoid changing the skb). There may be better explanations out there, but http://vger.kernel.org/~davem/skb_data.html covers the basics. Look for the references to "paged data." -J --- -Jay Vosburgh, jay.vosburgh@canonical.com
On Thu, Nov 17, 2022 at 12:04:11PM -0800, Jay Vosburgh wrote: > Hangbin Liu <liuhangbin@gmail.com> wrote: > > >On Wed, Nov 16, 2022 at 08:29:58PM -0800, Jay Vosburgh wrote: > >> > #if IS_ENABLED(CONFIG_IPV6) > >> >- } else if (is_ipv6) { > >> >+ } else if (is_ipv6 && skb_header_pointer(skb, 0, sizeof(ip6_hdr), &ip6_hdr)) { > >> > return bond_na_rcv(skb, bond, slave); > >> > #endif > >> > } else { > >> > > >> >What do you think? > >> > >> I don't see how this solves the icmp6_hdr() / ipv6_hdr() problem > >> in bond_na_rcv(); skb_header_pointer() doesn't do a pull, it just copies > >> into the supplied struct (if necessary). > > > >Hmm... Maybe I didn't get what you and Eric means. If we can copy the > >supplied buffer success, doesn't this make sure IPv6 header is in skb? > > The header is in the skb, but it may not be in the linear part > of the skb, i.e., the header is wholly or partially in a skb frag, not > in the area covered by skb->data ... skb->tail. The various *_hdr() > macros only look in the linear area, not the frags, and don't check to > see if the linear area contains the entire header. > > skb_header_pointer() is smart enough to check, and if the > requested data is entirely within the linear area, it returns a pointer > to there; if not, it copies from the frags into the supplied struct and > returns a pointer to that. What it doesn't do is a pull (move data from > a frag into the linear area), so merely calling skb_header_pointer() > doesn't affect the layout of what's in the skb (which is the point, > bonding uses it here to avoid changing the skb). > > There may be better explanations out there, but > > http://vger.kernel.org/~davem/skb_data.html > > covers the basics. Look for the references to "paged data." Hi Jay, Thanks a lot for your explanation. I thought you mean there may no IPv6 header in skb. Now I know the problem is using ipv6_hdr() for non-liner skb. I will update the patch with Eric suggested. Thanks Hangbin
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) + 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;
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(-)