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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Tue, 5 Sep 2023 10:32:17 +0000
From: Quan Tian <qtian@...are.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Lars Ekman <uablrek@...il.com>, Quan Tian <qtian@...are.com>
Subject: Re: [PATCH net] net/ipv6: SKB symmetric hash should incorporate
 transport ports

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@...are.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@...il.com>
> > Closes: https://github.com/antrea-io/antrea/issues/5457
> > Signed-off-by: Quan Tian <qtian@...are.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@...gle.com>
> 
> Thanks.

Thanks,
Quan