Re: [PATCH net 06/10] netfilter: ipset: add and use mtype_del_cidr_all helper

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: ipset: add and use mtype_del_cidr_all helper

This adds a mtype_del_cidr_all() helper that wraps the per-net-dimension
loop calling mtype_del_cidr(), and converts the three open-coded loops in
mtype_gc_do(), mtype_add() and mtype_del() to use it.  No functional
change is intended.

> diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
> index ef586b486f519..f00c82acd7f05 100644
> --- a/net/netfilter/ipset/ip_set_hash_gen.h
> +++ b/net/netfilter/ipset/ip_set_hash_gen.h

[ ... ]

> @@ -410,6 +412,17 @@ mtype_del_cidr(struct ip_set *set, struct htype *h, u8 cidr, u8 n)
>  }
>  #endif
>  
> +static void
> +mtype_del_cidr_all(struct ip_set *set, struct htype *h, const struct mtype_elem *data)
> +{
> +#ifdef IP_SET_HASH_WITH_NETS
> +	int k;
> +
> +	for (k = 0; k < IPSET_NET_COUNT; k++)
> +		mtype_del_cidr(set, h, DCIDR_GET(data->cidr, k), k);
> +#endif
> +}

[Severity: Critical]

This helper makes mtype_del_cidr() the single delete-side writer of
h->rnets[], so it may be worth asking about the locking of that array.
The problem does not originate in this patch but in the earlier commit of
this series, "netfilter: ipset: rework cidr bookkeeping", which turned the
fixed-size h->nets[] into an RCU-published, reallocated struct
net_prefixes; this patch routes all three delete sites into one of the
racing writers.

mtype_add_cidr() and mtype_del_cidr() both serialize on set->lock:

	spin_lock_bh(&set->lock);
	nets = __ipset_dereference(h->rnets[n]);
	...
	rcu_assign_pointer(h->rnets[n], tmp);
	kfree_rcu(nets, rcu);
unlock:
	spin_unlock_bh(&set->lock);

mtype_flush() does the same read-allocate-publish-free sequence with no
set->lock held:

	for (i = 0; i < IPSET_NET_COUNT; i++) {
		nets = ipset_dereference_nfnl(h->rnets[i]);
		tmp = kzalloc_obj(*tmp, GFP_ATOMIC);
		if (!tmp) {
			u8 j;

			for (j = 0; j < nets->len; j++)
				nets->nets[j].count = 0;
		} else {
			rcu_assign_pointer(h->rnets[i], tmp);
			kfree_rcu(nets, rcu);
		}
	}

ip_set_flush_set() does call ip_set_lock(set), but for the hash variants
(.region_lock = true) that is a no-op:

net/netfilter/ipset/ip_set_core.c:
	static inline void
	ip_set_lock(struct ip_set *set)
	{
		if (!set->variant->region_lock)
			spin_lock_bh(&set->lock);
	}

The hregion locks flush does take are all released before the rnets loop
runs, and mtype_add_cidr()/mtype_del_cidr() do not take hregion locks.

Can this interleaving happen?

CPU0 (ipset flush, process context, nfnl mutex only)
	nets_old = ipset_dereference_nfnl(h->rnets[i]);

CPU1 (SET target kadt in softirq -> mtype_add() -> mtype_add_cidr(),
      or gc worker -> mtype_gc_do() -> mtype_del_cidr_all())
	spin_lock_bh(&set->lock);
	nets_old = __ipset_dereference(h->rnets[n]);
	rcu_assign_pointer(h->rnets[n], tmp1);
	kfree_rcu(nets_old, rcu);
	spin_unlock_bh(&set->lock);

CPU0
	rcu_assign_pointer(h->rnets[i], tmp0);	/* tmp1 leaked */
	kfree_rcu(nets_old, rcu);		/* rcu_head reused */

Does the second kfree_rcu() on an object whose rcu_head is already queued
corrupt the callback list and double free the net_prefixes?  And does the
tmp0 publish also drop the accounting update CPU1 just made in tmp1?

On the flush allocation-failure path, flush writes nets->nets[j].count = 0
in place while mtype_del_cidr() may be doing nets->nets[found].count-- and
mtype_add_cidr() nets->nets[i].count++ on the same object.  Can that tear
the counts and the __counted_by(len) contents?

[ ... ]

> @@ -1004,11 +1010,7 @@ mtype_add(struct ip_set *set, void *value, const struct ip_set_ext *ext,
>  			j = 0;
>  		data = ahash_data(n, j, set->dsize);
>  		if (!deleted) {
> -#ifdef IP_SET_HASH_WITH_NETS
> -			for (i = 0; i < IPSET_NET_COUNT; i++)
> -				mtype_del_cidr(set, h,
> -					DCIDR_GET(data->cidr, i), i);
> -#endif
> +			mtype_del_cidr_all(set, h, data);
>  			ip_set_ext_destroy(set, data);
>  			t->hregion[r].elements--;
>  		}

[Severity: High]

Related to the decrement side this patch factors out: the matching
increment in mtype_add() ignores the new error return of
mtype_add_cidr().  As with the previous comment, this comes from
"netfilter: ipset: rework cidr bookkeeping" earlier in the series, which
changed mtype_add_cidr() from a void in-place update into an int that
allocates a replacement net_prefixes with GFP_ATOMIC and can return
-ENOMEM.

The insert path in mtype_add() still reads:

	#ifdef IP_SET_HASH_WITH_NETS
		for (i = 0; i < IPSET_NET_COUNT; i++)
			mtype_add_cidr(set, h, DCIDR_GET(d->cidr, i), i);
	#endif
		memcpy(data, d, sizeof(struct mtype_elem));

while mtype_add_cidr() can fail:

	tmp = kzalloc_flex(*tmp, nets, len, GFP_ATOMIC);
	if (!tmp) {
		ret = -ENOMEM;
		goto unlock;
	}

ret in mtype_add() stays 0, so ipset add reports success and the element
shows up in ipset list.  But mtype_test_cidrs() only builds lookup keys
from prefixes recorded in h->rnets[]:

	for (j = 0; j < nets0->len && !multi; j++) {
		if (!nets0->nets[j].count)
			continue;
		...
		key = HKEY(d, h->initval, t->htable_bits);

Can this leave an element that never matches a packet, even though
userspace was told the add succeeded?

There also seems to be a follow-on effect on unrelated members.  Since
mtype_add_cidr() only allocates when the cidr is absent, a failure means
that cidr is not registered at all.  If another element later registers
the same cidr (count = 1) and the unregistered element is then deleted,
mtype_del_cidr_all() -> mtype_del_cidr() finds that shared entry and does:

	nets->nets[found].count--;

Does that drop the count to 0 and remove a prefix that live elements still
use, so those elements stop matching too?

And in the case where mtype_del_cidr()'s own kzalloc_flex() previously
failed and left a hole (a count == 0 entry that keeps its cidr), deleting
the unregistered element decrements a count of 0.  Does that underflow the
u32 to 0xFFFFFFFF and pin the prefix in the lookup list permanently?

Would propagating the mtype_add_cidr() return value (and undoing the
already-successful increments) be preferable here?