* [PATCH net v5 0/1] net/sched: act_gate: snapshot parameters with RCU on replace
@ 2026-02-05 15:10 Paul Moses
2026-02-05 15:10 ` [PATCH net v5 1/1] " Paul Moses
0 siblings, 1 reply; 5+ messages in thread
From: Paul Moses @ 2026-02-05 15:10 UTC (permalink / raw)
To: Victor Nogueira, Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, netdev, linux-kernel, Paul Moses
This series fixes a schedule lifetime race in `act_gate` between the
control-plane replace path and the running hrtimer callback (gate_timer_func).
Changes since v4:
- Drop the gate_clockid_to_offset helper, validate clockid once, and set
tk_offset in one place.
- Seed missing attrs from the old params when entries are replaced
- Reimplemented the timer cancel from RCU snapshot and cancel before taking tcf_lock.
- Reuse a single protected deref under tcf_lock
- Compare old/new values directly in gate_timer_needs_cancel()
- Cleanup no longer assumes rtnl_lock
Paul Moses (1):
net/sched: act_gate: snapshot parameters with RCU on replace
include/net/tc_act/tc_gate.h | 33 ++++-
net/sched/act_gate.c | 266 ++++++++++++++++++++++++++---------
2 files changed, 228 insertions(+), 71 deletions(-)
--
2.52.GIT
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net v5 1/1] net/sched: act_gate: snapshot parameters with RCU on replace
2026-02-05 15:10 [PATCH net v5 0/1] net/sched: act_gate: snapshot parameters with RCU on replace Paul Moses
@ 2026-02-05 15:10 ` Paul Moses
2026-02-06 10:36 ` Victor Nogueira
0 siblings, 1 reply; 5+ messages in thread
From: Paul Moses @ 2026-02-05 15:10 UTC (permalink / raw)
To: Victor Nogueira, Jamal Hadi Salim, Cong Wang, Jiri Pirko
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, netdev, linux-kernel, Paul Moses, stable
The gate action can be replaced while the hrtimer callback or dump path is
walking the schedule list.
Convert the parameters to an RCU-protected snapshot and swap updates under
tcf_lock, freeing the previous snapshot via call_rcu(). When REPLACE omits
the entry list, preserve the existing schedule so the effective state is
unchanged.
Fixes: a51c328df310 ("net: qos: introduce a gate control flow action")
Cc: stable@vger.kernel.org
Signed-off-by: Paul Moses <p@1g4.org>
---
include/net/tc_act/tc_gate.h | 33 ++++-
net/sched/act_gate.c | 266 ++++++++++++++++++++++++++---------
2 files changed, 228 insertions(+), 71 deletions(-)
diff --git a/include/net/tc_act/tc_gate.h b/include/net/tc_act/tc_gate.h
index c1a67149c6b62..006111b978eda 100644
--- a/include/net/tc_act/tc_gate.h
+++ b/include/net/tc_act/tc_gate.h
@@ -32,6 +32,7 @@ struct tcf_gate_params {
s32 tcfg_clockid;
size_t num_entries;
struct list_head entries;
+ struct rcu_head rcu;
};
#define GATE_ACT_GATE_OPEN BIT(0)
@@ -39,7 +40,7 @@ struct tcf_gate_params {
struct tcf_gate {
struct tc_action common;
- struct tcf_gate_params param;
+ struct tcf_gate_params __rcu *param;
u8 current_gate_status;
ktime_t current_close_time;
u32 current_entry_octets;
@@ -51,47 +52,65 @@ struct tcf_gate {
#define to_gate(a) ((struct tcf_gate *)a)
+static inline struct tcf_gate_params *tcf_gate_params(const struct tc_action *a)
+{
+ struct tcf_gate *gact = to_gate(a);
+
+ return rcu_dereference_protected(gact->param,
+ lockdep_is_held(&gact->tcf_lock));
+}
+
static inline s32 tcf_gate_prio(const struct tc_action *a)
{
+ struct tcf_gate_params *p;
s32 tcfg_prio;
- tcfg_prio = to_gate(a)->param.tcfg_priority;
+ p = tcf_gate_params(a);
+ tcfg_prio = p->tcfg_priority;
return tcfg_prio;
}
static inline u64 tcf_gate_basetime(const struct tc_action *a)
{
+ struct tcf_gate_params *p;
u64 tcfg_basetime;
- tcfg_basetime = to_gate(a)->param.tcfg_basetime;
+ p = tcf_gate_params(a);
+ tcfg_basetime = p->tcfg_basetime;
return tcfg_basetime;
}
static inline u64 tcf_gate_cycletime(const struct tc_action *a)
{
+ struct tcf_gate_params *p;
u64 tcfg_cycletime;
- tcfg_cycletime = to_gate(a)->param.tcfg_cycletime;
+ p = tcf_gate_params(a);
+ tcfg_cycletime = p->tcfg_cycletime;
return tcfg_cycletime;
}
static inline u64 tcf_gate_cycletimeext(const struct tc_action *a)
{
+ struct tcf_gate_params *p;
u64 tcfg_cycletimeext;
- tcfg_cycletimeext = to_gate(a)->param.tcfg_cycletime_ext;
+ p = tcf_gate_params(a);
+ tcfg_cycletimeext = p->tcfg_cycletime_ext;
return tcfg_cycletimeext;
}
static inline u32 tcf_gate_num_entries(const struct tc_action *a)
{
+ struct tcf_gate_params *p;
u32 num_entries;
- num_entries = to_gate(a)->param.num_entries;
+ p = tcf_gate_params(a);
+ num_entries = p->num_entries;
return num_entries;
}
@@ -105,7 +124,7 @@ static inline struct action_gate_entry
u32 num_entries;
int i = 0;
- p = &to_gate(a)->param;
+ p = tcf_gate_params(a);
num_entries = p->num_entries;
list_for_each_entry(entry, &p->entries, list)
diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
index c1f75f2727576..4a1a10bfe3e62 100644
--- a/net/sched/act_gate.c
+++ b/net/sched/act_gate.c
@@ -32,9 +32,12 @@ static ktime_t gate_get_time(struct tcf_gate *gact)
return KTIME_MAX;
}
-static void gate_get_start_time(struct tcf_gate *gact, ktime_t *start)
+static void tcf_gate_params_free_rcu(struct rcu_head *head);
+
+static void gate_get_start_time(struct tcf_gate *gact,
+ const struct tcf_gate_params *param,
+ ktime_t *start)
{
- struct tcf_gate_params *param = &gact->param;
ktime_t now, base, cycle;
u64 n;
@@ -56,11 +59,10 @@ static void gate_start_timer(struct tcf_gate *gact, ktime_t start)
{
ktime_t expires;
- expires = hrtimer_get_expires(&gact->hitimer);
- if (expires == 0)
- expires = KTIME_MAX;
-
- start = min_t(ktime_t, start, expires);
+ if (hrtimer_active(&gact->hitimer)) {
+ expires = hrtimer_get_expires(&gact->hitimer);
+ start = min_t(ktime_t, start, expires);
+ }
hrtimer_start(&gact->hitimer, start, HRTIMER_MODE_ABS_SOFT);
}
@@ -69,12 +71,14 @@ static enum hrtimer_restart gate_timer_func(struct hrtimer *timer)
{
struct tcf_gate *gact = container_of(timer, struct tcf_gate,
hitimer);
- struct tcf_gate_params *p = &gact->param;
struct tcfg_gate_entry *next;
+ struct tcf_gate_params *p;
ktime_t close_time, now;
spin_lock(&gact->tcf_lock);
+ p = rcu_dereference_protected(gact->param,
+ lockdep_is_held(&gact->tcf_lock));
next = gact->next_entry;
/* cycle start, clear pending bit, clear total octets */
@@ -225,6 +229,37 @@ static void release_entry_list(struct list_head *entries)
}
}
+static int tcf_gate_copy_entries(struct tcf_gate_params *dst,
+ const struct tcf_gate_params *src,
+ struct netlink_ext_ack *extack)
+{
+ struct tcfg_gate_entry *entry;
+ int i = 0;
+
+ list_for_each_entry(entry, &src->entries, list) {
+ struct tcfg_gate_entry *new;
+
+ new = kzalloc(sizeof(*new), GFP_ATOMIC);
+ if (!new) {
+ NL_SET_ERR_MSG(extack, "Not enough memory for entry");
+ return -ENOMEM;
+ }
+
+ new->index = entry->index;
+ new->gate_state = entry->gate_state;
+ new->interval = entry->interval;
+ new->ipv = entry->ipv;
+ new->maxoctets = entry->maxoctets;
+ INIT_LIST_HEAD(&new->list);
+ list_add_tail(&new->list, &dst->entries);
+ i++;
+ }
+
+ dst->num_entries = i;
+
+ return 0;
+}
+
static int parse_gate_list(struct nlattr *list_attr,
struct tcf_gate_params *sched,
struct netlink_ext_ack *extack)
@@ -270,22 +305,27 @@ static int parse_gate_list(struct nlattr *list_attr,
return err;
}
-static void gate_setup_timer(struct tcf_gate *gact, u64 basetime,
- enum tk_offsets tko, s32 clockid,
- bool do_init)
+static bool gate_timer_needs_cancel(u64 basetime, u64 old_basetime,
+ enum tk_offsets tko,
+ enum tk_offsets old_tko,
+ s32 clockid, s32 old_clockid, bool do_init)
{
- if (!do_init) {
- if (basetime == gact->param.tcfg_basetime &&
- tko == gact->tk_offset &&
- clockid == gact->param.tcfg_clockid)
- return;
+ if (do_init)
+ return false;
- spin_unlock_bh(&gact->tcf_lock);
- hrtimer_cancel(&gact->hitimer);
- spin_lock_bh(&gact->tcf_lock);
- }
- gact->param.tcfg_basetime = basetime;
- gact->param.tcfg_clockid = clockid;
+ if (basetime != old_basetime)
+ return true;
+ if (clockid != old_clockid)
+ return true;
+ if (tko != old_tko)
+ return true;
+
+ return false;
+}
+
+static void gate_timer_setup(struct tcf_gate *gact, s32 clockid,
+ enum tk_offsets tko)
+{
gact->tk_offset = tko;
hrtimer_setup(&gact->hitimer, gate_timer_func, clockid, HRTIMER_MODE_ABS_SOFT);
}
@@ -296,15 +336,22 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
struct netlink_ext_ack *extack)
{
struct tc_action_net *tn = net_generic(net, act_gate_ops.net_id);
+ u64 cycletime = 0, basetime = 0, cycletime_ext = 0;
+ struct tcf_gate_params *p = NULL, *old_p = NULL;
+ enum tk_offsets old_tk_offset = TK_OFFS_TAI;
enum tk_offsets tk_offset = TK_OFFS_TAI;
bool bind = flags & TCA_ACT_FLAGS_BIND;
struct nlattr *tb[TCA_GATE_MAX + 1];
struct tcf_chain *goto_ch = NULL;
- u64 cycletime = 0, basetime = 0;
- struct tcf_gate_params *p;
+ bool clockid_provided = false;
+ bool use_old_entries = false;
+ s32 old_clockid = CLOCK_TAI;
+ bool need_cancel = false;
s32 clockid = CLOCK_TAI;
struct tcf_gate *gact;
+ bool do_init = false;
struct tc_gate *parm;
+ u64 old_basetime = 0;
int ret = 0, err;
u32 gflags = 0;
s32 prio = -1;
@@ -323,20 +370,11 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
if (tb[TCA_GATE_CLOCKID]) {
clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]);
- switch (clockid) {
- case CLOCK_REALTIME:
- tk_offset = TK_OFFS_REAL;
- break;
- case CLOCK_MONOTONIC:
- tk_offset = TK_OFFS_MAX;
- break;
- case CLOCK_BOOTTIME:
- tk_offset = TK_OFFS_BOOT;
- break;
- case CLOCK_TAI:
- tk_offset = TK_OFFS_TAI;
- break;
- default:
+ clockid_provided = true;
+ if (clockid != CLOCK_REALTIME &&
+ clockid != CLOCK_MONOTONIC &&
+ clockid != CLOCK_BOOTTIME &&
+ clockid != CLOCK_TAI) {
NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
return -EINVAL;
}
@@ -366,6 +404,37 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
return -EEXIST;
}
+ gact = to_gate(*a);
+ err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
+ if (err < 0)
+ goto release_idr;
+
+ if (ret != ACT_P_CREATED) {
+ rcu_read_lock();
+ old_p = rcu_dereference(gact->param);
+ if (old_p) {
+ basetime = old_p->tcfg_basetime;
+ prio = old_p->tcfg_priority;
+ gflags = old_p->tcfg_flags;
+ if (!clockid_provided)
+ clockid = old_p->tcfg_clockid;
+ cycletime_ext = old_p->tcfg_cycletime_ext;
+ old_basetime = old_p->tcfg_basetime;
+ old_clockid = old_p->tcfg_clockid;
+ }
+ do_init = !old_p;
+ old_tk_offset = READ_ONCE(gact->tk_offset);
+ rcu_read_unlock();
+ old_p = NULL;
+ }
+
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
+ if (!p) {
+ err = -ENOMEM;
+ goto release_idr;
+ }
+ INIT_LIST_HEAD(&p->entries);
+
if (tb[TCA_GATE_PRIORITY])
prio = nla_get_s32(tb[TCA_GATE_PRIORITY]);
@@ -375,24 +444,71 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
if (tb[TCA_GATE_FLAGS])
gflags = nla_get_u32(tb[TCA_GATE_FLAGS]);
- gact = to_gate(*a);
- if (ret == ACT_P_CREATED)
- INIT_LIST_HEAD(&gact->param.entries);
+ if (tb[TCA_GATE_CYCLE_TIME])
+ cycletime = nla_get_u64(tb[TCA_GATE_CYCLE_TIME]);
- err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
- if (err < 0)
- goto release_idr;
+ if (tb[TCA_GATE_CYCLE_TIME_EXT])
+ cycletime_ext = nla_get_u64(tb[TCA_GATE_CYCLE_TIME_EXT]);
+
+ switch (clockid) {
+ case CLOCK_REALTIME:
+ tk_offset = TK_OFFS_REAL;
+ break;
+ case CLOCK_MONOTONIC:
+ tk_offset = TK_OFFS_MAX;
+ break;
+ case CLOCK_BOOTTIME:
+ tk_offset = TK_OFFS_BOOT;
+ break;
+ case CLOCK_TAI:
+ tk_offset = TK_OFFS_TAI;
+ break;
+ default:
+ NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
+ err = -EINVAL;
+ goto err_free;
+ }
- spin_lock_bh(&gact->tcf_lock);
- p = &gact->param;
+ need_cancel = gate_timer_needs_cancel(basetime, old_basetime,
+ tk_offset, old_tk_offset,
+ clockid, old_clockid,
+ ret == ACT_P_CREATED || do_init);
- if (tb[TCA_GATE_CYCLE_TIME])
- cycletime = nla_get_u64(tb[TCA_GATE_CYCLE_TIME]);
+ if (need_cancel)
+ hrtimer_cancel(&gact->hitimer);
+
+ spin_lock_bh(&gact->tcf_lock);
+ if (ret != ACT_P_CREATED)
+ old_p = rcu_dereference_protected(gact->param,
+ lockdep_is_held(&gact->tcf_lock));
if (tb[TCA_GATE_ENTRY_LIST]) {
err = parse_gate_list(tb[TCA_GATE_ENTRY_LIST], p, extack);
if (err < 0)
- goto chain_put;
+ goto unlock;
+ if (!err) {
+ if (ret == ACT_P_CREATED) {
+ NL_SET_ERR_MSG(extack, "The entry list is empty");
+ err = -EINVAL;
+ goto unlock;
+ }
+ use_old_entries = true;
+ }
+ } else if (ret == ACT_P_CREATED) {
+ NL_SET_ERR_MSG(extack, "The entry list is empty");
+ err = -EINVAL;
+ goto unlock;
+ } else {
+ use_old_entries = true;
+ }
+
+ if (use_old_entries) {
+ err = tcf_gate_copy_entries(p, old_p, extack);
+ if (err)
+ goto unlock;
+
+ if (!tb[TCA_GATE_CYCLE_TIME])
+ cycletime = old_p->tcfg_cycletime;
}
if (!cycletime) {
@@ -404,20 +520,22 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
cycletime = cycle;
if (!cycletime) {
err = -EINVAL;
- goto chain_put;
+ goto unlock;
}
}
p->tcfg_cycletime = cycletime;
+ p->tcfg_cycletime_ext = cycletime_ext;
- if (tb[TCA_GATE_CYCLE_TIME_EXT])
- p->tcfg_cycletime_ext =
- nla_get_u64(tb[TCA_GATE_CYCLE_TIME_EXT]);
-
- gate_setup_timer(gact, basetime, tk_offset, clockid,
- ret == ACT_P_CREATED);
+ if (need_cancel || ret == ACT_P_CREATED)
+ gate_timer_setup(gact, clockid, tk_offset);
p->tcfg_priority = prio;
p->tcfg_flags = gflags;
- gate_get_start_time(gact, &start);
+ p->tcfg_basetime = basetime;
+ p->tcfg_clockid = clockid;
+ gate_get_start_time(gact, p, &start);
+
+ old_p = rcu_replace_pointer(gact->param, p,
+ lockdep_is_held(&gact->tcf_lock));
gact->current_close_time = start;
gact->current_gate_status = GATE_ACT_GATE_OPEN | GATE_ACT_PENDING;
@@ -434,23 +552,41 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
if (goto_ch)
tcf_chain_put_by_act(goto_ch);
+ if (old_p)
+ call_rcu(&old_p->rcu, tcf_gate_params_free_rcu);
+
return ret;
-chain_put:
+unlock:
spin_unlock_bh(&gact->tcf_lock);
if (goto_ch)
tcf_chain_put_by_act(goto_ch);
+ release_entry_list(&p->entries);
+ kfree(p);
release_idr:
/* action is not inserted in any list: it's safe to init hitimer
* without taking tcf_lock.
*/
if (ret == ACT_P_CREATED)
- gate_setup_timer(gact, gact->param.tcfg_basetime,
- gact->tk_offset, gact->param.tcfg_clockid,
- true);
+ gate_timer_setup(gact, clockid, tk_offset);
tcf_idr_release(*a, bind);
return err;
+
+err_free:
+ if (goto_ch)
+ tcf_chain_put_by_act(goto_ch);
+ release_entry_list(&p->entries);
+ kfree(p);
+ goto release_idr;
+}
+
+static void tcf_gate_params_free_rcu(struct rcu_head *head)
+{
+ struct tcf_gate_params *p = container_of(head, struct tcf_gate_params, rcu);
+
+ release_entry_list(&p->entries);
+ kfree(p);
}
static void tcf_gate_cleanup(struct tc_action *a)
@@ -458,9 +594,10 @@ static void tcf_gate_cleanup(struct tc_action *a)
struct tcf_gate *gact = to_gate(a);
struct tcf_gate_params *p;
- p = &gact->param;
hrtimer_cancel(&gact->hitimer);
- release_entry_list(&p->entries);
+ p = rcu_replace_pointer(gact->param, NULL, 1);
+ if (p)
+ call_rcu(&p->rcu, tcf_gate_params_free_rcu);
}
static int dumping_entry(struct sk_buff *skb,
@@ -512,7 +649,8 @@ static int tcf_gate_dump(struct sk_buff *skb, struct tc_action *a,
spin_lock_bh(&gact->tcf_lock);
opt.action = gact->tcf_action;
- p = &gact->param;
+ p = rcu_dereference_protected(gact->param,
+ lockdep_is_held(&gact->tcf_lock));
if (nla_put(skb, TCA_GATE_PARMS, sizeof(opt), &opt))
goto nla_put_failure;
--
2.52.GIT
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net v5 1/1] net/sched: act_gate: snapshot parameters with RCU on replace
2026-02-05 15:10 ` [PATCH net v5 1/1] " Paul Moses
@ 2026-02-06 10:36 ` Victor Nogueira
2026-02-12 12:17 ` Paul Moses
0 siblings, 1 reply; 5+ messages in thread
From: Victor Nogueira @ 2026-02-06 10:36 UTC (permalink / raw)
To: Paul Moses
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev,
linux-kernel, stable
> The gate action can be replaced while the hrtimer callback or dump path is
> walking the schedule list.
>
> Convert the parameters to an RCU-protected snapshot and swap updates under
> tcf_lock, freeing the previous snapshot via call_rcu(). When REPLACE omits
> the entry list, preserve the existing schedule so the effective state is
> unchanged.
> [...]
> diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> index c1f75f2727576..4a1a10bfe3e62 100644
> [...]
> -static void gate_setup_timer(struct tcf_gate *gact, u64 basetime,
> - enum tk_offsets tko, s32 clockid,
> - bool do_init)
> [...]
> +static void gate_timer_setup(struct tcf_gate *gact, s32 clockid,
> + enum tk_offsets tko)
> [...]
I don't believe you need to change the function name here.
> [...]
> @@ -323,20 +370,11 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>
> if (tb[TCA_GATE_CLOCKID]) {
> clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]);
> - switch (clockid) {
> - case CLOCK_REALTIME:
> - tk_offset = TK_OFFS_REAL;
> - break;
> - case CLOCK_MONOTONIC:
> - tk_offset = TK_OFFS_MAX;
> - break;
> - case CLOCK_BOOTTIME:
> - tk_offset = TK_OFFS_BOOT;
> - break;
> - case CLOCK_TAI:
> - tk_offset = TK_OFFS_TAI;
> - break;
> - default:
> + clockid_provided = true;
> + if (clockid != CLOCK_REALTIME &&
> + clockid != CLOCK_MONOTONIC &&
> + clockid != CLOCK_BOOTTIME &&
> + clockid != CLOCK_TAI) {
> NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
> return -EINVAL;
> }
This is better than what you had before, however it still
is redundant given that you do the switch statement later
and perform the same validation again. If there's no reason to
keep this code, you probably can also get rid of "clockid_provided".
> @@ -366,6 +404,37 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> [...]
> +
> + if (ret != ACT_P_CREATED) {
> + rcu_read_lock();
> + old_p = rcu_dereference(gact->param);
> + if (old_p) {
When do you believe old_p might be NULL here?
From what I understand, you can't arrive here while
a delete for the same action instance is happening in parallel.
Were you able to create such scenario when testing gate?
> [...]
> + if (use_old_entries) {
> + err = tcf_gate_copy_entries(p, old_p, extack);
> + if (err)
> + goto unlock;
> +
> + if (!tb[TCA_GATE_CYCLE_TIME])
> + cycletime = old_p->tcfg_cycletime;
Why did you keep this one as in v4?
You don't want to reuse the old "cycletime" if the user
specified new entries?
Not saying you are necessarily wrong.
Just trying to understand your logic.
> [...]
> -chain_put:
> +unlock:
> spin_unlock_bh(&gact->tcf_lock);
>
> if (goto_ch)
> tcf_chain_put_by_act(goto_ch);
> + release_entry_list(&p->entries);
> + kfree(p);
The 4 lines above look exactly like what you
do in err_free. Can't you label them as err_free
and remove the lines below?
> [...]
> +err_free:
> + if (goto_ch)
> + tcf_chain_put_by_act(goto_ch);
> + release_entry_list(&p->entries);
> + kfree(p);
> + goto release_idr;
> +}
> [...]
> static void tcf_gate_cleanup(struct tc_action *a)
> @@ -458,9 +594,10 @@ static void tcf_gate_cleanup(struct tc_action *a)
> struct tcf_gate *gact = to_gate(a);
> struct tcf_gate_params *p;
>
> - p = &gact->param;
> hrtimer_cancel(&gact->hitimer);
> - release_entry_list(&p->entries);
> + p = rcu_replace_pointer(gact->param, NULL, 1);
> + if (p)
> + call_rcu(&p->rcu, tcf_gate_params_free_rcu);
> }
Sorry, I think I lacked precision in my last comment.
I meant that you should've removed the rtnl requirement
(which you did), but also use rcu_dereference_protected as
act_vlan does. This relates to my previous comment on "old_p"
being NULL. I don't believe you need to set this to NULL
unless you were able to reproduce the scenario I described
earlier.
> static int dumping_entry(struct sk_buff *skb,
> @@ -512,7 +649,8 @@ static int tcf_gate_dump(struct sk_buff *skb, struct tc_action *a,
> spin_lock_bh(&gact->tcf_lock);
> opt.action = gact->tcf_action;
>
> - p = &gact->param;
> + p = rcu_dereference_protected(gact->param,
> + lockdep_is_held(&gact->tcf_lock));
You could've kept the rcu_read_lock approach here.
One of the main advantages of making the params rcu
is being able to dump without the tcf_lock.
cheers,
Victor
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v5 1/1] net/sched: act_gate: snapshot parameters with RCU on replace
2026-02-06 10:36 ` Victor Nogueira
@ 2026-02-12 12:17 ` Paul Moses
2026-02-12 14:59 ` Victor Nogueira
0 siblings, 1 reply; 5+ messages in thread
From: Paul Moses @ 2026-02-12 12:17 UTC (permalink / raw)
To: Victor Nogueira
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev,
linux-kernel, stable
Proposed changes from v5...v6:
1. Agreed
-Fixed in (net/sched: act_gate: keep gate_setup_timer helper name)
2. Agreed
-Fixed in (net/sched: act_gate: drop redundant clockid pre-validation)
3. I was not able to reproduce it. I tended to keep it since NULL became
representable in the conversion and it was not an expensive branch.
-Fixed in (net/sched: act_gate: assume params exist on replace path)
4. use_old_entries is true only when REPLACE does not provide a usable new entry
list (missing or empty) and we copy the previous entries into p to preserve
effective behavior. This block is skipped when new entries are provided, so
old cycletime is not reused in that case. It could be clearer but I didn't
think it was worth the diff increase.
5. Yes
-Fixed in (net/sched: act_gate: deduplicate init error cleanup labels)
6. Agreed
-Fixed in (net/sched: act_gate: align cleanup dereference with act_vlan)
7. Agreed
-Fixed in (net/sched: act_gate: dump params under rcu read-side lock)
Thanks,
Paul
On Friday, February 6th, 2026 at 4:36 AM, Victor Nogueira <victor@mojatatu.com> wrote:
> > The gate action can be replaced while the hrtimer callback or dump path is
> > walking the schedule list.
> >
> > Convert the parameters to an RCU-protected snapshot and swap updates under
> > tcf_lock, freeing the previous snapshot via call_rcu(). When REPLACE omits
> > the entry list, preserve the existing schedule so the effective state is
> > unchanged.
> > [...]
> > diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> > index c1f75f2727576..4a1a10bfe3e62 100644
> > [...]
> > -static void gate_setup_timer(struct tcf_gate *gact, u64 basetime,
> > - enum tk_offsets tko, s32 clockid,
> > - bool do_init)
> > [...]
> > +static void gate_timer_setup(struct tcf_gate *gact, s32 clockid,
> > + enum tk_offsets tko)
> > [...]
>
> I don't believe you need to change the function name here.
>
> > [...]
> > @@ -323,20 +370,11 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> >
> > if (tb[TCA_GATE_CLOCKID]) {
> > clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]);
> > - switch (clockid) {
> > - case CLOCK_REALTIME:
> > - tk_offset = TK_OFFS_REAL;
> > - break;
> > - case CLOCK_MONOTONIC:
> > - tk_offset = TK_OFFS_MAX;
> > - break;
> > - case CLOCK_BOOTTIME:
> > - tk_offset = TK_OFFS_BOOT;
> > - break;
> > - case CLOCK_TAI:
> > - tk_offset = TK_OFFS_TAI;
> > - break;
> > - default:
> > + clockid_provided = true;
> > + if (clockid != CLOCK_REALTIME &&
> > + clockid != CLOCK_MONOTONIC &&
> > + clockid != CLOCK_BOOTTIME &&
> > + clockid != CLOCK_TAI) {
> > NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
> > return -EINVAL;
> > }
>
> This is better than what you had before, however it still
> is redundant given that you do the switch statement later
> and perform the same validation again. If there's no reason to
> keep this code, you probably can also get rid of "clockid_provided".
>
> > @@ -366,6 +404,37 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> > [...]
> > +
> > + if (ret != ACT_P_CREATED) {
> > + rcu_read_lock();
> > + old_p = rcu_dereference(gact->param);
> > + if (old_p) {
>
> When do you believe old_p might be NULL here?
> From what I understand, you can't arrive here while
> a delete for the same action instance is happening in parallel.
> Were you able to create such scenario when testing gate?
>
> > [...]
> > + if (use_old_entries) {
> > + err = tcf_gate_copy_entries(p, old_p, extack);
> > + if (err)
> > + goto unlock;
> > +
> > + if (!tb[TCA_GATE_CYCLE_TIME])
> > + cycletime = old_p->tcfg_cycletime;
>
> Why did you keep this one as in v4?
> You don't want to reuse the old "cycletime" if the user
> specified new entries?
> Not saying you are necessarily wrong.
> Just trying to understand your logic.
>
> > [...]
> > -chain_put:
> > +unlock:
> > spin_unlock_bh(&gact->tcf_lock);
> >
> > if (goto_ch)
> > tcf_chain_put_by_act(goto_ch);
> > + release_entry_list(&p->entries);
> > + kfree(p);
>
> The 4 lines above look exactly like what you
> do in err_free. Can't you label them as err_free
> and remove the lines below?
>
> > [...]
> > +err_free:
> > + if (goto_ch)
> > + tcf_chain_put_by_act(goto_ch);
> > + release_entry_list(&p->entries);
> > + kfree(p);
> > + goto release_idr;
> > +}
> > [...]
> > static void tcf_gate_cleanup(struct tc_action *a)
> > @@ -458,9 +594,10 @@ static void tcf_gate_cleanup(struct tc_action *a)
> > struct tcf_gate *gact = to_gate(a);
> > struct tcf_gate_params *p;
> >
> > - p = &gact->param;
> > hrtimer_cancel(&gact->hitimer);
> > - release_entry_list(&p->entries);
> > + p = rcu_replace_pointer(gact->param, NULL, 1);
> > + if (p)
> > + call_rcu(&p->rcu, tcf_gate_params_free_rcu);
> > }
>
> Sorry, I think I lacked precision in my last comment.
> I meant that you should've removed the rtnl requirement
> (which you did), but also use rcu_dereference_protected as
> act_vlan does. This relates to my previous comment on "old_p"
> being NULL. I don't believe you need to set this to NULL
> unless you were able to reproduce the scenario I described
> earlier.
>
> > static int dumping_entry(struct sk_buff *skb,
> > @@ -512,7 +649,8 @@ static int tcf_gate_dump(struct sk_buff *skb, struct tc_action *a,
> > spin_lock_bh(&gact->tcf_lock);
> > opt.action = gact->tcf_action;
> >
> > - p = &gact->param;
> > + p = rcu_dereference_protected(gact->param,
> > + lockdep_is_held(&gact->tcf_lock));
>
> You could've kept the rcu_read_lock approach here.
> One of the main advantages of making the params rcu
> is being able to dump without the tcf_lock.
>
> cheers,
> Victor
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v5 1/1] net/sched: act_gate: snapshot parameters with RCU on replace
2026-02-12 12:17 ` Paul Moses
@ 2026-02-12 14:59 ` Victor Nogueira
0 siblings, 0 replies; 5+ messages in thread
From: Victor Nogueira @ 2026-02-12 14:59 UTC (permalink / raw)
To: Paul Moses
Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman, netdev,
linux-kernel, stable
On Thu, Feb 12, 2026 at 9:18 AM Paul Moses <p@1g4.org> wrote:
>
> Proposed changes from v5...v6:
> [...]
I think I understand what you are saying, but I believe I'll be able to
give you better comments once I see the code.
cheers,
Victor
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-02-12 14:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-02-05 15:10 [PATCH net v5 0/1] net/sched: act_gate: snapshot parameters with RCU on replace Paul Moses
2026-02-05 15:10 ` [PATCH net v5 1/1] " Paul Moses
2026-02-06 10:36 ` Victor Nogueira
2026-02-12 12:17 ` Paul Moses
2026-02-12 14:59 ` Victor Nogueira
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox