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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <fe2f6594-b330-bc5b-55a5-8e1686a2eac1@redhat.com>
Date: Thu, 11 May 2023 14:10:30 +0200
From: Dumitru Ceara <dceara@...hat.com>
To: Antoine Tenart <atenart@...nel.org>, davem@...emloft.net,
 kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com
Cc: netdev@...r.kernel.org, Ilya Maximets <i.maximets@....org>
Subject: Re: [PATCH net-next 4/4] net: skbuff: fix l4_hash comment

Hi Antoine,

On 5/11/23 11:34, Antoine Tenart wrote:
> Since commit 877d1f6291f8 ("net: Set sk_txhash from a random number")
> sk->sk_txhash is not a canonical 4-tuple hash. sk->sk_txhash is
> used in the TCP Tx path to populate skb->hash, with skb->l4_hash=1.
> With this, skb->l4_hash does not always indicate the hash is a
> "canonical 4-tuple hash over transport ports" but rather a hash from L4
> layer to provide a uniform distribution over flows. Reword the comment
> accordingly, to avoid misunderstandings.

But AFAIU the hash used to be a canonical 4-tuple hash and was used as
such by other components, e.g., OvS:

https://elixir.bootlin.com/linux/latest/source/net/openvswitch/actions.c#L1069

It seems to me at least unfortunate that semantics change without
considering other users.  The fact that we now fix the documentation
makes it seem like OvS was wrong to use the skb hash.  However, before
877d1f6291f8 ("net: Set sk_txhash from a random number") it was OK for
OvS to use the skb hash as a canonical 4-tuple hash.

Best regards,
Dumitru

> 
> Signed-off-by: Antoine Tenart <atenart@...nel.org>
> ---
>  include/linux/skbuff.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 738776ab8838..f54c84193b23 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -791,8 +791,8 @@ typedef unsigned char *sk_buff_data_t;
>   *	@active_extensions: active extensions (skb_ext_id types)
>   *	@ndisc_nodetype: router type (from link layer)
>   *	@ooo_okay: allow the mapping of a socket to a queue to be changed
> - *	@l4_hash: indicate hash is a canonical 4-tuple hash over transport
> - *		ports.
> + *	@l4_hash: indicate hash is from layer 4 and provides a uniform
> + *		distribution over flows.
>   *	@sw_hash: indicates hash was computed in software stack
>   *	@wifi_acked_valid: wifi_acked was set
>   *	@wifi_acked: whether frame was acked on wifi or not