netdev - Re: rhashtable issue

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: <yaxjp5k4o37vh2bl2ecuj3qoyz6x3lwau2kf7zevq5v3krcmtu@idoh3wd4zyqu>
Date: Mon, 25 Nov 2024 19:38:57 -0500
From: Kent Overstreet <kent.overstreet@...ux.dev>
To: NeilBrown <neilb@...e.de>
Cc: Herbert Xu <herbert@...dor.apana.org.au>, Thomas Graf <tgraf@...g.ch>, 
	netdev@...r.kernel.org
Subject: Re: rhashtable issue - -EBUSY

On Tue, Nov 26, 2024 at 10:38:10AM +1100, NeilBrown wrote:
> On Mon, 25 Nov 2024, Herbert Xu wrote:
> > On Mon, Nov 25, 2024 at 10:43:16AM +1100, NeilBrown wrote:
> > >
> > > So please turn it into a WARN_ON_ONCE and don't allow the code to return
> > > it.
> > 
> > You still have to handle ENOMEM, right?
> 
> I'd rather not.  Why is ENOMEM exposed to the caller?
> The rehash thread should *always* allocated the *whole* table before
> adding it to the chain and starting to rehash into it.  Until that
> allocation succeeds, just keeping inserting in the existing table.
> 
> rhashtables seems to have been written with an understanding that long
> hash chains are completely unacceptable and failure to insert is
> preferred.  I accept that in some circumstances that might be true, but
> in other circumstances failure to insert (except for -EEXIST) is
> anathema and long hash chains are simply unfortunate.
> 
> Now I could write an alternate resizable hashtable implementation which
> values predictable behaviour over short chains, but it would only be a
> tiny bit different from rhashtables so it seems to make more sense to
> add that option into rhashtables (which already has several options for
> different use cases).
> 
> So I don't ever want -E2BIG either.  Either -EEXIST or success should be
> the only possible results of "insert".

Networking and storage people seem to have quite different perspectives
on these issues; in filesystem and storage land in general, randomly
failing is flatly unacceptable.

We do need these issues fixed.

I don't think the -EBUSY is being taken seriously enough either,
especially given that the default hash is jhash. I've seen jhash produce
bad statistical behaviour before, and while rehashing with a different seen
may make jhash acceptable, I'm not an all confident that we won't ever
see -EBUSY show up in production.

That's the sort of issue that won't ever show up in testing, but it will
show up in production as users start coming up with all sorts of weird
workloads - and good luck debugging those...