* [PATCH net-next] net_sched: add back BH safety to tcf_lock
@ 2025-09-01 9:26 Eric Dumazet
2025-09-02 11:56 ` Jamal Hadi Salim
2025-09-02 23:00 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 5+ messages in thread
From: Eric Dumazet @ 2025-09-01 9:26 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski, Paolo Abeni
Cc: Simon Horman, Jamal Hadi Salim, Cong Wang, Jiri Pirko, netdev,
eric.dumazet, Eric Dumazet
Jamal reported that we had to use BH safety after all,
because stats can be updated from BH handler.
Fixes: 3133d5c15cb5 ("net_sched: remove BH blocking in eight actions")
Fixes: 53df77e78590 ("net_sched: act_skbmod: use RCU in tcf_skbmod_dump()")
Fixes: e97ae742972f ("net_sched: act_tunnel_key: use RCU in tunnel_key_dump()")
Fixes: 48b5e5dbdb23 ("net_sched: act_vlan: use RCU in tcf_vlan_dump()")
Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
Closes: https://lore.kernel.org/netdev/CAM0EoMmhq66EtVqDEuNik8MVFZqkgxFbMu=fJtbNoYD7YXg4bA@mail.gmail.com/
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
net/sched/act_connmark.c | 4 ++--
net/sched/act_csum.c | 4 ++--
net/sched/act_ct.c | 4 ++--
net/sched/act_ctinfo.c | 4 ++--
net/sched/act_mpls.c | 4 ++--
net/sched/act_nat.c | 4 ++--
net/sched/act_pedit.c | 4 ++--
net/sched/act_skbedit.c | 4 ++--
net/sched/act_skbmod.c | 4 ++--
net/sched/act_tunnel_key.c | 4 ++--
net/sched/act_vlan.c | 4 ++--
11 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
index bf2d6b6da042..3e89927d7116 100644
--- a/net/sched/act_connmark.c
+++ b/net/sched/act_connmark.c
@@ -169,10 +169,10 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
nparms->action = parm->action;
- spin_lock(&ci->tcf_lock);
+ spin_lock_bh(&ci->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
oparms = rcu_replace_pointer(ci->parms, nparms, lockdep_is_held(&ci->tcf_lock));
- spin_unlock(&ci->tcf_lock);
+ spin_unlock_bh(&ci->tcf_lock);
if (goto_ch)
tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
index 8bad91753615..0939e6b2ba4d 100644
--- a/net/sched/act_csum.c
+++ b/net/sched/act_csum.c
@@ -101,11 +101,11 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
params_new->update_flags = parm->update_flags;
params_new->action = parm->action;
- spin_lock(&p->tcf_lock);
+ spin_lock_bh(&p->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
params_new = rcu_replace_pointer(p->params, params_new,
lockdep_is_held(&p->tcf_lock));
- spin_unlock(&p->tcf_lock);
+ spin_unlock_bh(&p->tcf_lock);
if (goto_ch)
tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index 6d2355e73b0f..6749a4a9a9cd 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -1410,11 +1410,11 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
goto cleanup;
params->action = parm->action;
- spin_lock(&c->tcf_lock);
+ spin_lock_bh(&c->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
params = rcu_replace_pointer(c->params, params,
lockdep_is_held(&c->tcf_lock));
- spin_unlock(&c->tcf_lock);
+ spin_unlock_bh(&c->tcf_lock);
if (goto_ch)
tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index 6f79eed9a544..71efe04d00b5 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -258,11 +258,11 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
cp_new->action = actparm->action;
- spin_lock(&ci->tcf_lock);
+ spin_lock_bh(&ci->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, actparm->action, goto_ch);
cp_new = rcu_replace_pointer(ci->params, cp_new,
lockdep_is_held(&ci->tcf_lock));
- spin_unlock(&ci->tcf_lock);
+ spin_unlock_bh(&ci->tcf_lock);
if (goto_ch)
tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index ed7bdaa23f0d..6654011dcd2b 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -296,10 +296,10 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
htons(ETH_P_MPLS_UC));
p->action = parm->action;
- spin_lock(&m->tcf_lock);
+ spin_lock_bh(&m->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
p = rcu_replace_pointer(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
- spin_unlock(&m->tcf_lock);
+ spin_unlock_bh(&m->tcf_lock);
if (goto_ch)
tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
index 9cc2a1772cf8..26241d80ebe0 100644
--- a/net/sched/act_nat.c
+++ b/net/sched/act_nat.c
@@ -95,10 +95,10 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
p = to_tcf_nat(*a);
- spin_lock(&p->tcf_lock);
+ spin_lock_bh(&p->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
oparm = rcu_replace_pointer(p->parms, nparm, lockdep_is_held(&p->tcf_lock));
- spin_unlock(&p->tcf_lock);
+ spin_unlock_bh(&p->tcf_lock);
if (goto_ch)
tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index 8fc8f577cb7a..4b65901397a8 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -280,10 +280,10 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
p = to_pedit(*a);
nparms->action = parm->action;
- spin_lock(&p->tcf_lock);
+ spin_lock_bh(&p->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
oparms = rcu_replace_pointer(p->parms, nparms, 1);
- spin_unlock(&p->tcf_lock);
+ spin_unlock_bh(&p->tcf_lock);
if (oparms)
call_rcu(&oparms->rcu, tcf_pedit_cleanup_rcu);
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index aa6b1744de21..8c1d1554f657 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -261,11 +261,11 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
params_new->mask = *mask;
params_new->action = parm->action;
- spin_lock(&d->tcf_lock);
+ spin_lock_bh(&d->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
params_new = rcu_replace_pointer(d->params, params_new,
lockdep_is_held(&d->tcf_lock));
- spin_unlock(&d->tcf_lock);
+ spin_unlock_bh(&d->tcf_lock);
if (params_new)
kfree_rcu(params_new, rcu);
if (goto_ch)
diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
index fce625eafcb2..a9e0c1326e2a 100644
--- a/net/sched/act_skbmod.c
+++ b/net/sched/act_skbmod.c
@@ -194,7 +194,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
p->flags = lflags;
p->action = parm->action;
if (ovr)
- spin_lock(&d->tcf_lock);
+ spin_lock_bh(&d->tcf_lock);
/* Protected by tcf_lock if overwriting existing action. */
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
p_old = rcu_dereference_protected(d->skbmod_p, 1);
@@ -208,7 +208,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
rcu_assign_pointer(d->skbmod_p, p);
if (ovr)
- spin_unlock(&d->tcf_lock);
+ spin_unlock_bh(&d->tcf_lock);
if (p_old)
kfree_rcu(p_old, rcu);
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index e1c8b48c217c..876b30c5709e 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -531,11 +531,11 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
params_new->tcft_enc_metadata = metadata;
params_new->action = parm->action;
- spin_lock(&t->tcf_lock);
+ spin_lock_bh(&t->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
params_new = rcu_replace_pointer(t->params, params_new,
lockdep_is_held(&t->tcf_lock));
- spin_unlock(&t->tcf_lock);
+ spin_unlock_bh(&t->tcf_lock);
tunnel_key_release_params(params_new);
if (goto_ch)
tcf_chain_put_by_act(goto_ch);
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index b46f980f3b2a..a74621797d69 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -253,10 +253,10 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
}
p->action = parm->action;
- spin_lock(&v->tcf_lock);
+ spin_lock_bh(&v->tcf_lock);
goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
p = rcu_replace_pointer(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
- spin_unlock(&v->tcf_lock);
+ spin_unlock_bh(&v->tcf_lock);
if (goto_ch)
tcf_chain_put_by_act(goto_ch);
--
2.51.0.318.gd7df087d1a-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net_sched: add back BH safety to tcf_lock
2025-09-01 9:26 [PATCH net-next] net_sched: add back BH safety to tcf_lock Eric Dumazet
@ 2025-09-02 11:56 ` Jamal Hadi Salim
2025-09-02 12:12 ` Eric Dumazet
2025-09-02 12:13 ` Jamal Hadi Salim
2025-09-02 23:00 ` patchwork-bot+netdevbpf
1 sibling, 2 replies; 5+ messages in thread
From: Jamal Hadi Salim @ 2025-09-02 11:56 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
Cong Wang, Jiri Pirko, netdev, eric.dumazet
On Mon, Sep 1, 2025 at 5:26 AM Eric Dumazet <edumazet@google.com> wrote:
>
> Jamal reported that we had to use BH safety after all,
> because stats can be updated from BH handler.
>
> Fixes: 3133d5c15cb5 ("net_sched: remove BH blocking in eight actions")
> Fixes: 53df77e78590 ("net_sched: act_skbmod: use RCU in tcf_skbmod_dump()")
> Fixes: e97ae742972f ("net_sched: act_tunnel_key: use RCU in tunnel_key_dump()")
> Fixes: 48b5e5dbdb23 ("net_sched: act_vlan: use RCU in tcf_vlan_dump()")
> Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Closes: https://lore.kernel.org/netdev/CAM0EoMmhq66EtVqDEuNik8MVFZqkgxFbMu=fJtbNoYD7YXg4bA@mail.gmail.com/
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Eric - i dont think this will fix that lockdep splat i was referring
to though, no?
cheers,
jamal
> ---
> net/sched/act_connmark.c | 4 ++--
> net/sched/act_csum.c | 4 ++--
> net/sched/act_ct.c | 4 ++--
> net/sched/act_ctinfo.c | 4 ++--
> net/sched/act_mpls.c | 4 ++--
> net/sched/act_nat.c | 4 ++--
> net/sched/act_pedit.c | 4 ++--
> net/sched/act_skbedit.c | 4 ++--
> net/sched/act_skbmod.c | 4 ++--
> net/sched/act_tunnel_key.c | 4 ++--
> net/sched/act_vlan.c | 4 ++--
> 11 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
> index bf2d6b6da042..3e89927d7116 100644
> --- a/net/sched/act_connmark.c
> +++ b/net/sched/act_connmark.c
> @@ -169,10 +169,10 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
>
> nparms->action = parm->action;
>
> - spin_lock(&ci->tcf_lock);
> + spin_lock_bh(&ci->tcf_lock);
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> oparms = rcu_replace_pointer(ci->parms, nparms, lockdep_is_held(&ci->tcf_lock));
> - spin_unlock(&ci->tcf_lock);
> + spin_unlock_bh(&ci->tcf_lock);
>
> if (goto_ch)
> tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
> index 8bad91753615..0939e6b2ba4d 100644
> --- a/net/sched/act_csum.c
> +++ b/net/sched/act_csum.c
> @@ -101,11 +101,11 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
> params_new->update_flags = parm->update_flags;
> params_new->action = parm->action;
>
> - spin_lock(&p->tcf_lock);
> + spin_lock_bh(&p->tcf_lock);
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> params_new = rcu_replace_pointer(p->params, params_new,
> lockdep_is_held(&p->tcf_lock));
> - spin_unlock(&p->tcf_lock);
> + spin_unlock_bh(&p->tcf_lock);
>
> if (goto_ch)
> tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 6d2355e73b0f..6749a4a9a9cd 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -1410,11 +1410,11 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
> goto cleanup;
>
> params->action = parm->action;
> - spin_lock(&c->tcf_lock);
> + spin_lock_bh(&c->tcf_lock);
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> params = rcu_replace_pointer(c->params, params,
> lockdep_is_held(&c->tcf_lock));
> - spin_unlock(&c->tcf_lock);
> + spin_unlock_bh(&c->tcf_lock);
>
> if (goto_ch)
> tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
> index 6f79eed9a544..71efe04d00b5 100644
> --- a/net/sched/act_ctinfo.c
> +++ b/net/sched/act_ctinfo.c
> @@ -258,11 +258,11 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
>
> cp_new->action = actparm->action;
>
> - spin_lock(&ci->tcf_lock);
> + spin_lock_bh(&ci->tcf_lock);
> goto_ch = tcf_action_set_ctrlact(*a, actparm->action, goto_ch);
> cp_new = rcu_replace_pointer(ci->params, cp_new,
> lockdep_is_held(&ci->tcf_lock));
> - spin_unlock(&ci->tcf_lock);
> + spin_unlock_bh(&ci->tcf_lock);
>
> if (goto_ch)
> tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
> index ed7bdaa23f0d..6654011dcd2b 100644
> --- a/net/sched/act_mpls.c
> +++ b/net/sched/act_mpls.c
> @@ -296,10 +296,10 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
> htons(ETH_P_MPLS_UC));
> p->action = parm->action;
>
> - spin_lock(&m->tcf_lock);
> + spin_lock_bh(&m->tcf_lock);
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> p = rcu_replace_pointer(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
> - spin_unlock(&m->tcf_lock);
> + spin_unlock_bh(&m->tcf_lock);
>
> if (goto_ch)
> tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> index 9cc2a1772cf8..26241d80ebe0 100644
> --- a/net/sched/act_nat.c
> +++ b/net/sched/act_nat.c
> @@ -95,10 +95,10 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
>
> p = to_tcf_nat(*a);
>
> - spin_lock(&p->tcf_lock);
> + spin_lock_bh(&p->tcf_lock);
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> oparm = rcu_replace_pointer(p->parms, nparm, lockdep_is_held(&p->tcf_lock));
> - spin_unlock(&p->tcf_lock);
> + spin_unlock_bh(&p->tcf_lock);
>
> if (goto_ch)
> tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> index 8fc8f577cb7a..4b65901397a8 100644
> --- a/net/sched/act_pedit.c
> +++ b/net/sched/act_pedit.c
> @@ -280,10 +280,10 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
>
> p = to_pedit(*a);
> nparms->action = parm->action;
> - spin_lock(&p->tcf_lock);
> + spin_lock_bh(&p->tcf_lock);
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> oparms = rcu_replace_pointer(p->parms, nparms, 1);
> - spin_unlock(&p->tcf_lock);
> + spin_unlock_bh(&p->tcf_lock);
>
> if (oparms)
> call_rcu(&oparms->rcu, tcf_pedit_cleanup_rcu);
> diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> index aa6b1744de21..8c1d1554f657 100644
> --- a/net/sched/act_skbedit.c
> +++ b/net/sched/act_skbedit.c
> @@ -261,11 +261,11 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> params_new->mask = *mask;
>
> params_new->action = parm->action;
> - spin_lock(&d->tcf_lock);
> + spin_lock_bh(&d->tcf_lock);
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> params_new = rcu_replace_pointer(d->params, params_new,
> lockdep_is_held(&d->tcf_lock));
> - spin_unlock(&d->tcf_lock);
> + spin_unlock_bh(&d->tcf_lock);
> if (params_new)
> kfree_rcu(params_new, rcu);
> if (goto_ch)
> diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
> index fce625eafcb2..a9e0c1326e2a 100644
> --- a/net/sched/act_skbmod.c
> +++ b/net/sched/act_skbmod.c
> @@ -194,7 +194,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
> p->flags = lflags;
> p->action = parm->action;
> if (ovr)
> - spin_lock(&d->tcf_lock);
> + spin_lock_bh(&d->tcf_lock);
> /* Protected by tcf_lock if overwriting existing action. */
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> p_old = rcu_dereference_protected(d->skbmod_p, 1);
> @@ -208,7 +208,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
>
> rcu_assign_pointer(d->skbmod_p, p);
> if (ovr)
> - spin_unlock(&d->tcf_lock);
> + spin_unlock_bh(&d->tcf_lock);
>
> if (p_old)
> kfree_rcu(p_old, rcu);
> diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
> index e1c8b48c217c..876b30c5709e 100644
> --- a/net/sched/act_tunnel_key.c
> +++ b/net/sched/act_tunnel_key.c
> @@ -531,11 +531,11 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
> params_new->tcft_enc_metadata = metadata;
>
> params_new->action = parm->action;
> - spin_lock(&t->tcf_lock);
> + spin_lock_bh(&t->tcf_lock);
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> params_new = rcu_replace_pointer(t->params, params_new,
> lockdep_is_held(&t->tcf_lock));
> - spin_unlock(&t->tcf_lock);
> + spin_unlock_bh(&t->tcf_lock);
> tunnel_key_release_params(params_new);
> if (goto_ch)
> tcf_chain_put_by_act(goto_ch);
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index b46f980f3b2a..a74621797d69 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -253,10 +253,10 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
> }
>
> p->action = parm->action;
> - spin_lock(&v->tcf_lock);
> + spin_lock_bh(&v->tcf_lock);
> goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> p = rcu_replace_pointer(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
> - spin_unlock(&v->tcf_lock);
> + spin_unlock_bh(&v->tcf_lock);
>
> if (goto_ch)
> tcf_chain_put_by_act(goto_ch);
> --
> 2.51.0.318.gd7df087d1a-goog
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net_sched: add back BH safety to tcf_lock
2025-09-02 11:56 ` Jamal Hadi Salim
@ 2025-09-02 12:12 ` Eric Dumazet
2025-09-02 12:13 ` Jamal Hadi Salim
1 sibling, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2025-09-02 12:12 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
Cong Wang, Jiri Pirko, netdev, eric.dumazet
On Tue, Sep 2, 2025 at 4:56 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Mon, Sep 1, 2025 at 5:26 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > Jamal reported that we had to use BH safety after all,
> > because stats can be updated from BH handler.
> >
> > Fixes: 3133d5c15cb5 ("net_sched: remove BH blocking in eight actions")
> > Fixes: 53df77e78590 ("net_sched: act_skbmod: use RCU in tcf_skbmod_dump()")
> > Fixes: e97ae742972f ("net_sched: act_tunnel_key: use RCU in tunnel_key_dump()")
> > Fixes: 48b5e5dbdb23 ("net_sched: act_vlan: use RCU in tcf_vlan_dump()")
> > Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > Closes: https://lore.kernel.org/netdev/CAM0EoMmhq66EtVqDEuNik8MVFZqkgxFbMu=fJtbNoYD7YXg4bA@mail.gmail.com/
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Eric - i dont think this will fix that lockdep splat i was referring
> to though, no?
It should since we go back to the situation before
3133d5c15cb5 ("net_sched: remove BH blocking in eight actions")
as far as locking is concerned ?
Do you have a lockdep splat to share ?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net_sched: add back BH safety to tcf_lock
2025-09-02 11:56 ` Jamal Hadi Salim
2025-09-02 12:12 ` Eric Dumazet
@ 2025-09-02 12:13 ` Jamal Hadi Salim
1 sibling, 0 replies; 5+ messages in thread
From: Jamal Hadi Salim @ 2025-09-02 12:13 UTC (permalink / raw)
To: Eric Dumazet
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Simon Horman,
Cong Wang, Jiri Pirko, netdev, eric.dumazet
On Tue, Sep 2, 2025 at 7:56 AM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Mon, Sep 1, 2025 at 5:26 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > Jamal reported that we had to use BH safety after all,
> > because stats can be updated from BH handler.
> >
> > Fixes: 3133d5c15cb5 ("net_sched: remove BH blocking in eight actions")
> > Fixes: 53df77e78590 ("net_sched: act_skbmod: use RCU in tcf_skbmod_dump()")
> > Fixes: e97ae742972f ("net_sched: act_tunnel_key: use RCU in tunnel_key_dump()")
> > Fixes: 48b5e5dbdb23 ("net_sched: act_vlan: use RCU in tcf_vlan_dump()")
> > Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > Closes: https://lore.kernel.org/netdev/CAM0EoMmhq66EtVqDEuNik8MVFZqkgxFbMu=fJtbNoYD7YXg4bA@mail.gmail.com/
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Eric - i dont think this will fix that lockdep splat i was referring
> to though, no?
>
Quick test reveals it was fixed ;->
cheers,
jamal
> cheers,
> jamal
> > ---
> > net/sched/act_connmark.c | 4 ++--
> > net/sched/act_csum.c | 4 ++--
> > net/sched/act_ct.c | 4 ++--
> > net/sched/act_ctinfo.c | 4 ++--
> > net/sched/act_mpls.c | 4 ++--
> > net/sched/act_nat.c | 4 ++--
> > net/sched/act_pedit.c | 4 ++--
> > net/sched/act_skbedit.c | 4 ++--
> > net/sched/act_skbmod.c | 4 ++--
> > net/sched/act_tunnel_key.c | 4 ++--
> > net/sched/act_vlan.c | 4 ++--
> > 11 files changed, 22 insertions(+), 22 deletions(-)
> >
> > diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
> > index bf2d6b6da042..3e89927d7116 100644
> > --- a/net/sched/act_connmark.c
> > +++ b/net/sched/act_connmark.c
> > @@ -169,10 +169,10 @@ static int tcf_connmark_init(struct net *net, struct nlattr *nla,
> >
> > nparms->action = parm->action;
> >
> > - spin_lock(&ci->tcf_lock);
> > + spin_lock_bh(&ci->tcf_lock);
> > goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > oparms = rcu_replace_pointer(ci->parms, nparms, lockdep_is_held(&ci->tcf_lock));
> > - spin_unlock(&ci->tcf_lock);
> > + spin_unlock_bh(&ci->tcf_lock);
> >
> > if (goto_ch)
> > tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_csum.c b/net/sched/act_csum.c
> > index 8bad91753615..0939e6b2ba4d 100644
> > --- a/net/sched/act_csum.c
> > +++ b/net/sched/act_csum.c
> > @@ -101,11 +101,11 @@ static int tcf_csum_init(struct net *net, struct nlattr *nla,
> > params_new->update_flags = parm->update_flags;
> > params_new->action = parm->action;
> >
> > - spin_lock(&p->tcf_lock);
> > + spin_lock_bh(&p->tcf_lock);
> > goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > params_new = rcu_replace_pointer(p->params, params_new,
> > lockdep_is_held(&p->tcf_lock));
> > - spin_unlock(&p->tcf_lock);
> > + spin_unlock_bh(&p->tcf_lock);
> >
> > if (goto_ch)
> > tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> > index 6d2355e73b0f..6749a4a9a9cd 100644
> > --- a/net/sched/act_ct.c
> > +++ b/net/sched/act_ct.c
> > @@ -1410,11 +1410,11 @@ static int tcf_ct_init(struct net *net, struct nlattr *nla,
> > goto cleanup;
> >
> > params->action = parm->action;
> > - spin_lock(&c->tcf_lock);
> > + spin_lock_bh(&c->tcf_lock);
> > goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > params = rcu_replace_pointer(c->params, params,
> > lockdep_is_held(&c->tcf_lock));
> > - spin_unlock(&c->tcf_lock);
> > + spin_unlock_bh(&c->tcf_lock);
> >
> > if (goto_ch)
> > tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
> > index 6f79eed9a544..71efe04d00b5 100644
> > --- a/net/sched/act_ctinfo.c
> > +++ b/net/sched/act_ctinfo.c
> > @@ -258,11 +258,11 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
> >
> > cp_new->action = actparm->action;
> >
> > - spin_lock(&ci->tcf_lock);
> > + spin_lock_bh(&ci->tcf_lock);
> > goto_ch = tcf_action_set_ctrlact(*a, actparm->action, goto_ch);
> > cp_new = rcu_replace_pointer(ci->params, cp_new,
> > lockdep_is_held(&ci->tcf_lock));
> > - spin_unlock(&ci->tcf_lock);
> > + spin_unlock_bh(&ci->tcf_lock);
> >
> > if (goto_ch)
> > tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
> > index ed7bdaa23f0d..6654011dcd2b 100644
> > --- a/net/sched/act_mpls.c
> > +++ b/net/sched/act_mpls.c
> > @@ -296,10 +296,10 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
> > htons(ETH_P_MPLS_UC));
> > p->action = parm->action;
> >
> > - spin_lock(&m->tcf_lock);
> > + spin_lock_bh(&m->tcf_lock);
> > goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > p = rcu_replace_pointer(m->mpls_p, p, lockdep_is_held(&m->tcf_lock));
> > - spin_unlock(&m->tcf_lock);
> > + spin_unlock_bh(&m->tcf_lock);
> >
> > if (goto_ch)
> > tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> > index 9cc2a1772cf8..26241d80ebe0 100644
> > --- a/net/sched/act_nat.c
> > +++ b/net/sched/act_nat.c
> > @@ -95,10 +95,10 @@ static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
> >
> > p = to_tcf_nat(*a);
> >
> > - spin_lock(&p->tcf_lock);
> > + spin_lock_bh(&p->tcf_lock);
> > goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > oparm = rcu_replace_pointer(p->parms, nparm, lockdep_is_held(&p->tcf_lock));
> > - spin_unlock(&p->tcf_lock);
> > + spin_unlock_bh(&p->tcf_lock);
> >
> > if (goto_ch)
> > tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
> > index 8fc8f577cb7a..4b65901397a8 100644
> > --- a/net/sched/act_pedit.c
> > +++ b/net/sched/act_pedit.c
> > @@ -280,10 +280,10 @@ static int tcf_pedit_init(struct net *net, struct nlattr *nla,
> >
> > p = to_pedit(*a);
> > nparms->action = parm->action;
> > - spin_lock(&p->tcf_lock);
> > + spin_lock_bh(&p->tcf_lock);
> > goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > oparms = rcu_replace_pointer(p->parms, nparms, 1);
> > - spin_unlock(&p->tcf_lock);
> > + spin_unlock_bh(&p->tcf_lock);
> >
> > if (oparms)
> > call_rcu(&oparms->rcu, tcf_pedit_cleanup_rcu);
> > diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
> > index aa6b1744de21..8c1d1554f657 100644
> > --- a/net/sched/act_skbedit.c
> > +++ b/net/sched/act_skbedit.c
> > @@ -261,11 +261,11 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
> > params_new->mask = *mask;
> >
> > params_new->action = parm->action;
> > - spin_lock(&d->tcf_lock);
> > + spin_lock_bh(&d->tcf_lock);
> > goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > params_new = rcu_replace_pointer(d->params, params_new,
> > lockdep_is_held(&d->tcf_lock));
> > - spin_unlock(&d->tcf_lock);
> > + spin_unlock_bh(&d->tcf_lock);
> > if (params_new)
> > kfree_rcu(params_new, rcu);
> > if (goto_ch)
> > diff --git a/net/sched/act_skbmod.c b/net/sched/act_skbmod.c
> > index fce625eafcb2..a9e0c1326e2a 100644
> > --- a/net/sched/act_skbmod.c
> > +++ b/net/sched/act_skbmod.c
> > @@ -194,7 +194,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
> > p->flags = lflags;
> > p->action = parm->action;
> > if (ovr)
> > - spin_lock(&d->tcf_lock);
> > + spin_lock_bh(&d->tcf_lock);
> > /* Protected by tcf_lock if overwriting existing action. */
> > goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > p_old = rcu_dereference_protected(d->skbmod_p, 1);
> > @@ -208,7 +208,7 @@ static int tcf_skbmod_init(struct net *net, struct nlattr *nla,
> >
> > rcu_assign_pointer(d->skbmod_p, p);
> > if (ovr)
> > - spin_unlock(&d->tcf_lock);
> > + spin_unlock_bh(&d->tcf_lock);
> >
> > if (p_old)
> > kfree_rcu(p_old, rcu);
> > diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
> > index e1c8b48c217c..876b30c5709e 100644
> > --- a/net/sched/act_tunnel_key.c
> > +++ b/net/sched/act_tunnel_key.c
> > @@ -531,11 +531,11 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
> > params_new->tcft_enc_metadata = metadata;
> >
> > params_new->action = parm->action;
> > - spin_lock(&t->tcf_lock);
> > + spin_lock_bh(&t->tcf_lock);
> > goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > params_new = rcu_replace_pointer(t->params, params_new,
> > lockdep_is_held(&t->tcf_lock));
> > - spin_unlock(&t->tcf_lock);
> > + spin_unlock_bh(&t->tcf_lock);
> > tunnel_key_release_params(params_new);
> > if (goto_ch)
> > tcf_chain_put_by_act(goto_ch);
> > diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> > index b46f980f3b2a..a74621797d69 100644
> > --- a/net/sched/act_vlan.c
> > +++ b/net/sched/act_vlan.c
> > @@ -253,10 +253,10 @@ static int tcf_vlan_init(struct net *net, struct nlattr *nla,
> > }
> >
> > p->action = parm->action;
> > - spin_lock(&v->tcf_lock);
> > + spin_lock_bh(&v->tcf_lock);
> > goto_ch = tcf_action_set_ctrlact(*a, parm->action, goto_ch);
> > p = rcu_replace_pointer(v->vlan_p, p, lockdep_is_held(&v->tcf_lock));
> > - spin_unlock(&v->tcf_lock);
> > + spin_unlock_bh(&v->tcf_lock);
> >
> > if (goto_ch)
> > tcf_chain_put_by_act(goto_ch);
> > --
> > 2.51.0.318.gd7df087d1a-goog
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net_sched: add back BH safety to tcf_lock
2025-09-01 9:26 [PATCH net-next] net_sched: add back BH safety to tcf_lock Eric Dumazet
2025-09-02 11:56 ` Jamal Hadi Salim
@ 2025-09-02 23:00 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2025-09-02 23:00 UTC (permalink / raw)
To: Eric Dumazet
Cc: davem, kuba, pabeni, horms, jhs, xiyou.wangcong, jiri, netdev,
eric.dumazet
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Mon, 1 Sep 2025 09:26:07 +0000 you wrote:
> Jamal reported that we had to use BH safety after all,
> because stats can be updated from BH handler.
>
> Fixes: 3133d5c15cb5 ("net_sched: remove BH blocking in eight actions")
> Fixes: 53df77e78590 ("net_sched: act_skbmod: use RCU in tcf_skbmod_dump()")
> Fixes: e97ae742972f ("net_sched: act_tunnel_key: use RCU in tunnel_key_dump()")
> Fixes: 48b5e5dbdb23 ("net_sched: act_vlan: use RCU in tcf_vlan_dump()")
> Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Closes: https://lore.kernel.org/netdev/CAM0EoMmhq66EtVqDEuNik8MVFZqkgxFbMu=fJtbNoYD7YXg4bA@mail.gmail.com/
> Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> [...]
Here is the summary with links:
- [net-next] net_sched: add back BH safety to tcf_lock
https://git.kernel.org/netdev/net-next/c/3016024d7514
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-09-02 23:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-01 9:26 [PATCH net-next] net_sched: add back BH safety to tcf_lock Eric Dumazet
2025-09-02 11:56 ` Jamal Hadi Salim
2025-09-02 12:12 ` Eric Dumazet
2025-09-02 12:13 ` Jamal Hadi Salim
2025-09-02 23:00 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).