Date: Tue, 16 May 2023 23:25:19 +0200
From: Ilya Maximets <i.maximets@....org>
To: Antoine Tenart <atenart@...nel.org>, Dumitru Ceara <dceara@...hat.com>,
 Eric Dumazet <edumazet@...gle.com>
Cc: i.maximets@....org, 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

On 5/16/23 09:36, Antoine Tenart wrote:
> Quoting Ilya Maximets (2023-05-15 20:23:28)
>> On 5/15/23 10:12, Antoine Tenart wrote:
>>> 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.
>>
>> I respectfully disagree,  "is from layer 4" and "random number" do not
>> match for me.  So, "where it comes from" argument is not applicable.
>> Random numbers come from random number generator, and not "from layer 4".
>>
>> Unless by "from layer 4" you mean "from the code that handles layer 4
>> packet processing".  But that seems very confusing to me.  And it is
>> definitely not the first thing that comes to mind while reading the
>> documentation.
> 
> Yes that is what I meant, but if that is still confusing then this is
> not improving things so let's try something better. I intentionally did
> not mention how the hash is computed because it's easy to forget to
> update the documentation when the exact logic is changed. What's
> important here IMHO is to mention what the hash provides.
> 
> What about "indicates hash was set by layer 4 stack and provides a
> uniform distribution over flows"? Or/and we should we also add a
> disclaimer like "no guarantee on how the hash was computed"?

I'm still not sure this is correct.  Is a NIC driver part of layer 4 stack?
Also it still doesn't make a lot of sense to change to comment without
changing the code.

And there are lots of other inconsistencies around skb hash.  The following
is probably the most colorful that I found:

TCP code in tcp_make_synack() calls:

   skb_set_hash(skb, tcp_rsk(req)->txhash, PKT_HASH_TYPE_L4);

Where 'txhash' is a random number.  But it calls it PKT_HASH_TYPE_L4.

Going to the definition we can find this [1]:

/*
 * Packet hash types specify the type of hash in skb_set_hash.
 *
 * Hash types refer to the protocol layer addresses which are used to
 * construct a packet's hash. The hashes are used to differentiate or identify
 * flows of the protocol layer for the hash type. Hash types are either
 * layer-2 (L2), layer-3 (L3), or layer-4 (L4).
 *
 * Properties of hashes:
 *
 * 1) Two packets in different flows have different hash values
 * 2) Two packets in the same flow should have the same hash value

Now this directly contradicts to the hash being not stable and not being
computed form actual packet fields.

Later in the same file:

enum pkt_hash_types {
	PKT_HASH_TYPE_NONE,	/* Undefined type */
	PKT_HASH_TYPE_L2,	/* Input: src_MAC, dest_MAC */
	PKT_HASH_TYPE_L3,	/* Input: src_IP, dst_IP */
	PKT_HASH_TYPE_L4,	/* Input: src_IP, dst_IP, src_port, dst_port */
};

Here we see that PKT_HASH_TYPE_L4 supposed to use particular fields
as an input.

It's kind of pointless having all that documented if the l4_hash flag
is a random number and none of the kernel subsystems are able to use
it in a way it is documented.

[1] https://elixir.bootlin.com/linux/v6.4-rc1/source/include/linux/skbuff.h#L1419

Best regards, Ilya Maximets.

Powered by blists - more mailing lists