linux-kernel - Re: [Cluster-devel] [PATCH v2 0/2] gfs2: Stop using rhashtable_walk

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>] [day] [month] [year] [list]

Message-ID: <871sftxgmx.fsf@notabene.neil.brown.name>
Date:   Fri, 06 Apr 2018 11:33:58 +1000
From:   NeilBrown <neilb@...e.com>
To:     Andreas Grünbacher 
        <andreas.gruenbacher@...il.com>,
        Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Andreas Gruenbacher <agruenba@...hat.com>,
        Bob Peterson <rpeterso@...hat.com>,
        Thomas Graf <tgraf@...g.ch>, Tom Herbert <tom@...ntonium.net>,
        cluster-devel@...hat.com, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [Cluster-devel] [PATCH v2 0/2] gfs2: Stop using rhashtable_walk_peek

On Wed, Apr 04 2018, Andreas Grünbacher wrote:

> Herbert Xu <herbert@...dor.apana.org.au> schrieb am Mi. 4. Apr. 2018 um
> 17:51:
>
>> On Wed, Apr 04, 2018 at 11:46:28AM -0400, Bob Peterson wrote:
>> >
>> > The patches look good. The big question is whether to add them to this
>> > merge window while it's still open. Opinions?
>>
>> We're still hashing out the rhashtable interface so I don't think now is
>> the time to rush things.
>
>
> Fair enough. No matter how rhashtable_walk_peek changes, we‘ll still need
> these two patches to fix the glock dump though.

Those two patches look fine to me and don't depend on changes to
rhashtable, so it is up to you when they go upstream.

However, I think the code can be substantially simplified, particularly
once we make rhashtable a little cleverer.
So this is what I'll probably be doing for a similar situation in
lustre....

Having examined seqfile closely, it is apparent that if ->start never
changes *ppos, and if ->next always increments it (except maybe on error)
then

1/ ->next is only ever given a 'pos' that was returned by the previous
   call to ->start or ->next.  So it should *always* return the next
   object, after the one previously returned by ->start or ->next.  It
   never needs to 'seek'. The 'traverse()' function in seq_file.c does
   any seeking needed.  ->next needs to increase 'pos' and when walking
   a dynamic list, it is easiest if it just increments it.

2/ ->start is only called with a pos of:
    0 - in this case it should rewind to the start
    the last pos passed to ->start of ->next
         In this case it should return the same thing that was
         returned last time.  If it no longer exists, then
         the following one should be returned.
    one more than the last pos passed to ->start or ->next
         In this case it should return the object after the
         last one returned.

The proposed enhancement to rhashtable_walk* is to add a
rhashtable_walk_prev() which returns the previously returned object,
if it is still in the table, or NULL. It also enhances
rhashtable_walk_start() so that if the previously returned object is
still in the table, it is preserved as the current cursor.
This means that if you take some action to ensure that the
previously returned object remains in the table until the next ->start,
then you can reliably walk the table with no duplicates or omissions
(unless a concurrent rehash causes duplicates)
If you don't keep the object in the table and it gets removed, then
the 'skip' counter is used to find your place, and you might get
duplicates or omissions.

NeilBrown

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