netdev - Re: [PATCH net-next 4/4] net: skbuff: fix l4

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 for Android: free password hash cracker in your pocket

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

Date: Mon, 15 May 2023 10:12:10 +0200
From: Antoine Tenart <atenart@...nel.org>
To: Dumitru Ceara <dceara@...hat.com>, Eric Dumazet <edumazet@...gle.com>, Ilya Maximets <i.maximets@....org>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 4/4] net: skbuff: fix l4_hash comment

Quoting Dumitru Ceara (2023-05-11 22:50:32)
> On 5/11/23 19:54, Ilya Maximets wrote:
> >>> Note that skb->hash has never been considered as canonical, for obvious reasons.
> > 
> > I guess, the other point here is that it's not an L4 hash either.
> > 
> > It's a random number.  So, the documentation will still not be
> > correct even after the change proposed in this patch.

The proposed changed is "indicate hash is from layer 4 and provides a
uniform distribution over flows", which does not describe *how* the hash
is computed but *where* it comes from. This matches "random number set
by TCP" and changes in how hashes are computed won't affect the comment,
so we'll not end up in the same situation.

> > One way to not break everything doing that will be to introduce a
> > new flag, e.g. 'rnd_hash' that will be a hash that is "not related
> > to packet fields, but provides a uniform distribution over flows".
> > 
> > skb_get_hash() then may return the current hash if it's any of
> > l4, rnd or sw.  That should preserve the current logic across
> > the kernel code.
> > But having a new flag, we could introduce a new helper, for example
> > skb_get_stable_hash() or skb_get_hash_nonrandom() or something like
> > that, that will be equal to the current version of skb_get_hash(),
> > i.e. not take the random hash into account.
> > 
> > Affected subsystems (OVS, ECMP, SRv6) can be changed to use that
> > new function.  This way these subsystems will get a software hash
> > based on the real packet fields, if it was originally random.
> > This will also preserve ability to use hash provided by the HW,
> > since it is not normally random.

But then the whole point of txrehash would be dismissed, if ECMP and
others stop using the hash provided by TCP. This needs to be a
conditional setting, to make the skb hash to be stable over time only
when needed. That way both scenario are supported.

> What I had in mind is not really a stable hash but a "good enough
> alternative".  It's probably "good enough" (at least for OvS/OVN) if the
> hash used by OvS doesn't change throughout the lifetime of a TCP session.

So what's important is not how the hash is computed but the fact that
it should be stable over time when requested. Isn't exactly what
net.core.txrehash=0 does? If there are some bugs they should be fixed.

On top of this, if OvS needs to additionally provide a canonical
4/5-tuple hash because not only the stability over time is needed but
also the method is important, it needs to compute its own hash. As part
of such potential series ways to cache the result can be explored.
Numbers would help too. (This can be discussed here, that's fine, but I
thought it's important to distinguish the two topics).

Antoine