[PATCH] netfilter: nft_ct: Use __refcount_inc() for per-CPU nft_ct_pcpu_template.

[PATCH] netfilter: nft_ct: Use __refcount_inc() for per-CPU nft_ct_pcpu_template.

[Sebastian Andrzej Siewior]

nft_ct_pcpu_template is a per-CPU variable and relies on disabled BH for its
locking. The refcounter is read and if its value is set to one then the
refcounter is incremented and variable is used - otherwise it is already
in use and left untouched.

Without per-CPU locking in local_bh_disable() on PREEMPT_RT the
read-then-increment operation is not atomic and therefore racy.

This can be avoided by using unconditionally __refcount_inc() which will
increment counter and return the old value as an atomic operation.
In case the returned counter is not one, the variable is in use and we
need to decrement counter. Otherwise we can use it.

Use __refcount_inc() instead of read and a conditional increment.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/netfilter/nft_ct.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 2e59aba681a13..d526e69a2a2b8 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -230,6 +230,7 @@ static void nft_ct_set_zone_eval(const struct nft_expr *expr,
 	enum ip_conntrack_info ctinfo;
 	u16 value = nft_reg_load16(&regs->data[priv->sreg]);
 	struct nf_conn *ct;
+	int oldcnt;
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if (ct) /* already tracked */
@@ -250,10 +251,11 @@ static void nft_ct_set_zone_eval(const struct nft_expr *expr,
 
 	ct = this_cpu_read(nft_ct_pcpu_template);
 
-	if (likely(refcount_read(&ct->ct_general.use) == 1)) {
-		refcount_inc(&ct->ct_general.use);
+	__refcount_inc(&ct->ct_general.use, &oldcnt);
+	if (likely(oldcnt == 1)) {
 		nf_ct_zone_add(ct, &zone);
 	} else {
+		refcount_dec(&ct->ct_general.use);
 		/* previous skb got queued to userspace, allocate temporary
 		 * one until percpu template can be reused.
 		 */
-- 
2.47.2

Re: [PATCH] netfilter: nft_ct: Use __refcount_inc() for per-CPU nft_ct_pcpu_template.

[Florian Westphal]

Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> nft_ct_pcpu_template is a per-CPU variable and relies on disabled BH for its
> locking. The refcounter is read and if its value is set to one then the
> refcounter is incremented and variable is used - otherwise it is already
> in use and left untouched.
> 
> Without per-CPU locking in local_bh_disable() on PREEMPT_RT the
> read-then-increment operation is not atomic and therefore racy.
> 
> This can be avoided by using unconditionally __refcount_inc() which will
> increment counter and return the old value as an atomic operation.
> In case the returned counter is not one, the variable is in use and we
> need to decrement counter. Otherwise we can use it.
> 
> Use __refcount_inc() instead of read and a conditional increment.

Reviewed-by: Florian Westphal <fw@strlen.de>
Fixes: edee4f1e9245 ("netfilter: nft_ct: add zone id set support")

Re: [PATCH] netfilter: nft_ct: Use __refcount_inc() for per-CPU nft_ct_pcpu_template.

[Pablo Neira Ayuso]

On Mon, Feb 17, 2025 at 05:02:42PM +0100, Sebastian Andrzej Siewior wrote:
> nft_ct_pcpu_template is a per-CPU variable and relies on disabled BH for its
> locking. The refcounter is read and if its value is set to one then the
> refcounter is incremented and variable is used - otherwise it is already
> in use and left untouched.
> 
> Without per-CPU locking in local_bh_disable() on PREEMPT_RT the
> read-then-increment operation is not atomic and therefore racy.
> 
> This can be avoided by using unconditionally __refcount_inc() which will
> increment counter and return the old value as an atomic operation.
> In case the returned counter is not one, the variable is in use and we
> need to decrement counter. Otherwise we can use it.
> 
> Use __refcount_inc() instead of read and a conditional increment.

Applied nf.git, thanks and sorry for taking a while to collect this.