Skip to contentSkip to conversation

netfilter: ipset: add resched points during set listing

PATCH In review Florian Westphal 3 participants 3 messages Nov 30, 2017 – Dec 6, 2017

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Florian Westphal

author maintainer
#0 lore raw

PATCH 1/1 netfilter: ipset: add resched points during set listing

When sets are extremely large we can get softlockup during ipset -L. We could fix this by adding cond_resched_rcu() at the right location during iteration, but this only works if RCU nesting depth is 1.

At this time entire variant->list() is called under under rcu_read_lock_bh. This used to be a read_lock_bh() but as rcu doesn't really lock anything, it does not appear to be needed, so remove it (ipset increments set reference count before this, so a set deletion should not be possible).

Reported-by Li Shuang <[email protected]>
Signed-off-by Florian Westphal <[email protected]>
---
 net/netfilter/ipset/ip_set_bitmap_gen.h | 1 +
 net/netfilter/ipset/ip_set_core.c       | 2 --
 net/netfilter/ipset/ip_set_hash_gen.h   | 1 +
 3 files changed, 2 insertions(+), 2 deletions(-)
net/netfilter/ipset/ip_set_bitmap_gen.h +1 -0
index 5ca18f07683b5..8afe882f846d5 100644
@@ -227,6 +227,7 @@ mtype_list(const struct ip_set *set,
 	rcu_read_lock();
 	for (; cb->args[IPSET_CB_ARG0] < map->elements;
 	     cb->args[IPSET_CB_ARG0]++) {
+		cond_resched_rcu();
 		id = cb->args[IPSET_CB_ARG0];
 		x = get_ext(set, map, id);
 		if (!test_bit(id, map->members) ||
net/netfilter/ipset/ip_set_core.c +0 -2
index 1f3c03b3bebf2..89b44458a7613 100644
@@ -1388,9 +1388,7 @@ ip_set_dump_start(struct sk_buff *skb, struct netlink_callback *cb)
 				set->variant->uref(set, cb, true);
 			/* fall through */
 		default:
-			rcu_read_lock_bh();
 			ret = set->variant->list(set, skb, cb);
-			rcu_read_unlock_bh();
 			if (!cb->args[IPSET_CB_ARG0])
 				/* Set is done, proceed with next one */
 				goto next_set;
net/netfilter/ipset/ip_set_hash_gen.h +1 -0
index efffc8eabafea..8ef079db7d347 100644
@@ -1143,6 +1143,7 @@ mtype_list(const struct ip_set *set,
 	rcu_read_lock();
 	for (; cb->args[IPSET_CB_ARG0] < jhash_size(t->htable_bits);
 	     cb->args[IPSET_CB_ARG0]++) {
+		cond_resched_rcu();
 		incomplete = skb_tail_pointer(skb);
 		n = rcu_dereference(hbucket(t, cb->args[IPSET_CB_ARG0]));
 		pr_debug("cb->arg bucket: %lu, t %p n %p\n",
Signature · 1 line
2.14.3
Acked-by on v1 1/1
#1 lore raw

Hi Florian,

Quoted 9 lines · Florian Westphal

When sets are extremely large we can get softlockup during ipset -L. We could fix this by adding cond_resched_rcu() at the right location during iteration, but this only works if RCU nesting depth is 1.

At this time entire variant->list() is called under under rcu_read_lock_bh. This used to be a read_lock_bh() but as rcu doesn't really lock anything, it does not appear to be needed, so remove it (ipset increments set reference count before this, so a set deletion should not be possible).

Yes, the call of rcu_read_lock_bh() seems to be unnecessary, the set->variant->list() functions protect the sensitive parts with rcu_read_lock() anyway. Thanks!

Acked-by Jozsef Kadlecsik <[email protected]>

Best regards,
Jozsef

Quoted 54 lines
Reported-by Li Shuang <[email protected]>
Signed-off-by Florian Westphal <[email protected]>
---
 net/netfilter/ipset/ip_set_bitmap_gen.h | 1 +
 net/netfilter/ipset/ip_set_core.c       | 2 --
 net/netfilter/ipset/ip_set_hash_gen.h   | 1 +
 3 files changed, 2 insertions(+), 2 deletions(-)
net/netfilter/ipset/ip_set_bitmap_gen.h +1 -0
index 5ca18f07683b5..8afe882f846d5 100644
@@ -227,6 +227,7 @@ mtype_list(const struct ip_set *set,
 	rcu_read_lock();
 	for (; cb->args[IPSET_CB_ARG0] < map->elements;
 	     cb->args[IPSET_CB_ARG0]++) {
+		cond_resched_rcu();
 		id = cb->args[IPSET_CB_ARG0];
 		x = get_ext(set, map, id);
 		if (!test_bit(id, map->members) ||
net/netfilter/ipset/ip_set_core.c +0 -2
index 1f3c03b3bebf2..89b44458a7613 100644
@@ -1388,9 +1388,7 @@ ip_set_dump_start(struct sk_buff *skb, struct netlink_callback *cb)
 				set->variant->uref(set, cb, true);
 			/* fall through */
 		default:
-			rcu_read_lock_bh();
 			ret = set->variant->list(set, skb, cb);
-			rcu_read_unlock_bh();
 			if (!cb->args[IPSET_CB_ARG0])
 				/* Set is done, proceed with next one */
 				goto next_set;
net/netfilter/ipset/ip_set_hash_gen.h +1 -0
index efffc8eabafea..8ef079db7d347 100644
@@ -1143,6 +1143,7 @@ mtype_list(const struct ip_set *set,
 	rcu_read_lock();
 	for (; cb->args[IPSET_CB_ARG0] < jhash_size(t->htable_bits);
 	     cb->args[IPSET_CB_ARG0]++) {
+		cond_resched_rcu();
 		incomplete = skb_tail_pointer(skb);
 		n = rcu_dereference(hbucket(t, cb->args[IPSET_CB_ARG0]));
 		pr_debug("cb->arg bucket: %lu, t %p n %p\n",
Signature · 6 lines
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

-
E-mail : [email protected], [email protected] 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
maintainer
#2 lore raw
Quoted 14 lines · Jozsef Kadlecsik

Hi Florian,

Quoted 9 lines · Florian Westphal

When sets are extremely large we can get softlockup during ipset -L. We could fix this by adding cond_resched_rcu() at the right location during iteration, but this only works if RCU nesting depth is 1.

At this time entire variant->list() is called under under rcu_read_lock_bh. This used to be a read_lock_bh() but as rcu doesn't really lock anything, it does not appear to be needed, so remove it (ipset increments set reference count before this, so a set deletion should not be possible).

Quoted 5 lines

Yes, the call of rcu_read_lock_bh() seems to be unnecessary, the set->variant->list() functions protect the sensitive parts with rcu_read_lock() anyway. Thanks!

Acked-by Jozsef Kadlecsik <[email protected]>

Also applied, thanks.