netfilter: xt_hashlimit: unregister proc file before releasing mutex

Before releasing the global mutex, we only unlink the hashtable
from the hash list, its proc file is still not unregistered at
this point. So syzbot could trigger a race condition where a
parallel htable_create() could register the same file immediately
after the mutex is released.

Move htable_remove_proc_entry() back to mutex protection to
fix this. And, fold htable_destroy() into htable_put() to make
the code slightly easier to understand.

Reported-and-tested-by: syzbot+d195fd3b9a364ddd6731@syzkaller.appspotmail.com
Fixes: c4a3922d2d20 ("netfilter: xt_hashlimit: reduce hashlimit_mutex scope for htable_put()")
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 7a2c4b8..8c835ad 100644 (file)
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -402,15 +402,6 @@ static void htable_remove_proc_entry(struct xt_hashlimit_htable *hinfo)
                remove_proc_entry(hinfo->name, parent);
 }
 
-static void htable_destroy(struct xt_hashlimit_htable *hinfo)
-{
-       cancel_delayed_work_sync(&hinfo->gc_work);
-       htable_remove_proc_entry(hinfo);
-       htable_selective_cleanup(hinfo, true);
-       kfree(hinfo->name);
-       vfree(hinfo);
-}
-
 static struct xt_hashlimit_htable *htable_find_get(struct net *net,
                                                   const char *name,
                                                   u_int8_t family)
@@ -432,8 +423,13 @@ static void htable_put(struct xt_hashlimit_htable *hinfo)
 {
        if (refcount_dec_and_mutex_lock(&hinfo->use, &hashlimit_mutex)) {
                hlist_del(&hinfo->node);
+               htable_remove_proc_entry(hinfo);
                mutex_unlock(&hashlimit_mutex);
-               htable_destroy(hinfo);
+
+               cancel_delayed_work_sync(&hinfo->gc_work);
+               htable_selective_cleanup(hinfo, true);
+               kfree(hinfo->name);
+               vfree(hinfo);
        }
 }