[v1] net/ipv6: SKB symmetric hash should incorporate transport ports

[PATCH net] net/ipv6: SKB symmetric hash should incorporate transport ports

Posted by Quan Tian 2 years, 5 months ago

__skb_get_hash_symmetric() was added to compute a symmetric hash over
the protocol, addresses and transport ports, by commit eb70db875671
("packet: Use symmetric hash for PACKET_FANOUT_HASH."). It uses
flow_keys_dissector_symmetric_keys as the flow_dissector to incorporate
IPv4 addresses, IPv6 addresses and ports. However, it should not specify
the flag as FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL, which stops further
dissection when an IPv6 flow label is encountered, making transport
ports not being incorporated in such case.

As a consequence, the symmetric hash is based on 5-tuple for IPv4 but
3-tuple for IPv6 when flow label is present. It caused a few problems,
e.g. when nft symhash and openvswitch l4_sym rely on the symmetric hash
to perform load balancing as different L4 flows between two given IPv6
addresses would always get the same symmetric hash, leading to uneven
traffic distribution.

Removing the use of FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL makes sure the
symmetric hash is based on 5-tuple for both IPv4 and IPv6 consistently.

Reported-by: Lars Ekman <uablrek@gmail.com>
Closes: https://github.com/antrea-io/antrea/issues/5457
Signed-off-by: Quan Tian <qtian@vmware.com>
---
 net/core/flow_dissector.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 89d15ceaf9af..b3b3af0e7844 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1831,8 +1831,7 @@ u32 __skb_get_hash_symmetric(const struct sk_buff *skb)
 
 	memset(&keys, 0, sizeof(keys));
 	__skb_flow_dissect(NULL, skb, &flow_keys_dissector_symmetric,
-			   &keys, NULL, 0, 0, 0,
-			   FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
+			   &keys, NULL, 0, 0, 0, 0);
 
 	return __flow_hash_from_keys(&keys, &hashrnd);
 }
-- 
2.42.0

Re: [PATCH net] net/ipv6: SKB symmetric hash should incorporate transport ports

Posted by Eric Dumazet 2 years, 5 months ago

On Mon, Sep 4, 2023 at 5:45 PM Quan Tian <qtian@vmware.com> wrote:
>
> __skb_get_hash_symmetric() was added to compute a symmetric hash over
> the protocol, addresses and transport ports, by commit eb70db875671
> ("packet: Use symmetric hash for PACKET_FANOUT_HASH."). It uses
> flow_keys_dissector_symmetric_keys as the flow_dissector to incorporate
> IPv4 addresses, IPv6 addresses and ports. However, it should not specify
> the flag as FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL, which stops further
> dissection when an IPv6 flow label is encountered, making transport
> ports not being incorporated in such case.
>
> As a consequence, the symmetric hash is based on 5-tuple for IPv4 but
> 3-tuple for IPv6 when flow label is present. It caused a few problems,
> e.g. when nft symhash and openvswitch l4_sym rely on the symmetric hash
> to perform load balancing as different L4 flows between two given IPv6
> addresses would always get the same symmetric hash, leading to uneven
> traffic distribution.
>
> Removing the use of FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL makes sure the
> symmetric hash is based on 5-tuple for both IPv4 and IPv6 consistently.
>
> Reported-by: Lars Ekman <uablrek@gmail.com>
> Closes: https://github.com/antrea-io/antrea/issues/5457
> Signed-off-by: Quan Tian <qtian@vmware.com>
> ---
>  net/core/flow_dissector.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 89d15ceaf9af..b3b3af0e7844 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -1831,8 +1831,7 @@ u32 __skb_get_hash_symmetric(const struct sk_buff *skb)
>
>         memset(&keys, 0, sizeof(keys));
>         __skb_flow_dissect(NULL, skb, &flow_keys_dissector_symmetric,
> -                          &keys, NULL, 0, 0, 0,
> -                          FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
> +                          &keys, NULL, 0, 0, 0, 0);
>
>         return __flow_hash_from_keys(&keys, &hashrnd);
>  }
> --
> 2.42.0
>

Networking patches for net branches must include a Fixes: tag, to help
automation.
(providing the sha1 in the changelog is requesting human investigation
for stable teams,
because a sha1 could be mentioned even if it is not the bug origin)

Fixes: eb70db875671 ("packet: Use symmetric hash for PACKET_FANOUT_HASH.")

Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks.

Re: [PATCH net] net/ipv6: SKB symmetric hash should incorporate transport ports

Posted by Quan Tian 2 years, 5 months ago

On Tue, Sep 05, 2023 at 06:28:24AM +0200, Eric Dumazet wrote:
> On Mon, Sep 4, 2023 at 5:45 PM Quan Tian <qtian@vmware.com> wrote:
> >
> > __skb_get_hash_symmetric() was added to compute a symmetric hash over
> > the protocol, addresses and transport ports, by commit eb70db875671
> > ("packet: Use symmetric hash for PACKET_FANOUT_HASH."). It uses
> > flow_keys_dissector_symmetric_keys as the flow_dissector to incorporate
> > IPv4 addresses, IPv6 addresses and ports. However, it should not specify
> > the flag as FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL, which stops further
> > dissection when an IPv6 flow label is encountered, making transport
> > ports not being incorporated in such case.
> >
> > As a consequence, the symmetric hash is based on 5-tuple for IPv4 but
> > 3-tuple for IPv6 when flow label is present. It caused a few problems,
> > e.g. when nft symhash and openvswitch l4_sym rely on the symmetric hash
> > to perform load balancing as different L4 flows between two given IPv6
> > addresses would always get the same symmetric hash, leading to uneven
> > traffic distribution.
> >
> > Removing the use of FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL makes sure the
> > symmetric hash is based on 5-tuple for both IPv4 and IPv6 consistently.
> >
> > Reported-by: Lars Ekman <uablrek@gmail.com>
> > Closes: https://github.com/antrea-io/antrea/issues/5457
> > Signed-off-by: Quan Tian <qtian@vmware.com>
> > ---
> >  net/core/flow_dissector.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 89d15ceaf9af..b3b3af0e7844 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -1831,8 +1831,7 @@ u32 __skb_get_hash_symmetric(const struct sk_buff *skb)
> >
> >         memset(&keys, 0, sizeof(keys));
> >         __skb_flow_dissect(NULL, skb, &flow_keys_dissector_symmetric,
> > -                          &keys, NULL, 0, 0, 0,
> > -                          FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL);
> > +                          &keys, NULL, 0, 0, 0, 0);
> >
> >         return __flow_hash_from_keys(&keys, &hashrnd);
> >  }
> > --
> > 2.42.0
> >
> 
> Networking patches for net branches must include a Fixes: tag, to help
> automation.
> (providing the sha1 in the changelog is requesting human investigation
> for stable teams,
> because a sha1 could be mentioned even if it is not the bug origin)

Thank you! Will add the tag in the next version.

> 
> Fixes: eb70db875671 ("packet: Use symmetric hash for PACKET_FANOUT_HASH.")
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> 
> Thanks.

Thanks,
Quan