netdev - Re: [PATCH 1/3] rhashtable: use cmpxchg() in nested_table

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: <87lg1gocj8.fsf@notabene.neil.brown.name>
Date:   Fri, 15 Mar 2019 17:51:55 +1100
From:   NeilBrown <neilb@...e.com>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Thomas Graf <tgraf@...g.ch>, netdev@...r.kernel.org,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] rhashtable: use cmpxchg() in nested_table_alloc()

On Fri, Mar 15 2019, Herbert Xu wrote:

> Hi Neil:
>
> On Thu, Mar 14, 2019 at 04:05:28PM +1100, NeilBrown wrote:
>> nested_table_alloc() relies on the fact that there is
>> at most one spinlock allocated for every slot in the top
>> level nested table, so it is not possible for two threads
>> to try to allocate the same table at the same time.
>> 
>> This assumption is a little fragile (it is not explicit) and is
>> unnecessary.  We can instead protect against
>> a race using cmpxchg() - if it the cmp fails, free the table
>> that was just allocated.
>> 
>> Signed-off-by: NeilBrown <neilb@...e.com>
>> ---
>>  lib/rhashtable.c |    8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> You probably explained it to me before but it's been long enough
> that I no longer remember why we need this change.  So please
> explain in the commit log why this change is needed.  Because
> on the face of it you are adding locking/sychronisation and not
> taking it away.
>

I hoped the patch could be justified on the basis that the current
behaviour is fragile - the dependency that a single spin lock covers a
while slot (and all children) in the top-level nested table is not at
all obvious.

I do have a stronger reason though - I want the replace the spinlocks
with bit-spin-locks.  With those we will only hold a lock for the
particular chain being worked on.  If you need that extra explanation to
justify the patch, I'll hold it over until the other two patches land
and the rest of the bit-spin-lock series is ready.

Thanks,
NeilBrown

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)