netdev - Re: [PATCH net-next 0/4] Allow configuration of multipath hash seed

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]

Date: Thu, 30 May 2024 21:07:43 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Petr Machata <petrm@...dia.com>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 Ido Schimmel <idosch@...dia.com>, David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH net-next 0/4] Allow configuration of multipath hash seed

On 5/30/24 20:27, Nikolay Aleksandrov wrote:
> On 5/30/24 18:25, Petr Machata wrote:
>>
>> Nikolay Aleksandrov <razor@...ckwall.org> writes:
>>
>>> I think that using memory management for such simple task is an
>>> overkill. Would it be simpler to define 2 x 4 byte seed variables
>>> in netns_ipv4 (e.g. user_seed, mp_seed). One is set only by the
>>> user through the sysctl, which would also set mp_seed. Then you
>>> can use mp_seed in the fast-path to construct that siphash key.
>>> If the user_seed is set to 0 then you reset to some static init
>>> hash value that is generated on init_net's creation. The idea
>>
>> Currently the flow dissector siphash key is initialized lazily so that
>> the pool of random bytes is full when the initialization is done:
>>
>>     https://lore.kernel.org/all/20131023180527.GC2403@order.stressinduktion.org
>>     https://lore.kernel.org/all/20131023111219.GA31531@order.stressinduktion.org
>>
>> I'm not sure how important that is -- the mailing does not really
>> discuss much in the way of rationale, and admits it's not critical. But
>> initializing the seed during net init would undo that. At the same time,
>> initializing it lazily would be a bit of a mess, as we would have to
>> possibly retroactively update mp_hash as well, which would be racy vs.
>> user_hash update unless locked. So dunno.
> 
> If you want to keep the late init, untested:
> init_mp_seed_once() -> DO_ONCE(func (net_get_random_once(&init_mp_seed),
> memcpy(&init_net.mp_seed, &init_mp_seed))
> 

Blah, just get_random_bytes() instead of net_get*. :)
It'll be executed once anyway. But again I think we can do without all
of this.

>>
>> If you are OK with giving up on the siphash key "quality", I'm fine with
>> this.
>>
> 
> IMO that's fine, early init of the seed wouldn't be a problem. net's
> hash_mix is already initialized early.
> 
[snip]
>>
>> I kept the RCU stuff in because it makes it easy to precompute the
>> siphash key while allowing readers to access it lock-free. I could
>> inline it and guard with a seqlock instead, but that's a bit messier
>> code-wise. Or indeed construct in-situ, it's an atomic access plus like
>> four instructions or something like that.
> 
> You can READ/WRITE_ONCE() the full 8 bytes every time so it's lock-free
> and consistent view of both values for observers. For fast-path it'll
> only be accessing one of the two values, so it's fine either way. You
> can use barriers to ensure latest value is seen by interested readers,
> but for most eventual consistency would be enough.
> 

Actually aren't we interested only in user_seed in the external reader
case? We don't care what's in mp_seed, so this is much simpler.