| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <196743.1752026655@famine>
Date: Tue, 08 Jul 2025 19:04:15 -0700
From: Jay Vosburgh <jv@...sburgh.net>
To: Tonghao Zhang <tonghao@...aicloud.com>
cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
Jonathan Corbet <corbet@....net>,
Andrew Lunn <andrew+netdev@...n.ch>,
Nikolay Aleksandrov <razor@...ckwall.org>,
Zengbing Tu <tuzengbing@...iglobal.com>
Subject: Re: [PATCH net-next RESEND] net: bonding: add bond_is_icmpv6_nd()
helper
Tonghao Zhang <tonghao@...aicloud.com> wrote:
>Introduce ipv6 ns/nd checking helper, using skb_header_pointer()
>instead of pskb_network_may_pull() on tx path.
>
>alb_determine_nd introduced from commit 0da8aa00bfcfe
>
>Cc: Jay Vosburgh <jv@...sburgh.net>
>Cc: "David S. Miller" <davem@...emloft.net>
>Cc: Eric Dumazet <edumazet@...gle.com>
>Cc: Jakub Kicinski <kuba@...nel.org>
>Cc: Paolo Abeni <pabeni@...hat.com>
>Cc: Simon Horman <horms@...nel.org>
>Cc: Jonathan Corbet <corbet@....net>
>Cc: Andrew Lunn <andrew+netdev@...n.ch>
>Cc: Nikolay Aleksandrov <razor@...ckwall.org>
>Signed-off-by: Tonghao Zhang <tonghao@...aicloud.com>
>Signed-off-by: Zengbing Tu <tuzengbing@...iglobal.com>
>---
> drivers/net/bonding/bond_alb.c | 32 +++++++-------------------------
> drivers/net/bonding/bond_main.c | 17 ++---------------
> include/net/bonding.h | 19 +++++++++++++++++++
> 3 files changed, 28 insertions(+), 40 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index 2d37b07c8215..8e5b9ce52077 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -19,7 +19,6 @@
> #include <linux/in.h>
> #include <net/arp.h>
> #include <net/ipv6.h>
>-#include <net/ndisc.h>
> #include <asm/byteorder.h>
> #include <net/bonding.h>
> #include <net/bond_alb.h>
>@@ -1280,27 +1279,6 @@ static int alb_set_mac_address(struct bonding *bond, void *addr)
> return res;
> }
>
>-/* determine if the packet is NA or NS */
>-static bool alb_determine_nd(struct sk_buff *skb, struct bonding *bond)
>-{
>- struct ipv6hdr *ip6hdr;
>- struct icmp6hdr *hdr;
>-
>- if (!pskb_network_may_pull(skb, sizeof(*ip6hdr)))
>- return true;
>-
>- ip6hdr = ipv6_hdr(skb);
>- if (ip6hdr->nexthdr != IPPROTO_ICMPV6)
>- return false;
>-
>- if (!pskb_network_may_pull(skb, sizeof(*ip6hdr) + sizeof(*hdr)))
>- return true;
>-
>- hdr = icmp6_hdr(skb);
>- return hdr->icmp6_type == NDISC_NEIGHBOUR_ADVERTISEMENT ||
>- hdr->icmp6_type == NDISC_NEIGHBOUR_SOLICITATION;
>-}
>-
> /************************ exported alb functions ************************/
>
> int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>@@ -1381,7 +1359,7 @@ struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
> if (!is_multicast_ether_addr(eth_data->h_dest)) {
> switch (skb->protocol) {
> case htons(ETH_P_IPV6):
>- if (alb_determine_nd(skb, bond))
>+ if (bond_is_icmpv6_nd(skb))
> break;
> fallthrough;
> case htons(ETH_P_IP):
>@@ -1467,16 +1445,20 @@ struct slave *bond_xmit_alb_slave_get(struct bonding *bond,
> break;
> }
>
>- if (alb_determine_nd(skb, bond)) {
>+ if (bond_is_icmpv6_nd(skb)) {
> do_tx_balance = false;
> break;
> }
>
>- /* The IPv6 header is pulled by alb_determine_nd */
> /* Additionally, DAD probes should not be tx-balanced as that
> * will lead to false positives for duplicate addresses and
> * prevent address configuration from working.
> */
>+ if (!pskb_network_may_pull(skb, sizeof(*ip6hdr))) {
>+ do_tx_balance = false;
>+ break;
>+ }
>+
It's nice to consolidate some duplicate code and use the more
efficient skb_header_pointer, but the above will do a pull anyway for
nearly every packet that passed through the function.
Would it be better to not use the bond_is_icmpv6_nd helper here,
but instead call skb_header_pointer directly, and then reuse its
returned data for the hash computation that occurs just after this point
in the code?
> ip6hdr = ipv6_hdr(skb);
> if (ipv6_addr_any(&ip6hdr->saddr)) {
> do_tx_balance = false;
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 17c7542be6a5..a8034a561011 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -5338,10 +5338,6 @@ static bool bond_should_broadcast_neighbor(struct sk_buff *skb,
> struct net_device *dev)
> {
> struct bonding *bond = netdev_priv(dev);
>- struct {
>- struct ipv6hdr ip6;
>- struct icmp6hdr icmp6;
>- } *combined, _combined;
>
> if (!static_branch_unlikely(&bond_bcast_neigh_enabled))
> return false;
>@@ -5349,19 +5345,10 @@ static bool bond_should_broadcast_neighbor(struct sk_buff *skb,
> if (!bond->params.broadcast_neighbor)
> return false;
>
>- if (skb->protocol == htons(ETH_P_ARP))
>+ if (skb->protocol == htons(ETH_P_ARP) ||
>+ (skb->protocol == htons(ETH_P_IPV6) && bond_is_icmpv6_nd(skb)))
> return true;
>
>- if (skb->protocol == htons(ETH_P_IPV6)) {
>- combined = skb_header_pointer(skb, skb_mac_header_len(skb),
>- sizeof(_combined),
>- &_combined);
>- if (combined && combined->ip6.nexthdr == NEXTHDR_ICMP &&
>- (combined->icmp6.icmp6_type == NDISC_NEIGHBOUR_SOLICITATION ||
>- combined->icmp6.icmp6_type == NDISC_NEIGHBOUR_ADVERTISEMENT))
>- return true;
>- }
>-
> return false;
> }
>
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index e06f0d63b2c1..32d9fcca858c 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -29,6 +29,7 @@
> #include <net/bond_options.h>
> #include <net/ipv6.h>
> #include <net/addrconf.h>
>+#include <net/ndisc.h>
>
> #define BOND_MAX_ARP_TARGETS 16
> #define BOND_MAX_NS_TARGETS BOND_MAX_ARP_TARGETS
>@@ -814,4 +815,22 @@ static inline netdev_tx_t bond_tx_drop(struct net_device *dev, struct sk_buff *s
> return NET_XMIT_DROP;
> }
>
>+static inline bool bond_is_icmpv6_nd(struct sk_buff *skb)
>+{
Minor nit: no need to specify inline, the compiler will decide.
-J
>+ struct {
>+ struct ipv6hdr ip6;
>+ struct icmp6hdr icmp6;
>+ } *combined, _combined;
>+
>+ combined = skb_header_pointer(skb, skb_mac_header_len(skb),
>+ sizeof(_combined),
>+ &_combined);
>+ if (combined && combined->ip6.nexthdr == NEXTHDR_ICMP &&
>+ (combined->icmp6.icmp6_type == NDISC_NEIGHBOUR_SOLICITATION ||
>+ combined->icmp6.icmp6_type == NDISC_NEIGHBOUR_ADVERTISEMENT))
>+ return true;
>+
>+ return false;
>+}
>+
> #endif /* _NET_BONDING_H */
>--
>2.34.1
>
---
-Jay Vosburgh, jv@...sburgh.net
Powered by blists - more mailing lists