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

[Quan Tian <qtian@vmware.com>]

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