netfilter: ipset: fix race condition between swap/destroy and kernel side add/del...

The patch "netfilter: ipset: fix race condition between swap/destroy
and kernel side add/del/test", commit 28628fa9 fixes a race condition.
But the synchronize_rcu() added to the swap function unnecessarily slows
it down: it can safely be moved to destroy and use call_rcu() instead.

Eric Dumazet pointed out that simply calling the destroy functions as
rcu callback does not work: sets with timeout use garbage collectors
which need cancelling at destroy which can wait. Therefore the destroy
functions are split into two: cancelling garbage collectors safely at
executing the command received by netlink and moving the remaining
part only into the rcu callback.

Link: https://lore.kernel.org/lkml/C0829B10-EAA6-4809-874E-E1E9C05A8D84@automattic.com/
Fixes: 28628fa952fe ("netfilter: ipset: fix race condition between swap/destroy and kernel side add/del/test")
Reported-by: Ale Crismani <ale.crismani@automattic.com>
Reported-by: David Wang <00107082@163.com>
Tested-by: David Wang <00107082@163.com>
Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>

diff --git a/kernel/include/linux/netfilter/ipset/ip_set.h b/kernel/include/linux/netfilter/ipset/ip_set.h
index 0c6eee2528e5a2efcdfd188097d34ed74c23301b..7691b7a2bb7a2c13c80981eae299a8b517950f55 100644 (file)
--- a/kernel/include/linux/netfilter/ipset/ip_set.h
+++ b/kernel/include/linux/netfilter/ipset/ip_set.h
@@ -189,6 +189,8 @@ struct ip_set_type_variant {
        /* Return true if "b" set is the same as "a"
         * according to the create set parameters */
        bool (*same_set)(const struct ip_set *a, const struct ip_set *b);
+       /* Cancel ongoing garbage collectors before destroying the set*/
+       void (*cancel_gc)(struct ip_set *set);
        /* Region-locking is used */
        bool region_lock;
 };
@@ -245,6 +247,8 @@ extern void ip_set_type_unregister(struct ip_set_type *set_type);
 
 /* A generic IP set */
 struct ip_set {
+       /* For call_cru in destroy */
+       struct rcu_head rcu;
        /* The name of the set */
        char name[IPSET_MAXNAMELEN];
        /* Lock protecting the set data */

diff --git a/kernel/net/netfilter/ipset/ip_set_bitmap_gen.h b/kernel/net/netfilter/ipset/ip_set_bitmap_gen.h
index 0479750e2a5e1dabde3da79b5932584fc0d215e9..3245b6b22d1d751460d6860450861301f0c0ede3 100644 (file)
--- a/kernel/net/netfilter/ipset/ip_set_bitmap_gen.h
+++ b/kernel/net/netfilter/ipset/ip_set_bitmap_gen.h
@@ -29,6 +29,7 @@
 #define mtype_del              IPSET_TOKEN(MTYPE, _del)
 #define mtype_list             IPSET_TOKEN(MTYPE, _list)
 #define mtype_gc               IPSET_TOKEN(MTYPE, _gc)
+#define mtype_cancel_gc                IPSET_TOKEN(MTYPE, _cancel_gc)
 #define mtype                  MTYPE
 
 #define get_ext(set, map, id)  ((map)->extensions + ((set)->dsize * (id)))
@@ -58,9 +59,6 @@ mtype_destroy(struct ip_set *set)
 {
        struct mtype *map = set->data;
 
-       if (SET_WITH_TIMEOUT(set))
-               del_timer_sync(&map->gc);
-
        if (set->dsize && set->extensions & IPSET_EXT_DESTROY)
                mtype_ext_cleanup(set);
        ip_set_free(map->members);
@@ -290,6 +288,15 @@ mtype_gc(GC_ARG)
        add_timer(&map->gc);
 }
 
+static void
+mtype_cancel_gc(struct ip_set *set)
+{
+       struct mtype *map = set->data;
+
+       if (SET_WITH_TIMEOUT(set))
+               del_timer_sync(&map->gc);
+}
+
 static const struct ip_set_type_variant mtype = {
        .kadt   = mtype_kadt,
        .uadt   = mtype_uadt,
@@ -303,6 +310,7 @@ static const struct ip_set_type_variant mtype = {
        .head   = mtype_head,
        .list   = mtype_list,
        .same_set = mtype_same_set,
+       .cancel_gc = mtype_cancel_gc,
 };
 
 #endif /* __IP_SET_BITMAP_IP_GEN_H */

diff --git a/kernel/net/netfilter/ipset/ip_set_core.c b/kernel/net/netfilter/ipset/ip_set_core.c
index 3a93fd5297c470c7ec78cdae5be78759cab2a723..126312d749c3ad9a6789c122070092c97adfcb8c 100644 (file)
--- a/kernel/net/netfilter/ipset/ip_set_core.c
+++ b/kernel/net/netfilter/ipset/ip_set_core.c
@@ -1192,6 +1192,14 @@ ip_set_destroy_set(struct ip_set *set)
        kfree(set);
 }
 
+static void
+ip_set_destroy_set_rcu(struct rcu_head *head)
+{
+       struct ip_set *set = container_of(head, struct ip_set, rcu);
+
+       ip_set_destroy_set(set);
+}
+
 static int
 IPSET_CBFN(ip_set_destroy, struct net *net, struct sock *ctnl,
           struct sk_buff *skb, const struct nlmsghdr *nlh,
@@ -1207,9 +1215,6 @@ IPSET_CBFN(ip_set_destroy, struct net *net, struct sock *ctnl,
        if (unlikely(protocol_min_failed(attr)))
                return -IPSET_ERR_PROTOCOL;
 
-       /* Must wait for flush to be really finished in list:set */
-       rcu_barrier();
-
        /* Commands are serialized and references are
         * protected by the ip_set_ref_lock.
         * External systems (i.e. xt_set) must call
@@ -1220,8 +1225,10 @@ IPSET_CBFN(ip_set_destroy, struct net *net, struct sock *ctnl,
         * counter, so if it's already zero, we can proceed
         * without holding the lock.
         */
-       read_lock_bh(&ip_set_ref_lock);
        if (!attr[IPSET_ATTR_SETNAME]) {
+               /* Must wait for flush to be really finished in list:set */
+               rcu_barrier();
+               read_lock_bh(&ip_set_ref_lock);
                for (i = 0; i < inst->ip_set_max; i++) {
                        s = ip_set(inst, i);
                        if (s && (s->ref || s->ref_netlink)) {
@@ -1235,6 +1242,8 @@ IPSET_CBFN(ip_set_destroy, struct net *net, struct sock *ctnl,
                        s = ip_set(inst, i);
                        if (s) {
                                ip_set(inst, i) = NULL;
+                               /* Must cancel garbage collectors */
+                               s->variant->cancel_gc(s);
                                ip_set_destroy_set(s);
                        }
                }
@@ -1242,6 +1251,9 @@ IPSET_CBFN(ip_set_destroy, struct net *net, struct sock *ctnl,
                inst->is_destroyed = false;
        } else {
                u32 flags = flag_exist(INFO_NLH(info, nlh));
+               u16 features = 0;
+
+               read_lock_bh(&ip_set_ref_lock);
                s = find_set_and_id(inst, nla_data(attr[IPSET_ATTR_SETNAME]),
                                    &i);
                if (!s) {
@@ -1252,10 +1264,16 @@ IPSET_CBFN(ip_set_destroy, struct net *net, struct sock *ctnl,
                        ret = -IPSET_ERR_BUSY;
                        goto out;
                }
+               features = s->type->features;
                ip_set(inst, i) = NULL;
                read_unlock_bh(&ip_set_ref_lock);
-
-               ip_set_destroy_set(s);
+               if (features & IPSET_TYPE_NAME) {
+                       /* Must wait for flush to be really finished  */
+                       rcu_barrier();
+               }
+               /* Must cancel garbage collectors */
+               s->variant->cancel_gc(s);
+               call_rcu(&s->rcu, ip_set_destroy_set_rcu);
        }
        return 0;
 out:
@@ -1420,9 +1438,6 @@ IPSET_CBFN(ip_set_swap, struct net *net, struct sock *ctnl,
        ip_set(inst, to_id) = from;
        write_unlock_bh(&ip_set_ref_lock);
 
-       /* Make sure all readers of the old set pointers are completed. */
-       synchronize_rcu();
-
        return 0;
 }
 
@@ -2540,8 +2555,11 @@ ip_set_fini(void)
 {
        nf_unregister_sockopt(&so_set);
        nfnetlink_subsys_unregister(&ip_set_netlink_subsys);
-
        UNREGISTER_PERNET_SUBSYS(&ip_set_net_ops);
+
+       /* Wait for call_rcu() in destroy */
+       rcu_barrier();
+
        pr_debug("these are the famous last words\n");
 }
 

diff --git a/kernel/net/netfilter/ipset/ip_set_hash_gen.h b/kernel/net/netfilter/ipset/ip_set_hash_gen.h
index af3899152af6dbaa23c31c2e3570b36e8b6d7209..a88acf405bc968c8ed3a9873ccc1835a8d8f6467 100644 (file)
--- a/kernel/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/kernel/net/netfilter/ipset/ip_set_hash_gen.h
@@ -222,6 +222,7 @@ static const union nf_inet_addr zeromask = {};
 #undef mtype_gc_do
 #undef mtype_gc
 #undef mtype_gc_init
+#undef mtype_cancel_gc
 #undef mtype_variant
 #undef mtype_data_match
 
@@ -266,6 +267,7 @@ static const union nf_inet_addr zeromask = {};
 #define mtype_gc_do            IPSET_TOKEN(MTYPE, _gc_do)
 #define mtype_gc               IPSET_TOKEN(MTYPE, _gc)
 #define mtype_gc_init          IPSET_TOKEN(MTYPE, _gc_init)
+#define mtype_cancel_gc                IPSET_TOKEN(MTYPE, _cancel_gc)
 #define mtype_variant          IPSET_TOKEN(MTYPE, _variant)
 #define mtype_data_match       IPSET_TOKEN(MTYPE, _data_match)
 
@@ -450,9 +452,6 @@ mtype_destroy(struct ip_set *set)
        struct htype *h = set->data;
        struct list_head *l, *lt;
 
-       if (SET_WITH_TIMEOUT(set))
-               cancel_delayed_work_sync(&h->gc.dwork);
-
        mtype_ahash_destroy(set, ipset_dereference_nfnl(h->table), true);
        list_for_each_safe(l, lt, &h->ad) {
                list_del(l);
@@ -598,6 +597,15 @@ mtype_gc_init(struct htable_gc *gc)
        queue_delayed_work(system_power_efficient_wq, &gc->dwork, HZ);
 }
 
+static void
+mtype_cancel_gc(struct ip_set *set)
+{
+       struct htype *h = set->data;
+
+       if (SET_WITH_TIMEOUT(set))
+               cancel_delayed_work_sync(&h->gc.dwork);
+}
+
 static int
 mtype_add(struct ip_set *set, void *value, const struct ip_set_ext *ext,
          struct ip_set_ext *mext, u32 flags);
@@ -1441,6 +1449,7 @@ static const struct ip_set_type_variant mtype_variant = {
        .uref   = mtype_uref,
        .resize = mtype_resize,
        .same_set = mtype_same_set,
+       .cancel_gc = mtype_cancel_gc,
        .region_lock = true,
 };
 

diff --git a/kernel/net/netfilter/ipset/ip_set_list_set.c b/kernel/net/netfilter/ipset/ip_set_list_set.c
index 8c7fef8e6858bdbba388b0ce9284b54392cc2350..d7d487c97d2eb5df8e4c5a2dabd349cc8fdc4277 100644 (file)
--- a/kernel/net/netfilter/ipset/ip_set_list_set.c
+++ b/kernel/net/netfilter/ipset/ip_set_list_set.c
@@ -429,9 +429,6 @@ list_set_destroy(struct ip_set *set)
        struct list_set *map = set->data;
        struct set_elem *e, *n;
 
-       if (SET_WITH_TIMEOUT(set))
-               del_timer_sync(&map->gc);
-
        list_for_each_entry_safe(e, n, &map->members, list) {
                list_del(&e->list);
                ip_set_put_byindex(map->net, e->id);
@@ -548,6 +545,15 @@ list_set_same_set(const struct ip_set *a, const struct ip_set *b)
               a->extensions == b->extensions;
 }
 
+static void
+list_set_cancel_gc(struct ip_set *set)
+{
+       struct list_set *map = set->data;
+
+       if (SET_WITH_TIMEOUT(set))
+               del_timer_sync(&map->gc);
+}
+
 static const struct ip_set_type_variant set_variant = {
        .kadt   = list_set_kadt,
        .uadt   = list_set_uadt,
@@ -561,6 +567,7 @@ static const struct ip_set_type_variant set_variant = {
        .head   = list_set_head,
        .list   = list_set_list,
        .same_set = list_set_same_set,
+       .cancel_gc = list_set_cancel_gc,
 };
 
 static void