netdev - Re: [PATCH net] bonding: discard lowest hash bit for 802.3ad layer3+4

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89iKWvhALs8sChpHWirtzi2v8YcoczW6MqrvokZxp9a00jA@mail.gmail.com>
Date:   Sun, 5 Nov 2017 13:38:47 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Hangbin Liu <liuhangbin@...il.com>
Cc:     netdev <netdev@...r.kernel.org>, Paolo Abeni <pabeni@...hat.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] bonding: discard lowest hash bit for 802.3ad layer3+4

On Sun, Nov 5, 2017 at 6:56 AM, Hangbin Liu <liuhangbin@...il.com> wrote:
> After commit 07f4c90062f8 ("tcp/dccp: try to not exhaust ip_local_port_range
> in connect()"), we will try to use even ports for connect(). Then If an
> application (seen clearly with iperf) opens multiple streams to the same
> destination IP and port, each stream will be given an even source port.
>
> So the bonding driver's simple xmit_hash_policy based on layer3+4 addressing
> will always hash all these streams to the same interface. And the total
> throughput will limited to a single slave.
>
> Change the tcp code will impact the whole tcp behavior, only for bonding
> usage. Paolo Abeni suggested fix this by changing the bonding code only,
> which should be more reasonable, and less impact.
>
> Fix this by discarding the lowest hash bit because it contains little entropy.
> After the fix we can re-balance between slaves.
>
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
>  drivers/net/bonding/bond_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index c99dc59..728fa08 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3237,7 +3237,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>
>         if (bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP34 &&
>             skb->l4_hash)
> -               return skb->hash;
> +               return skb->hash >> 1;

Why are you changing this part ?

The l4 hash provided by local TCP stack does not use a pathological
XOR based on ports/addresses,
but a random value with pretty good entropy.

No need to try do 'enhance' it by actually being slightly worse.


>
>         if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER2 ||
>             !bond_flow_dissect(bond, skb, &flow))
> @@ -3253,7 +3253,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>         hash ^= (hash >> 16);
>         hash ^= (hash >> 8);
>
> -       return hash;
> +       return hash >> 1;
>  }
>
>  /*-------------------------- Device entry points ----------------------------*/
> --
> 2.5.5
>