[PATCH nf] netfilter: ipset: restore element before rescanning cidrs on seqcount retry

[PATCH nf] netfilter: ipset: restore element before rescanning cidrs on seqcount retry

[Tamaki Yanagawa]

mtype_test_cidrs() masks the element under test in place with
mtype_data_netmask() while walking the cidr array from the most to the
least specific prefix. Since the walk is restarted on concurrent cidr
array updates, the IPSET_NET_COUNT == 1 types (hash:net, hash:net,port,
hash:ip,port,net, hash:net,iface) rescan with an element already masked
to the widest prefix tried, so a lookup can wrongly match a more specific
entry at the network base address (e.g. 10.200.3.4 matching 10.0.0.0/29).
Restore the element before rescanning.

Fixes: e80456d79ec8 ("netfilter: ipset: remove need to allocate memory on delete operations")
Signed-off-by: Tamaki Yanagawa <ty@000ty.net>
Assisted-by: Claude:claude-opus-5-5
---
 net/netfilter/ipset/ip_set_hash_gen.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_hash_gen.h b/net/netfilter/ipset/ip_set_hash_gen.h
index 80ca523f30..4f94443dcd 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -1307,9 +1307,9 @@ mtype_test_cidrs(struct ip_set *set, struct mtype_elem *d,
 	struct net_prefixes *nets0;
 	struct hbucket *n;
 	struct mtype_elem *data;
+	struct mtype_elem orig = *d;
 #if IPSET_NET_COUNT == 2
 	struct net_prefixes *nets1;
-	struct mtype_elem orig = *d;
 	unsigned int seq1;
 	int ret, i, j, k;
 #else
@@ -1372,13 +1372,17 @@ mtype_test_cidrs(struct ip_set *set, struct mtype_elem *d,
 	ret = 0;
 unlock:
 	if (read_seqcount_retry(&nets0->seq, seq0))
-		goto retry;
+		goto restore;
 #if IPSET_NET_COUNT == 2
 	if (read_seqcount_retry(&nets1->seq, seq1))
-		goto retry;
+		goto restore;
 #endif
 	rcu_read_unlock_bh();
 	return ret;
+restore:
+	/* mtype_data_netmask() masks @d in place, undo it before rescanning */
+	*d = orig;
+	goto retry;
 }
 #endif
 

base-commit: ab699d296e066eb64f51d2a8a1d42237a3b186c8
-- 
2.52.0.windows.1

Re: [PATCH nf] netfilter: ipset: restore element before rescanning cidrs on seqcount retry

[Florian Westphal]

Tamaki Yanagawa <ty@000ty.net> wrote:
> mtype_test_cidrs() masks the element under test in place with
> mtype_data_netmask() while walking the cidr array from the most to the
> least specific prefix. Since the walk is restarted on concurrent cidr
> array updates, the IPSET_NET_COUNT == 1 types (hash:net, hash:net,port,
> hash:ip,port,net, hash:net,iface) rescan with an element already masked
> to the widest prefix tried, so a lookup can wrongly match a more specific
> entry at the network base address (e.g. 10.200.3.4 matching 10.0.0.0/29).
> Restore the element before rescanning.

Is this a real world encountered bug or an LLM finding?

> +	/* mtype_data_netmask() masks @d in place, undo it before rescanning */
> +	*d = orig;
> +	goto retry;

Why do we need to do this instead of not masking @d in place to begin
with?

-mtype_test_cidrs(struct ip_set *set, struct mtype_elem *d,
+mtype_test_cidrs(struct ip_set *set, const struct mtype_elem *orig,

+       struct mtype_elem *data, d;
        struct hbucket *n;
-       struct mtype_elem *data;

 retry:
+       d = *orig;


... and fix up all reset calls:

 static void
 hash_netportnet6_data_reset_elem(struct hash_netportnet6_elem *elem,
-                                struct hash_netportnet6_elem *orig)
+                                const struct hash_netportnet6_elem *orig)

... much cleaner and less error prone, I think.

Re: [PATCH nf] netfilter: ipset: restore element before rescanning cidrs on seqcount retry

[Tamaki Yanagawa]

Florian Westphal <fw@strlen.de> wrote:
> Is this a real world encountered bug or an LLM finding?

An LLM finding (Claude, hence the Assisted-by tag), I haven't hit it in
production. I did confirm it on a KVM guest though: a hash:net set with
10.0.0.0/29 and 11.0.0.0/8, an "iptables -m set --match-set s dst" rule,
a loop adding and deleting 172.16.0.0/12, and UDP traffic to 10.200.3.4.
70 out of ~9.5M packets matched the rule in 60 seconds without the fix,
none with it.

> Why do we need to do this instead of not masking @d in place to begin
> with?

No good reason, your version is cleaner. I'll send a v2 that works on a
local copy and constifies the reset_elem helpers.

Thanks,
Tamaki

Re: [PATCH nf] netfilter: ipset: restore element before rescanning cidrs on seqcount retry

[Florian Westphal]

Tamaki Yanagawa <ty@000ty.net> wrote:
> No good reason, your version is cleaner. I'll send a v2 that works on a
> local copy and constifies the reset_elem helpers.

Thanks.