netdev - Re: nft_hash rhashtable question

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: <20150112224249.GA11857@casper.infradead.org>
Date:	Mon, 12 Jan 2015 22:42:49 +0000
From:	tgraf <tgraf@...g.ch>
To:	John Fastabend <john.fastabend@...il.com>
Cc:	netdev <netdev@...r.kernel.org>
Subject: Re: nft_hash rhashtable question

On 01/12/15 at 02:30pm, John Fastabend wrote:
> >        /* Stop an eventual async resizing */
> >        priv->being_destroyed = true;

This aborts and eventual resize in the background.

> >        mutex_lock(&priv->mutex); <-- get the lock so we have single updater

this ensures that the resize finished.

> >        tbl = rht_dereference(priv->tbl, priv);
> >        for (i = 0; i < tbl->size; i++) {
> >                rht_for_each_entry_safe(he, pos, next, tbl, i, node)
> >                        nft_hash_elem_destroy(set, he);              <-- does a kfree on he?
> >        }
> >        mutex_unlock(&priv->mutex); <-- release the lock
> >
> >        rhashtable_destroy(priv);

Since no insert or removal can occur we can be assured that no new
entry was added in the meantime so we can destroy the rhashtable
without any further protection.

> >}
> 
> 
> Is it really safe to kfree 'he' without waiting a grace
> period for any rcu readers to drop the reference?
> 
> I'm considering what happens if nft_hash_destroy runs in
> parallel with nft_hash_lookup?

The nft_set API ensures that a destroy can't occur in parallel to
an insertion or removal. All we have to ensure is that any resizing
in the background is aborted and completed.

If you look at the code before the rhashtable there was no locking
at all:

static void nft_hash_destroy(const struct nft_set *set)
{
        const struct nft_hash *priv = nft_set_priv(set);
        const struct nft_hash_table *tbl = nft_dereference(priv->tbl);
        struct nft_hash_elem *he, *next;
        unsigned int i;

        for (i = 0; i < tbl->size; i++) {
                for (he = nft_dereference(tbl->buckets[i]); he != NULL;
                     he = next) {
                        next = nft_dereference(he->next);
                        nft_hash_elem_destroy(set, he);
                }
        }
        kfree(tbl);
}
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html