netdev - Re: [PATCH net-next 1/5] rhashtable: Don't reset walker table in 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>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 1 Dec 2017 17:07:43 -0800
From:   Tom Herbert <tom@...ntonium.net>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Rohit LastName <rohit@...ntonium.net>
Subject: Re: [PATCH net-next 1/5] rhashtable: Don't reset walker table in rhashtable_walk_start

On Fri, Dec 1, 2017 at 3:29 PM, Tom Herbert <tom@...ntonium.net> wrote:
> On Fri, Dec 1, 2017 at 2:18 PM, Herbert Xu <herbert@...dor.apana.org.au> wrote:
>> On Thu, Nov 30, 2017 at 04:03:01PM -0800, Tom Herbert wrote:
>>> Remove the code that resets the walker table. The walker table should
>>> only be initialized in the walk init function or when a future table is
>>> encountered. If the walker table is NULL this is the indication that
>>> the walk has completed and this information can be used to break a
>>> multi-call walk in the table (e.g. successive calls to nelink_dump
>>> that are dumping elements of an rhashtable).
>>>
>>> This also allows us to change rhashtable_walk_start to return void
>>> since the only error it was returning was -EAGAIN for a table change.
>>> This patch changes all the callers of rhashtable_walk_start to expect
>>> void which eliminates logic needed to check the return value for a
>>> rare condition. Note that -EAGAIN will be returned in a call
>>> to rhashtable_walk_next which seems to always follow the start
>>> of the walk so there should be no behavioral change in doing this.
>>>
>>> Signed-off-by: Tom Herbert <tom@...ntonium.net>
>>
>> Doesn't this mean that if a walk encounters a rehash you may end up
>> missing half or more of the hash table?
>>
> Because of tbl->rehash < tbl->size conditions in walk stop? How about
> we add a flag to iter that indicates table needs a reset and set it
> along with setting walker.tbl to NULL? On the next walk start do the
> reload when walker.tbl is NULL and flag is set. In this case walk
> start would automatically set walker.tbl which is already done by
> nearly all callers already in that they ignore -EAGAIN returned from
> start walk.
>
Herbert,

Looking at this some more, I am wondering if the walkers list is
necessary. When a rehash table is done, the new table is assigned to
ht->tbl and walker->tbl is cleared for all walkers. In walk start the
walker tbl is checked and if it's NULL ht->tbl is loaded. Assuming
that -EAGAIN isn't interesting to callers here, it seems like we could
just get iter->walker.tbl in each call to walk start and not need to
maintain the walkers list at all. Am I missing something?

Tom

> Thanks,
> Tom
>
>> Cheers,
>> --
>> Email: Herbert Xu <herbert@...dor.apana.org.au>
>> Home Page: http://gondor.apana.org.au/~herbert/
>> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt