[PATCH nft-testing v3 0/2] netfilter: nft_set_pipapo: Use nested-BH locking for nft_pipapo_scratch

[PATCH nft-testing v3 0/2] netfilter: nft_set_pipapo: Use nested-BH locking for nft_pipapo_scratch

[Sebastian Andrzej Siewior]

The pipapo set type uses a per-CPU scratch buffer which is protected
only by disabling BH. This series adds nested-BH locking which is only
used on PREEMPT_RT.

v2…v3: https://lore.kernel.org/all/20250815160937.1192748-1-bigeasy@linutronix.de
     - dropped applied patches while rebasing to nf-testing
     - move kernel_fpu_begin() to one place so the locks need to be
       added in spot.
 
v1…v2: https://lore.kernel.org/all/20250701221304.3846333-1-bigeasy@linutronix.de
     - rebase on top of nf-next.

Sebastian Andrzej Siewior (2):
  netfilter: nft_set_pipapo*: Move FPU handling to pipapo_get_avx2()
  netfilter: nft_set_pipapo: Use nested-BH locking for
    nft_pipapo_scratch

 net/netfilter/nft_set_pipapo.c      |  7 +++++--
 net/netfilter/nft_set_pipapo.h      |  1 +
 net/netfilter/nft_set_pipapo_avx2.c | 19 +++++++++++++------
 3 files changed, 19 insertions(+), 8 deletions(-)

-- 
2.50.1

[PATCH nft-testing v3 1/2] netfilter: nft_set_pipapo*: Move FPU handling to pipapo_get_avx2()

[Sebastian Andrzej Siewior]

Move kernel_fpu_begin_mask()/ _end() to pipapo_get_avx2() where it is
required.
This is a preparation for adding local_lock_t to struct
nft_pipapo_scratch in order to protect the __map pointer. The lock can
not be acquired in preemption disabled context which is what
kernel_fpu_begin*() does.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/netfilter/nft_set_pipapo.c      |  2 --
 net/netfilter/nft_set_pipapo_avx2.c | 15 +++++++++------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
index 515eb64bff9f8..1a1ba0b47696e 100644
--- a/net/netfilter/nft_set_pipapo.c
+++ b/net/netfilter/nft_set_pipapo.c
@@ -528,9 +528,7 @@ static struct nft_pipapo_elem *pipapo_get(const struct nft_pipapo_match *m,
 #if defined(CONFIG_X86_64) && !defined(CONFIG_UML)
 	if (boot_cpu_has(X86_FEATURE_AVX2) && boot_cpu_has(X86_FEATURE_AVX) &&
 	    irq_fpu_usable()) {
-		kernel_fpu_begin_mask(0);
 		e = pipapo_get_avx2(m, data, genmask, tstamp);
-		kernel_fpu_end();
 		local_bh_enable();
 		return e;
 	}
diff --git a/net/netfilter/nft_set_pipapo_avx2.c b/net/netfilter/nft_set_pipapo_avx2.c
index a584ffff73769..951868a904a25 100644
--- a/net/netfilter/nft_set_pipapo_avx2.c
+++ b/net/netfilter/nft_set_pipapo_avx2.c
@@ -1170,6 +1170,12 @@ struct nft_pipapo_elem *pipapo_get_avx2(const struct nft_pipapo_match *m,
 
 	pipapo_resmap_init_avx2(m, res);
 
+	/* Note that we don't need a valid MXCSR state for any of the
+	 * operations we use here, so pass 0 as mask and spare a LDMXCSR
+	 * instruction.
+	 */
+	kernel_fpu_begin_mask(0);
+
 	nft_pipapo_avx2_prepare();
 
 next_match:
@@ -1221,6 +1227,7 @@ struct nft_pipapo_elem *pipapo_get_avx2(const struct nft_pipapo_match *m,
 
 		if (ret < 0) {
 			scratch->map_index = map_index;
+			kernel_fpu_end();
 			return NULL;
 		}
 
@@ -1233,6 +1240,7 @@ struct nft_pipapo_elem *pipapo_get_avx2(const struct nft_pipapo_match *m,
 				goto next_match;
 
 			scratch->map_index = map_index;
+			kernel_fpu_end();
 			return e;
 		}
 
@@ -1241,6 +1249,7 @@ struct nft_pipapo_elem *pipapo_get_avx2(const struct nft_pipapo_match *m,
 		data += NFT_PIPAPO_GROUPS_PADDED_SIZE(f);
 	}
 
+	kernel_fpu_end();
 	return NULL;
 }
 
@@ -1280,13 +1289,7 @@ nft_pipapo_avx2_lookup(const struct net *net, const struct nft_set *set,
 
 	m = rcu_dereference(priv->match);
 
-	/* Note that we don't need a valid MXCSR state for any of the
-	 * operations we use here, so pass 0 as mask and spare a LDMXCSR
-	 * instruction.
-	 */
-	kernel_fpu_begin_mask(0);
 	e = pipapo_get_avx2(m, rp, genmask, get_jiffies_64());
-	kernel_fpu_end();
 	local_bh_enable();
 
 	return e ? &e->ext : NULL;
-- 
2.50.1

[PATCH nft-testing v3 2/2] netfilter: nft_set_pipapo: Use nested-BH locking for nft_pipapo_scratch

[Sebastian Andrzej Siewior]

nft_pipapo_scratch is a per-CPU variable and relies on disabled BH for
its locking. Without per-CPU locking in local_bh_disable() on PREEMPT_RT
this data structure requires explicit locking.

Add a local_lock_t to the data structure and use local_lock_nested_bh() for
locking. This change adds only lockdep coverage and does not alter the
functional behaviour for !PREEMPT_RT.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/netfilter/nft_set_pipapo.c      | 5 +++++
 net/netfilter/nft_set_pipapo.h      | 1 +
 net/netfilter/nft_set_pipapo_avx2.c | 4 ++++
 3 files changed, 10 insertions(+)

diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
index 1a1ba0b47696e..e7c28b009b9a7 100644
--- a/net/netfilter/nft_set_pipapo.c
+++ b/net/netfilter/nft_set_pipapo.c
@@ -429,6 +429,7 @@ static struct nft_pipapo_elem *pipapo_get_slow(const struct nft_pipapo_match *m,
 	scratch = *raw_cpu_ptr(m->scratch);
 	if (unlikely(!scratch))
 		goto out;
+	__local_lock_nested_bh(&scratch->bh_lock);
 
 	map_index = scratch->map_index;
 
@@ -465,6 +466,7 @@ static struct nft_pipapo_elem *pipapo_get_slow(const struct nft_pipapo_match *m,
 				  last);
 		if (b < 0) {
 			scratch->map_index = map_index;
+			__local_unlock_nested_bh(&scratch->bh_lock);
 			local_bh_enable();
 
 			return NULL;
@@ -484,6 +486,7 @@ static struct nft_pipapo_elem *pipapo_get_slow(const struct nft_pipapo_match *m,
 			 * *next* bitmap (not initial) for the next packet.
 			 */
 			scratch->map_index = map_index;
+			__local_unlock_nested_bh(&scratch->bh_lock);
 			local_bh_enable();
 			return e;
 		}
@@ -498,6 +501,7 @@ static struct nft_pipapo_elem *pipapo_get_slow(const struct nft_pipapo_match *m,
 		data += NFT_PIPAPO_GROUPS_PADDING(f);
 	}
 
+	__local_unlock_nested_bh(&scratch->bh_lock);
 out:
 	local_bh_enable();
 	return NULL;
@@ -1215,6 +1219,7 @@ static int pipapo_realloc_scratch(struct nft_pipapo_match *clone,
 		}
 
 		pipapo_free_scratch(clone, i);
+		local_lock_init(&scratch->bh_lock);
 		*per_cpu_ptr(clone->scratch, i) = scratch;
 	}
 
diff --git a/net/netfilter/nft_set_pipapo.h b/net/netfilter/nft_set_pipapo.h
index 3655aa41fa949..4d9addea854c4 100644
--- a/net/netfilter/nft_set_pipapo.h
+++ b/net/netfilter/nft_set_pipapo.h
@@ -129,6 +129,7 @@ struct nft_pipapo_field {
  * @__map:	store partial matching results during lookup
  */
 struct nft_pipapo_scratch {
+	local_lock_t bh_lock;
 	u8 map_index;
 	unsigned long __map[];
 };
diff --git a/net/netfilter/nft_set_pipapo_avx2.c b/net/netfilter/nft_set_pipapo_avx2.c
index 951868a904a25..8270a95c8ca27 100644
--- a/net/netfilter/nft_set_pipapo_avx2.c
+++ b/net/netfilter/nft_set_pipapo_avx2.c
@@ -1163,6 +1163,7 @@ struct nft_pipapo_elem *pipapo_get_avx2(const struct nft_pipapo_match *m,
 	if (unlikely(!scratch))
 		return NULL;
 
+	__local_lock_nested_bh(&scratch->bh_lock);
 	map_index = scratch->map_index;
 	map = NFT_PIPAPO_LT_ALIGN(&scratch->__map[0]);
 	res  = map + (map_index ? m->bsize_max : 0);
@@ -1228,6 +1229,7 @@ struct nft_pipapo_elem *pipapo_get_avx2(const struct nft_pipapo_match *m,
 		if (ret < 0) {
 			scratch->map_index = map_index;
 			kernel_fpu_end();
+			__local_unlock_nested_bh(&scratch->bh_lock);
 			return NULL;
 		}
 
@@ -1241,6 +1243,7 @@ struct nft_pipapo_elem *pipapo_get_avx2(const struct nft_pipapo_match *m,
 
 			scratch->map_index = map_index;
 			kernel_fpu_end();
+			__local_unlock_nested_bh(&scratch->bh_lock);
 			return e;
 		}
 
@@ -1250,6 +1253,7 @@ struct nft_pipapo_elem *pipapo_get_avx2(const struct nft_pipapo_match *m,
 	}
 
 	kernel_fpu_end();
+	__local_unlock_nested_bh(&scratch->bh_lock);
 	return NULL;
 }
 
-- 
2.50.1

Re: [PATCH nft-testing v3 1/2] netfilter: nft_set_pipapo*: Move FPU handling to pipapo_get_avx2()

[Florian Westphal]

Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> Move kernel_fpu_begin_mask()/ _end() to pipapo_get_avx2() where it is
> required.
> This is a preparation for adding local_lock_t to struct
> nft_pipapo_scratch in order to protect the __map pointer. The lock can
> not be acquired in preemption disabled context which is what
> kernel_fpu_begin*() does.

FYI, I ended up squashing this commit into
'netfilter: nft_set_pipapo: use avx2 algorithm for insertions too'.

This way there is no forth-and-back-again.
I added your SoB tag to the commit and a brief note that this
collapse happened.

Result is in nf-next:testing if you want to take a look.
There are no code changes to the previous incarnation of the branch,
only a few spelling fixes in some comments.