ipset: list:set: Decrease refcount synchronously on deletion and replace

Commit 45040978c899 ("netfilter: ipset: Fix set:list type crash
when flush/dump set in parallel") postponed decreasing set
reference counters to the RCU callback.

An 'ipset del' command can terminate before the RCU grace period
is elapsed, and if sets are listed before then, the reference
counter shown in userspace will be wrong:

 # ipset create h hash:ip; ipset create l list:set; ipset add l
 # ipset del l h; ipset list h
 Name: h
 Type: hash:ip
 Revision: 4
 Header: family inet hashsize 1024 maxelem 65536
 Size in memory: 88
 References: 1
 Number of entries: 0
 Members:
 # sleep 1; ipset list h
 Name: h
 Type: hash:ip
 Revision: 4
 Header: family inet hashsize 1024 maxelem 65536
 Size in memory: 88
 References: 0
 Number of entries: 0
 Members:

Fix this by making the reference count update synchronous again.

As a result, when sets are listed, ip_set_name_byindex() might
now fetch a set whose reference count is already zero. Instead
of relying on the reference count to protect against concurrent
set renaming, grab ip_set_ref_lock as reader and copy the name,
while holding the same lock in ip_set_rename() as writer
instead.

Reported-by: Li Shuang <shuali@redhat.com>
Fixes: 45040978c899 ("netfilter: ipset: Fix set:list type crash when flush/dump set in parallel")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>

diff --git a/kernel/include/linux/netfilter/ipset/ip_set.h b/kernel/include/linux/netfilter/ipset/ip_set.h
index 3986e63111b339f6314ae5d2e1d6ca52d4a2f597..94f126c98cb6d056988637dbd0205aa6a1d10794 100644 (file)
--- a/kernel/include/linux/netfilter/ipset/ip_set.h
+++ b/kernel/include/linux/netfilter/ipset/ip_set.h
@@ -315,7 +315,7 @@ enum {
 extern ip_set_id_t ip_set_get_byname(struct net *net,
                                     const char *name, struct ip_set **set);
 extern void ip_set_put_byindex(struct net *net, ip_set_id_t index);
-extern const char *ip_set_name_byindex(struct net *net, ip_set_id_t index);
+extern void ip_set_name_byindex(struct net *net, ip_set_id_t index, char *name);
 extern ip_set_id_t ip_set_nfnl_get_byindex(struct net *net, ip_set_id_t index);
 extern void ip_set_nfnl_put(struct net *net, ip_set_id_t index);
 

diff --git a/kernel/net/netfilter/ipset/ip_set_core.c b/kernel/net/netfilter/ipset/ip_set_core.c
index 5de24b1b59ec6eee1df9110cc0c18c6cc93ccca8..ceca70609341060a605e394d8ae447a5ad9036dd 100644 (file)
--- a/kernel/net/netfilter/ipset/ip_set_core.c
+++ b/kernel/net/netfilter/ipset/ip_set_core.c
@@ -693,21 +693,20 @@ ip_set_put_byindex(struct net *net, ip_set_id_t index)
 EXPORT_SYMBOL_GPL(ip_set_put_byindex);
 
 /* Get the name of a set behind a set index.
- * We assume the set is referenced, so it does exist and
- * can't be destroyed. The set cannot be renamed due to
- * the referencing either.
- *
+ * Set itself is protected by RCU, but its name isn't: to protect against
+ * renaming, grab ip_set_ref_lock as reader (see ip_set_rename()) and copy the
+ * name.
  */
-const char *
-ip_set_name_byindex(struct net *net, ip_set_id_t index)
+void
+ip_set_name_byindex(struct net *net, ip_set_id_t index, char *name)
 {
-       const struct ip_set *set = ip_set_rcu_get(net, index);
+       struct ip_set *set = ip_set_rcu_get(net, index);
 
        BUG_ON(!set);
-       BUG_ON(set->ref == 0);
 
-       /* Referenced, so it's safe */
-       return set->name;
+       read_lock_bh(&ip_set_ref_lock);
+       strncpy(name, set->name, IPSET_MAXNAMELEN);
+       read_unlock_bh(&ip_set_ref_lock);
 }
 EXPORT_SYMBOL_GPL(ip_set_name_byindex);
 
@@ -1159,7 +1158,7 @@ IPSET_CBFN(ip_set_rename, struct net *net, struct sock *ctnl,
        if (!set)
                return -ENOENT;
 
-       read_lock_bh(&ip_set_ref_lock);
+       write_lock_bh(&ip_set_ref_lock);
        if (set->ref != 0) {
                ret = -IPSET_ERR_REFERENCED;
                goto out;
@@ -1176,7 +1175,7 @@ IPSET_CBFN(ip_set_rename, struct net *net, struct sock *ctnl,
        strncpy(set->name, name2, IPSET_MAXNAMELEN);
 
 out:
-       read_unlock_bh(&ip_set_ref_lock);
+       write_unlock_bh(&ip_set_ref_lock);
        return ret;
 }
 

diff --git a/kernel/net/netfilter/ipset/ip_set_list_set.c b/kernel/net/netfilter/ipset/ip_set_list_set.c
index e8c67dc20ee3e330fcd17040e7dbe188bd8e8074..4a744a559f20663e10d05598769f2fb34e6e91a8 100644 (file)
--- a/kernel/net/netfilter/ipset/ip_set_list_set.c
+++ b/kernel/net/netfilter/ipset/ip_set_list_set.c
@@ -150,9 +150,7 @@ __list_set_del_rcu(struct rcu_head * rcu)
 {
        struct set_elem *e = container_of(rcu, struct set_elem, rcu);
        struct ip_set *set = e->set;
-       struct list_set *map = set->data;
 
-       ip_set_put_byindex(map->net, e->id);
        ip_set_ext_destroy(set, e);
        kfree(e);
 }
@@ -160,15 +158,21 @@ __list_set_del_rcu(struct rcu_head * rcu)
 static inline void
 list_set_del(struct ip_set *set, struct set_elem *e)
 {
+       struct list_set *map = set->data;
+
        set->elements--;
        list_del_rcu(&e->list);
+       ip_set_put_byindex(map->net, e->id);
        call_rcu(&e->rcu, __list_set_del_rcu);
 }
 
 static inline void
-list_set_replace(struct set_elem *e, struct set_elem *old)
+list_set_replace(struct ip_set *set, struct set_elem *e, struct set_elem *old)
 {
+       struct list_set *map = set->data;
+
        list_replace_rcu(&old->list, &e->list);
+       ip_set_put_byindex(map->net, old->id);
        call_rcu(&old->rcu, __list_set_del_rcu);
 }
 
@@ -300,7 +304,7 @@ list_set_uadd(struct ip_set *set, void *value, const struct ip_set_ext *ext,
        INIT_LIST_HEAD(&e->list);
        list_set_init_extensions(set, ext, e);
        if (n)
-               list_set_replace(e, n);
+               list_set_replace(set, e, n);
        else if (next)
                list_add_tail_rcu(&e->list, &next->list);
        else if (prev)
@@ -488,6 +492,7 @@ list_set_list(const struct ip_set *set,
        const struct list_set *map = set->data;
        struct nlattr *atd, *nested;
        u32 i = 0, first = cb->args[IPSET_CB_ARG0];
+       char name[IPSET_MAXNAMELEN];
        struct set_elem *e;
        int ret = 0;
 
@@ -506,8 +511,8 @@ list_set_list(const struct ip_set *set,
                nested = ipset_nest_start(skb, IPSET_ATTR_DATA);
                if (!nested)
                        goto nla_put_failure;
-               if (nla_put_string(skb, IPSET_ATTR_NAME,
-                                  ip_set_name_byindex(map->net, e->id)))
+               ip_set_name_byindex(map->net, e->id, name);
+               if (nla_put_string(skb, IPSET_ATTR_NAME, name))
                        goto nla_put_failure;
                if (ip_set_put_extensions(skb, set, e, true))
                        goto nla_put_failure;