| Message ID | 37edf6674675cf3543d5026ae4412a085c54a2f4.1530173328.git.sbrivio@redhat.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Jozsef Kadlecsik |
| Headers | show |
| Series | netfilter: ipset: list:set: Decrease refcount synchronously on deletion and replace | expand |
Hi, On Thu, 28 Jun 2018, Stefano Brivio wrote: > 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 and listing, note that those two operations are > serialised by the nfnl mutex, and that the set itself is > protected by RCU nowadays. Listing is not serialized by the nfnl mutex because a netlink dump is used behind it. So I believe the patch is not correct and therefore I cannot apply it. Best regards, Jozsef > 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> > --- > net/netfilter/ipset/ip_set_core.c | 7 +------ > net/netfilter/ipset/ip_set_list_set.c | 12 ++++++++---- > 2 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c > index bc4bd247bb7d..afe42af0ad13 100644 > --- a/net/netfilter/ipset/ip_set_core.c > +++ b/net/netfilter/ipset/ip_set_core.c > @@ -693,10 +693,7 @@ 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. > - * > + * This can only be called by operations serialised by nfnl mutex. > */ > const char * > ip_set_name_byindex(struct net *net, ip_set_id_t index) > @@ -704,9 +701,7 @@ ip_set_name_byindex(struct net *net, ip_set_id_t index) > const 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; > } > EXPORT_SYMBOL_GPL(ip_set_name_byindex); > diff --git a/net/netfilter/ipset/ip_set_list_set.c b/net/netfilter/ipset/ip_set_list_set.c > index 072a658fde04..9a056729968f 100644 > --- a/net/netfilter/ipset/ip_set_list_set.c > +++ b/net/netfilter/ipset/ip_set_list_set.c > @@ -148,9 +148,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); > } > @@ -158,14 +156,20 @@ __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--; > + ip_set_put_byindex(map->net, e->id); > list_del_rcu(&e->list); > 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; > + > + ip_set_put_byindex(map->net, old->id); > list_replace_rcu(&old->list, &e->list); > call_rcu(&old->rcu, __list_set_del_rcu); > } > @@ -298,7 +302,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) > -- > 2.15.1 > > -- > To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, 2 Jul 2018 19:56:37 +0200 (CEST) Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > > 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 and listing, note that those two operations are > > serialised by the nfnl mutex, and that the set itself is > > protected by RCU nowadays. > > Listing is not serialized by the nfnl mutex because a netlink dump is used > behind it. So I believe the patch is not correct and therefore I cannot > apply it. Thanks for checking. But my understanding is that the list operation still goes through nfnetlink, and nfnetlink_rcv_msg() takes the nfnl mutex. I tested this assumption by checking that ip_set_name_byindex() and ip_set_rename() don't run concurrently if I run concurrent loops of 'ipset list' and 'ipset rename', and also by adding a: WARN_ON(subsys_id == NFNL_SUBSYS_IPSET); in nfnl_lock() and nfnl_unlock(), and it triggers on 'ipset list'. What am I missing?
On Tue, Jul 03, 2018 at 02:05:12PM +0200, Stefano Brivio wrote: > Hi, > > On Mon, 2 Jul 2018 19:56:37 +0200 (CEST) > Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > > > > 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 and listing, note that those two operations are > > > serialised by the nfnl mutex, and that the set itself is > > > protected by RCU nowadays. > > > > Listing is not serialized by the nfnl mutex because a netlink dump is used > > behind it. So I believe the patch is not correct and therefore I cannot > > apply it. > > Thanks for checking. But my understanding is that the list operation > still goes through nfnetlink, and nfnetlink_rcv_msg() takes the nfnl > mutex. > > I tested this assumption by checking that ip_set_name_byindex() and > ip_set_rename() don't run concurrently if I run concurrent loops of > 'ipset list' and 'ipset rename', and also by adding a: > > WARN_ON(subsys_id == NFNL_SUBSYS_IPSET); > > in nfnl_lock() and nfnl_unlock(), and it triggers on 'ipset list'. What > am I missing? Only the netlink_recvmsg() first call is protected under nfnl lock, follow up calls happen from the netlink_dump() path which in netfilter is rcu based. We have callbacks in nfnetlink to achieve full rcu dumps. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 4 Jul 2018 18:54:55 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Jul 03, 2018 at 02:05:12PM +0200, Stefano Brivio wrote: > > Hi, > > > > On Mon, 2 Jul 2018 19:56:37 +0200 (CEST) > > Jozsef Kadlecsik <kadlec@blackhole.kfki.hu> wrote: > > > > > > 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 and listing, note that those two operations are > > > > serialised by the nfnl mutex, and that the set itself is > > > > protected by RCU nowadays. > > > > > > Listing is not serialized by the nfnl mutex because a netlink dump is used > > > behind it. So I believe the patch is not correct and therefore I cannot > > > apply it. > > > > Thanks for checking. But my understanding is that the list operation > > still goes through nfnetlink, and nfnetlink_rcv_msg() takes the nfnl > > mutex. > > > > I tested this assumption by checking that ip_set_name_byindex() and > > ip_set_rename() don't run concurrently if I run concurrent loops of > > 'ipset list' and 'ipset rename', and also by adding a: > > > > WARN_ON(subsys_id == NFNL_SUBSYS_IPSET); > > > > in nfnl_lock() and nfnl_unlock(), and it triggers on 'ipset list'. What > > am I missing? > > Only the netlink_recvmsg() first call is protected under nfnl lock, > follow up calls happen from the netlink_dump() path which in netfilter > is rcu based. Of course, I see now, thanks for the explanation! > We have callbacks in nfnetlink to achieve full rcu dumps. So I guess you are suggesting to change ipset dumps to use those callbacks. Unless Jozsef has any objection, I would, at least for the moment being, fix this specific issue in another way (i.e. using the ref_netlink refcount as it's already done for swap and delete operations), and then at some point consider again this idea.
Hi, On Thu, 5 Jul 2018, Stefano Brivio wrote: > > Only the netlink_recvmsg() first call is protected under nfnl lock, > > follow up calls happen from the netlink_dump() path which in netfilter > > is rcu based. > > Of course, I see now, thanks for the explanation! > > > We have callbacks in nfnetlink to achieve full rcu dumps. > > So I guess you are suggesting to change ipset dumps to use those > callbacks. ipset uses the same infrastructure, callbacks. Dumping is quite different from the other operations in the netlink world. > Unless Jozsef has any objection, I would, at least for the moment > being, fix this specific issue in another way (i.e. using the > ref_netlink refcount as it's already done for swap and delete > operations), and then at some point consider again this idea. No objections from me :-). Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c index bc4bd247bb7d..afe42af0ad13 100644 --- a/net/netfilter/ipset/ip_set_core.c +++ b/net/netfilter/ipset/ip_set_core.c @@ -693,10 +693,7 @@ 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. - * + * This can only be called by operations serialised by nfnl mutex. */ const char * ip_set_name_byindex(struct net *net, ip_set_id_t index) @@ -704,9 +701,7 @@ ip_set_name_byindex(struct net *net, ip_set_id_t index) const 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; } EXPORT_SYMBOL_GPL(ip_set_name_byindex); diff --git a/net/netfilter/ipset/ip_set_list_set.c b/net/netfilter/ipset/ip_set_list_set.c index 072a658fde04..9a056729968f 100644 --- a/net/netfilter/ipset/ip_set_list_set.c +++ b/net/netfilter/ipset/ip_set_list_set.c @@ -148,9 +148,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); } @@ -158,14 +156,20 @@ __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--; + ip_set_put_byindex(map->net, e->id); list_del_rcu(&e->list); 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; + + ip_set_put_byindex(map->net, old->id); list_replace_rcu(&old->list, &e->list); call_rcu(&old->rcu, __list_set_del_rcu); } @@ -298,7 +302,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)
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 and listing, note that those two operations are serialised by the nfnl mutex, and that the set itself is protected by RCU nowadays. 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> --- net/netfilter/ipset/ip_set_core.c | 7 +------ net/netfilter/ipset/ip_set_list_set.c | 12 ++++++++---- 2 files changed, 9 insertions(+), 10 deletions(-)