netfilter: ipset: exclude gc when resize is in progress

Zhengchuan Liang and Eulgyu Kim reported that because resize
does not copy the comment extension into the resized set but
uses it's pointer, ongoing gc can free the extension in the
original set which then results stale pointer in the resized
one. The proposed patch was to recreate the extensions for
every element in the resized set. It is both expensive and
wastes memory, so better exclude gc when resizing in progress
detected: resizing will destroy the original set anyway,
so doing gc on it is unnecessary.

Introduce a new spinlock to exclude parallel gc and resize.
Because we just set and check a bool value, there's no need
for the parameter to be atomic_t and rename it for better
readability.

Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Reported by: Zhengchuan Liang <zcliangcn@gmail.com>
Reported by: Eulgyu Kim <eulgyukim@snu.ac.kr>
Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>

diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index c9a07176624388b3772c07be8ed01dfcb58eea2d..8104dbac02fa65b83bf97371d01237ff1e802afe 100644 (file)
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -75,12 +75,13 @@ struct hbucket {
 struct htable_gc {
        struct delayed_work dwork;
        struct ip_set *set;     /* Set the gc belongs to */
+       spinlock_t lock;        /* Lock to exclude gc and resize */
        u32 region;             /* Last gc run position */
 };
 
 /* The hash table: the table size stored here in order to make resizing easy */
 struct htable {
-       atomic_t ref;           /* References for resizing */
+       bool resizing;          /* Mark ongoing resize */
        atomic_t uref;          /* References for dumping and gc */
        u8 htable_bits;         /* size of hash table == 2^htable_bits */
        u32 maxelem;            /* Maxelem per region */
@@ -582,9 +583,12 @@ mtype_gc(struct work_struct *work)
        if (next_run < HZ/10)
                next_run = HZ/10;
 
-       mtype_gc_do(set, h, t, r);
+       spin_lock_bh(&gc->lock);
+       if (!t->resizing)
+               mtype_gc_do(set, h, t, r);
+       spin_unlock_bh(&gc->lock);
 
-       if (atomic_dec_and_test(&t->uref) && atomic_read(&t->ref)) {
+       if (atomic_dec_and_test(&t->uref) && t->resizing) {
                pr_debug("Table destroy after resize by expire: %p\n", t);
                mtype_ahash_destroy(set, t, false);
        }
@@ -672,11 +676,13 @@ retry:
                spin_lock_init(&t->hregion[i].lock);
 
        /* There can't be another parallel resizing,
-        * but dumping, gc, kernel side add/del are possible
+        * but dumping and kernel side add/del are possible
         */
        orig = ipset_dereference_bh_nfnl(h->table);
-       atomic_set(&orig->ref, 1);
        atomic_inc(&orig->uref);
+       spin_lock_bh(&h->gc.lock);
+       orig->resizing = true;
+       spin_unlock_bh(&h->gc.lock);
        pr_debug("attempt to resize set %s from %u to %u, t %p\n",
                 set->name, orig->htable_bits, htable_bits, orig);
        for (r = 0; r < ahash_numof_locks(orig->htable_bits); r++) {
@@ -792,7 +798,9 @@ out:
 
 cleanup:
        rcu_read_unlock_bh();
-       atomic_set(&orig->ref, 0);
+       spin_lock_bh(&h->gc.lock);
+       orig->resizing = false;
+       spin_unlock_bh(&h->gc.lock);
        atomic_dec(&orig->uref);
        mtype_ahash_destroy(set, t, false);
        if (ret == -EAGAIN)
@@ -1000,7 +1008,7 @@ overwrite_extensions:
        ret = 0;
 resize:
        spin_unlock_bh(&t->hregion[r].lock);
-       if (atomic_read(&t->ref) && ext->target) {
+       if (t->resizing && ext && ext->target) {
                /* Resize is in process and kernel side add, save values */
                struct mtype_resize_ad *x;
 
@@ -1027,7 +1035,7 @@ set_full:
 unlock:
        spin_unlock_bh(&t->hregion[r].lock);
 out:
-       if (atomic_dec_and_test(&t->uref) && atomic_read(&t->ref)) {
+       if (atomic_dec_and_test(&t->uref) && t->resizing) {
                pr_debug("Table destroy after resize by add: %p\n", t);
                mtype_ahash_destroy(set, t, false);
        }
@@ -1090,7 +1098,7 @@ mtype_del(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 #endif
                ip_set_ext_destroy(set, data);
 
-               if (atomic_read(&t->ref) && ext->target) {
+               if (t->resizing && ext && ext->target) {
                        /* Resize is in process and kernel side del,
                         * save values
                         */
@@ -1141,7 +1149,7 @@ out:
                list_add(&x->list, &h->ad);
                spin_unlock_bh(&set->lock);
        }
-       if (atomic_dec_and_test(&t->uref) && atomic_read(&t->ref)) {
+       if (atomic_dec_and_test(&t->uref) && t->resizing) {
                pr_debug("Table destroy after resize by del: %p\n", t);
                mtype_ahash_destroy(set, t, false);
        }
@@ -1350,7 +1358,7 @@ mtype_uref(struct ip_set *set, struct netlink_callback *cb, bool start)
                rcu_read_unlock_bh();
        } else if (cb->args[IPSET_CB_PRIVATE]) {
                t = (struct htable *)cb->args[IPSET_CB_PRIVATE];
-               if (atomic_dec_and_test(&t->uref) && atomic_read(&t->ref)) {
+               if (atomic_dec_and_test(&t->uref) && t->resizing) {
                        pr_debug("Table destroy after resize "
                                 " by dump: %p\n", t);
                        mtype_ahash_destroy(set, t, false);
@@ -1590,6 +1598,7 @@ IPSET_TOKEN(HTYPE, _create)(struct net *net, struct ip_set *set,
                return -ENOMEM;
        }
        h->gc.set = set;
+       spin_lock_init(&h->gc.lock);
        for (i = 0; i < ahash_numof_locks(hbits); i++)
                spin_lock_init(&t->hregion[i].lock);
        h->maxelem = maxelem;