netfilter: ipset: Fix race between namespace cleanup and gc in the list:set type

Lion Ackermann reported that there is a race condition between namespace cleanup
in ipset and the garbage collection of the list:set type. The namespace
cleanup can destroy the list:set type of sets while the gc of the set type is
waiting to run in rcu cleanup. The latter uses data from the destroyed set which
thus leads use after free. The patch contains the following parts:

- When destroying all sets, first remove the garbage collectors, then wait
  if needed and then destroy the sets.
- Fix the badly ordered "wait then remove gc" for the destroy a single set
  case.
- Fix the missing rcu locking in the list:set type in the userspace test
  case.
- Use proper RCU list handlings in the list:set type.

The patch depends on 975403cda657 (netfilter: ipset: Add list flush to cancel_gc).

Fixes: fdb8e12cc2cc (netfilter: ipset: fix performance regression in swap operation)
Reported-by: Lion Ackermann <nnamrec@gmail.com>
Tested-by: Lion Ackermann <nnamrec@gmail.com>
Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>

diff --git a/kernel/net/netfilter/ipset/ip_set_core.c b/kernel/net/netfilter/ipset/ip_set_core.c
index a99c1bdf2aff29e58f19e3e06712c5d7a569f1d4..9ddb59410ac7f6ff41a8349cc51c51da73596dda 100644 (file)
--- a/kernel/net/netfilter/ipset/ip_set_core.c
+++ b/kernel/net/netfilter/ipset/ip_set_core.c
@@ -1182,48 +1182,50 @@ ip_set_setname_policy[IPSET_ATTR_CMD_MAX + 1] = {
                                    .len = IPSET_MAXNAMELEN - 1 },
 };
 
-/* Destroying a set is split into two stages when a DESTROY command issued:
- * - Cancel garbage collectors and decrement the module reference counter:
- *     - Cancelling may wait and we are allowed to do it at this stage.
- *     - Module remove is protected by rcu_barrier() which waits for
- *       the second stage to be finished.
- * - In order to prevent the race between kernel side add/del/test element
- *   operations and destroy, the destroying of the set data areas are
- *   performed via a call_rcu() call.
+/* In order to return quickly when destroying a single set, it is split
+ * into two stages:
+ * - Cancel garbage collector
+ * - Destroy the set itself via call_rcu()
  */
 
-/* Call set variant specific destroy function and reclaim the set data. */
 static void
-ip_set_destroy_set_variant(struct ip_set *set)
-{
-       /* Must call it without holding any lock */
-       set->variant->destroy(set);
-       kfree(set);
-}
-
-static void
-ip_set_destroy_set_variant_rcu(struct rcu_head *head)
+ip_set_destroy_set_rcu(struct rcu_head *head)
 {
        struct ip_set *set = container_of(head, struct ip_set, rcu);
 
-       ip_set_destroy_set_variant(set);
-}
-
-/* Cancel the garbage collectors and decrement module references */
-static void
-ip_set_destroy_cancel_gc(struct ip_set *set)
-{
-       set->variant->cancel_gc(set);
+       set->variant->destroy(set);
        module_put(set->type->me);
+       kfree(set);
 }
 
-/* Use when we may wait for the complete destroy to be finished.
- */
 static void
-ip_set_destroy_set(struct ip_set *set)
+_destroy_all_sets(struct ip_set_net *inst)
 {
-       ip_set_destroy_cancel_gc(set);
-       ip_set_destroy_set_variant(set);
+       struct ip_set *set;
+       ip_set_id_t i;
+       bool need_wait = false;
+
+       /* First cancel gc's: set:list sets are flushed as well */
+       for (i = 0; i < inst->ip_set_max; i++) {
+               set = ip_set(inst, i);
+               if (set) {
+                       set->variant->cancel_gc(set);
+                       if (set->type->features & IPSET_TYPE_NAME)
+                               need_wait = true;
+               }
+       }
+       /* Must wait for flush to be really finished  */
+       if (need_wait)
+               rcu_barrier();
+       for (i = 0; i < inst->ip_set_max; i++) {
+               set = ip_set(inst, i);
+               if (set) {
+                       ip_set(inst, i) = NULL;
+                       set->variant->destroy(set);
+                       module_put(set->type->me);
+                       kfree(set);
+               }
+       }
 }
 
 static int
@@ -1244,7 +1246,7 @@ IPSET_CBFN(ip_set_destroy, struct net *net, struct sock *ctnl,
        /* Commands are serialized and references are
         * protected by the ip_set_ref_lock.
         * External systems (i.e. xt_set) must call
-        * ip_set_put|get_nfnl_* functions, that way we
+        * ip_set_nfnl_get_* functions, that way we
         * can safely check references here.
         *
         * list:set timer can only decrement the reference
@@ -1252,8 +1254,6 @@ IPSET_CBFN(ip_set_destroy, struct net *net, struct sock *ctnl,
         * without holding the 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);
@@ -1264,13 +1264,7 @@ IPSET_CBFN(ip_set_destroy, struct net *net, struct sock *ctnl,
                }
                inst->is_destroyed = true;
                read_unlock_bh(&ip_set_ref_lock);
-               for (i = 0; i < inst->ip_set_max; i++) {
-                       s = ip_set(inst, i);
-                       if (s) {
-                               ip_set(inst, i) = NULL;
-                               ip_set_destroy_set(s);
-                       }
-               }
+               _destroy_all_sets(inst);
                /* Modified by ip_set_destroy() only, which is serialized */
                inst->is_destroyed = false;
        } else {
@@ -1291,13 +1285,13 @@ IPSET_CBFN(ip_set_destroy, struct net *net, struct sock *ctnl,
                features = s->type->features;
                ip_set(inst, i) = NULL;
                read_unlock_bh(&ip_set_ref_lock);
+               /* Must cancel garbage collectors */
+               s->variant->cancel_gc(s);
                if (features & IPSET_TYPE_NAME) {
                        /* Must wait for flush to be really finished  */
                        rcu_barrier();
                }
-               /* Must cancel garbage collectors */
-               ip_set_destroy_cancel_gc(s);
-               call_rcu(&s->rcu, ip_set_destroy_set_variant_rcu);
+               call_rcu(&s->rcu, ip_set_destroy_set_rcu);
        }
        return 0;
 out:
@@ -1835,6 +1829,7 @@ CALL_AD(struct net *net, struct sock *ctnl, struct sk_buff *skb,
                errmsg->error = ret;
                unsafe_memcpy(&errmsg->msg, nlh, nlh->nlmsg_len,
                              /* Bounds checked by the skb layer. */);
+
                cmdattr = (void *)&errmsg->msg + min_len;
 
                ret = NLA_PARSE(cda, IPSET_ATTR_CMD_MAX, cmdattr,
@@ -2497,24 +2492,19 @@ err_alloc:
 }
 
 static void __net_exit
-ip_set_net_exit(struct net *net)
+ip_set_net_pre_exit(struct net *net)
 {
        struct ip_set_net *inst = ip_set_pernet(net);
 
-       struct ip_set *set = NULL;
-       ip_set_id_t i;
-
        inst->is_deleted = true; /* flag for ip_set_nfnl_put */
+}
 
-       nfnl_lock(NFNL_SUBSYS_IPSET);
-       for (i = 0; i < inst->ip_set_max; i++) {
-               set = ip_set(inst, i);
-               if (set) {
-                       ip_set(inst, i) = NULL;
-                       ip_set_destroy_set(set);
-               }
-       }
-       nfnl_unlock(NFNL_SUBSYS_IPSET);
+static void __net_exit
+ip_set_net_exit(struct net *net)
+{
+       struct ip_set_net *inst = ip_set_pernet(net);
+
+       _destroy_all_sets(inst);
        kvfree(rcu_dereference_protected(inst->ip_set_list, 1));
 #ifndef HAVE_NET_OPS_ID
        kvfree(inst);
@@ -2523,6 +2513,7 @@ ip_set_net_exit(struct net *net)
 
 static struct pernet_operations ip_set_net_ops = {
        .init   = ip_set_net_init,
+       .pre_exit = ip_set_net_pre_exit,
        .exit   = ip_set_net_exit,
 #ifdef HAVE_NET_OPS_ID
        .id     = &ip_set_net_id,

diff --git a/kernel/net/netfilter/ipset/ip_set_list_set.c b/kernel/net/netfilter/ipset/ip_set_list_set.c
index 0d15f4f00a4f99fab1ec11811d8e99f74e0d66f4..fd78e2db06e4f88dffb60372dcf87fc66c4bab53 100644 (file)
--- a/kernel/net/netfilter/ipset/ip_set_list_set.c
+++ b/kernel/net/netfilter/ipset/ip_set_list_set.c
@@ -82,7 +82,7 @@ list_set_kadd(struct ip_set *set, const struct sk_buff *skb,
        struct set_elem *e;
        int ret;
 
-       list_for_each_entry(e, &map->members, list) {
+       list_for_each_entry_rcu(e, &map->members, list) {
                if (SET_WITH_TIMEOUT(set) &&
                    ip_set_timeout_expired(ext_timeout(e, set)))
                        continue;
@@ -102,7 +102,7 @@ list_set_kdel(struct ip_set *set, const struct sk_buff *skb,
        struct set_elem *e;
        int ret;
 
-       list_for_each_entry(e, &map->members, list) {
+       list_for_each_entry_rcu(e, &map->members, list) {
                if (SET_WITH_TIMEOUT(set) &&
                    ip_set_timeout_expired(ext_timeout(e, set)))
                        continue;
@@ -191,9 +191,10 @@ list_set_utest(struct ip_set *set, void *value, const struct ip_set_ext *ext,
        struct list_set *map = set->data;
        struct set_adt_elem *d = value;
        struct set_elem *e, *next, *prev = NULL;
-       int ret;
+       int ret = 0;
 
-       list_for_each_entry(e, &map->members, list) {
+       rcu_read_lock();
+       list_for_each_entry_rcu(e, &map->members, list) {
                if (SET_WITH_TIMEOUT(set) &&
                    ip_set_timeout_expired(ext_timeout(e, set)))
                        continue;
@@ -204,6 +205,7 @@ list_set_utest(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 
                if (d->before == 0) {
                        ret = 1;
+                       goto out;
                } else if (d->before > 0) {
                        next = list_next_entry(e, list);
                        ret = !list_is_last(&e->list, &map->members) &&
@@ -211,9 +213,11 @@ list_set_utest(struct ip_set *set, void *value, const struct ip_set_ext *ext,
                } else {
                        ret = prev && prev->id == d->refid;
                }
-               return ret;
+               goto out;
        }
-       return 0;
+out:
+       rcu_read_unlock();
+       return ret;
 }
 
 static void
@@ -242,7 +246,7 @@ list_set_uadd(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 
        /* Find where to add the new entry */
        n = prev = next = NULL;
-       list_for_each_entry(e, &map->members, list) {
+       list_for_each_entry_rcu(e, &map->members, list) {
                if (SET_WITH_TIMEOUT(set) &&
                    ip_set_timeout_expired(ext_timeout(e, set)))
                        continue;
@@ -319,9 +323,9 @@ list_set_udel(struct ip_set *set, void *value, const struct ip_set_ext *ext,
 {
        struct list_set *map = set->data;
        struct set_adt_elem *d = value;
-       struct set_elem *e, *next, *prev = NULL;
+       struct set_elem *e, *n, *next, *prev = NULL;
 
-       list_for_each_entry(e, &map->members, list) {
+       list_for_each_entry_safe(e, n, &map->members, list) {
                if (SET_WITH_TIMEOUT(set) &&
                    ip_set_timeout_expired(ext_timeout(e, set)))
                        continue;
@@ -427,14 +431,8 @@ static void
 list_set_destroy(struct ip_set *set)
 {
        struct list_set *map = set->data;
-       struct set_elem *e, *n;
 
-       list_for_each_entry_safe(e, n, &map->members, list) {
-               list_del(&e->list);
-               ip_set_put_byindex(map->net, e->id);
-               ip_set_ext_destroy(set, e);
-               kfree(e);
-       }
+       BUG_ON(!list_empty(&map->members));
        kfree(map);
 
        set->data = NULL;