Netdev List
 help / color / mirror / Atom feed
* [PATCH net] net/sched: add get_fill_size callbacks for actions missing them
@ 2026-08-10 16:43 Victor Nogueira
  2026-08-12  1:19 ` Jakub Kicinski
  0 siblings, 1 reply; 4+ messages in thread
From: Victor Nogueira @ 2026-08-10 16:43 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, jhs, jiri, daniel, john.fastabend,
	sdf, martin.lau, ast
  Cc: andrii, eddyz87, memxor, song, jolsa, emil, horms, vega, netdev,
	bpf

Several tc actions (act_police, act_bpf, act_pedit, act_ife, act_sample,
act_ct, act_ctinfo, act_tunnel_key) provide no get_fill_size() callback,
so tcf_action_fill_size() falls back to tcf_action_shared_attrs_size()
which does not account for the action-specific netlink attributes emitted
inside TCA_ACT_OPTIONS by their dump functions.

When an RTM_NEWACTION request with NLM_F_ECHO (or an RTNLGRP_TC
listener) creates several actions, tcf_add_notify_msg() allocates the
echo skb from this underestimated size. act_bpf is the clearest case:
TCA_ACT_BPF_OPS alone reaches 32KB per action (BPF_MAXINSNS), so two
actions are enough to overrun the allocation. act_pedit overruns with
32 actions of four munge keys each, act_police with 32 policers once the
optional rate/peakrate/result/avrate attributes are present, and
act_tunnel_key with a single action carrying a maximum-sized geneve
option blob, whose 63 class/type/data attribute triplets expand to about
1.2KB. tca_get_fill() then fails and the request returns -EINVAL, but
tcf_action_init() has already committed the actions via
tcf_idr_insert_many(), so they stay installed even though userspace is
told the request failed.

To fix this, add the missing get_fill_size callbacks returning the
worst-case size of each action's dump attributes, following the pattern
used by act_gact/act_skbedit/act_vlan.

Note: We only provided fixes for the actions we reproduced this bug with
as of today. We can send a separate hardening patch for the remaining
actions to net-next later.

Fixes: 4e76e75d6aba ("net sched actions: calculate add/delete event message size")
Reported-by: Vega <vega@nebusec.ai>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/act_api.c        |  5 ++-
 net/sched/act_bpf.c        | 21 ++++++++++
 net/sched/act_ct.c         | 46 ++++++++++++++++++++++
 net/sched/act_ctinfo.c     | 11 ++++++
 net/sched/act_ife.c        | 23 +++++++++++
 net/sched/act_pedit.c      | 24 ++++++++++++
 net/sched/act_police.c     | 12 ++++++
 net/sched/act_sample.c     |  9 +++++
 net/sched/act_tunnel_key.c | 80 ++++++++++++++++++++++++++++++++++++++
 9 files changed, 230 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index f141634df214..9b8d91030381 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -454,7 +454,10 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act)
 		/* TCA_STATS_QUEUE */
 		+ nla_total_size_64bit(sizeof(struct gnet_stats_queue))
 		+ nla_total_size(0) /* TCA_ACT_OPTIONS nested */
-		+ nla_total_size(sizeof(struct tcf_t)); /* TCA_GACT_TM */
+		/* TCA_GACT_TM; actions dump their tcf_t with nla_put_64bit(),
+		 * which may emit an extra NLA_PAD attribute.
+		 */
+		+ nla_total_size_64bit(sizeof(struct tcf_t));
 }
 
 static size_t tcf_action_full_attrs_size(size_t sz)
diff --git a/net/sched/act_bpf.c b/net/sched/act_bpf.c
index 09d46e195e33..db4a5ffbb0df 100644
--- a/net/sched/act_bpf.c
+++ b/net/sched/act_bpf.c
@@ -389,6 +389,26 @@ static void tcf_bpf_cleanup(struct tc_action *act)
 	tcf_bpf_cfg_cleanup(&tmp);
 }
 
+static size_t tcf_bpf_get_fill_size(const struct tc_action *act)
+{
+	struct tcf_bpf *prog = to_bpf(act);
+	size_t size = nla_total_size(sizeof(struct tc_act_bpf));
+
+	if (tcf_bpf_is_ebpf(prog)) {
+		/* TCA_ACT_BPF_NAME */
+		size += nla_total_size(ACT_BPF_NAME_LEN + 1);
+		size += nla_total_size(sizeof(u32)); /* TCA_ACT_BPF_ID */
+		size += nla_total_size(BPF_TAG_SIZE); /* TCA_ACT_BPF_TAG */
+	} else {
+		size += nla_total_size(sizeof(u16)); /* TCA_ACT_BPF_OPS_LEN */
+		/* TCA_ACT_BPF_OPS */
+		size += nla_total_size(prog->bpf_num_ops *
+				       sizeof(struct sock_filter));
+	}
+
+	return size;
+}
+
 static struct tc_action_ops act_bpf_ops __read_mostly = {
 	.kind		=	"bpf",
 	.id		=	TCA_ID_BPF,
@@ -397,6 +417,7 @@ static struct tc_action_ops act_bpf_ops __read_mostly = {
 	.dump		=	tcf_bpf_dump,
 	.cleanup	=	tcf_bpf_cleanup,
 	.init		=	tcf_bpf_init,
+	.get_fill_size	=	tcf_bpf_get_fill_size,
 	.size		=	sizeof(struct tcf_bpf),
 };
 MODULE_ALIAS_NET_ACT("bpf");
diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
index be535a261fa0..e9c6d420ab88 100644
--- a/net/sched/act_ct.c
+++ b/net/sched/act_ct.c
@@ -1636,6 +1636,51 @@ static int tcf_ct_offload_act_setup(struct tc_action *act, void *entry_data,
 	return 0;
 }
 
+static size_t tcf_ct_get_fill_size(const struct tc_action *act)
+{
+	const struct tcf_ct_params *p;
+	size_t size;
+
+	size = nla_total_size(sizeof(struct tc_ct)) /* TCA_CT_PARMS */
+		+ nla_total_size(sizeof(u16)); /* TCA_CT_ACTION */
+
+	rcu_read_lock();
+	p = rcu_dereference(to_ct(act)->params);
+
+	if (p->ct_action & TCA_CT_ACT_CLEAR)
+		goto out;
+
+	/* TCA_CT_MARK, TCA_CT_MARK_MASK */
+	if (IS_ENABLED(CONFIG_NF_CONNTRACK_MARK))
+		size += nla_total_size(sizeof(p->mark))
+			+ nla_total_size(sizeof(p->mark_mask));
+
+	/* TCA_CT_LABELS, TCA_CT_LABELS_MASK */
+	if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS))
+		size += nla_total_size(sizeof(p->labels))
+			+ nla_total_size(sizeof(p->labels_mask));
+
+	if (IS_ENABLED(CONFIG_NF_CONNTRACK_ZONES))
+		size += nla_total_size(sizeof(p->zone)); /* TCA_CT_ZONE */
+
+	if (p->ct_action & TCA_CT_ACT_NAT)
+		/* TCA_CT_NAT_IPV6_{MIN,MAX}, the larger of the two address
+		 * variants, plus TCA_CT_NAT_PORT_{MIN,MAX}.
+		 */
+		size += 2 * nla_total_size(sizeof(struct in6_addr))
+			+ 2 * nla_total_size(sizeof(__be16));
+
+	/* TCA_CT_HELPER_{NAME,FAMILY,PROTO} */
+	if (p->helper)
+		size += nla_total_size(NF_CT_HELPER_NAME_LEN)
+			+ nla_total_size(sizeof(u8))
+			+ nla_total_size(sizeof(u8));
+out:
+	rcu_read_unlock();
+
+	return size;
+}
+
 static struct tc_action_ops act_ct_ops = {
 	.kind		=	"ct",
 	.id		=	TCA_ID_CT,
@@ -1645,6 +1690,7 @@ static struct tc_action_ops act_ct_ops = {
 	.init		=	tcf_ct_init,
 	.cleanup	=	tcf_ct_cleanup,
 	.stats_update	=	tcf_stats_update,
+	.get_fill_size	=	tcf_ct_get_fill_size,
 	.offload_act_setup =	tcf_ct_offload_act_setup,
 	.size		=	sizeof(struct tcf_ct),
 };
diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
index 1886ffd2ca95..fced4b1094af 100644
--- a/net/sched/act_ctinfo.c
+++ b/net/sched/act_ctinfo.c
@@ -356,6 +356,16 @@ static void tcf_ctinfo_cleanup(struct tc_action *a)
 		kfree_rcu(cp, rcu);
 }
 
+static size_t tcf_ctinfo_get_fill_size(const struct tc_action *act)
+{
+	return nla_total_size(sizeof(struct tc_ctinfo)) /* TCA_CTINFO_ACT */
+		+ nla_total_size(sizeof(u16)) /* TCA_CTINFO_ZONE */
+		/* TCA_CTINFO_PARMS_{DSCP_MASK,DSCP_STATEMASK,CPMARK_MASK} */
+		+ 3 * nla_total_size(sizeof(u32))
+		/* TCA_CTINFO_STATS_{DSCP_SET,DSCP_ERROR,CPMARK_SET} */
+		+ 3 * nla_total_size_64bit(sizeof(u64));
+}
+
 static struct tc_action_ops act_ctinfo_ops = {
 	.kind	= "ctinfo",
 	.id	= TCA_ID_CTINFO,
@@ -364,6 +374,7 @@ static struct tc_action_ops act_ctinfo_ops = {
 	.dump	= tcf_ctinfo_dump,
 	.init	= tcf_ctinfo_init,
 	.cleanup= tcf_ctinfo_cleanup,
+	.get_fill_size = tcf_ctinfo_get_fill_size,
 	.size	= sizeof(struct tcf_ctinfo),
 };
 MODULE_ALIAS_NET_ACT("ctinfo");
diff --git a/net/sched/act_ife.c b/net/sched/act_ife.c
index 065228026c58..ff2b16e35b9b 100644
--- a/net/sched/act_ife.c
+++ b/net/sched/act_ife.c
@@ -878,6 +878,28 @@ TC_INDIRECT_SCOPE int tcf_ife_act(struct sk_buff *skb,
 	return tcf_ife_decode(skb, a, res);
 }
 
+static size_t tcf_ife_get_fill_size(const struct tc_action *act)
+{
+	struct tcf_ife_info *ife = to_ife(act);
+	const struct tcf_ife_params *p;
+	struct tcf_meta_info *e;
+	size_t size = nla_total_size(sizeof(struct tc_ife)) /* TCA_IFE_PARMS */
+		+ nla_total_size(ETH_ALEN) /* TCA_IFE_DMAC */
+		+ nla_total_size(ETH_ALEN) /* TCA_IFE_SMAC */
+		+ nla_total_size(2) /* TCA_IFE_TYPE */
+		+ nla_total_size(0); /* TCA_IFE_METALST */
+
+	rcu_read_lock();
+	p = rcu_dereference(ife->params);
+	if (p) {
+		list_for_each_entry_rcu(e, &p->metalist, metalist)
+			size += nla_total_size(sizeof(u32));
+	}
+	rcu_read_unlock();
+
+	return size;
+}
+
 static struct tc_action_ops act_ife_ops = {
 	.kind = "ife",
 	.id = TCA_ID_IFE,
@@ -886,6 +908,7 @@ static struct tc_action_ops act_ife_ops = {
 	.dump = tcf_ife_dump,
 	.cleanup = tcf_ife_cleanup,
 	.init = tcf_ife_init,
+	.get_fill_size = tcf_ife_get_fill_size,
 	.size =	sizeof(struct tcf_ife_info),
 };
 MODULE_ALIAS_NET_ACT("ife");
diff --git a/net/sched/act_pedit.c b/net/sched/act_pedit.c
index d4d47a9921f4..99d7e36510bd 100644
--- a/net/sched/act_pedit.c
+++ b/net/sched/act_pedit.c
@@ -626,6 +626,29 @@ static int tcf_pedit_offload_act_setup(struct tc_action *act, void *entry_data,
 	return 0;
 }
 
+static size_t tcf_pedit_get_fill_size(const struct tc_action *act)
+{
+	const struct tcf_pedit_parms *parms;
+	size_t size;
+
+	rcu_read_lock();
+	parms = rcu_dereference(to_pedit(act)->parms);
+	size = nla_total_size(struct_size_t(struct tc_pedit, keys,
+					    parms->tcfp_nkeys));
+	if (parms->tcfp_keys_ex) {
+		/* TCA_PEDIT_KEYS_EX, holding one TCA_PEDIT_KEY_EX nest with a
+		 * HTYPE and a CMD attribute per key.
+		 */
+		size += nla_total_size(0)
+			+ parms->tcfp_nkeys * (nla_total_size(0)
+					       + nla_total_size(sizeof(u16))
+					       + nla_total_size(sizeof(u16)));
+	}
+	rcu_read_unlock();
+
+	return size;
+}
+
 static struct tc_action_ops act_pedit_ops = {
 	.kind		=	"pedit",
 	.id		=	TCA_ID_PEDIT,
@@ -635,6 +658,7 @@ static struct tc_action_ops act_pedit_ops = {
 	.dump		=	tcf_pedit_dump,
 	.cleanup	=	tcf_pedit_cleanup,
 	.init		=	tcf_pedit_init,
+	.get_fill_size	=	tcf_pedit_get_fill_size,
 	.offload_act_setup =	tcf_pedit_offload_act_setup,
 	.size		=	sizeof(struct tcf_pedit),
 };
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index b16468a98c55..1a5a69768702 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -484,6 +484,17 @@ static int tcf_police_offload_act_setup(struct tc_action *act, void *entry_data,
 	return 0;
 }
 
+static size_t tcf_police_get_fill_size(const struct tc_action *act)
+{
+	return nla_total_size(sizeof(struct tc_police)) /* TCA_POLICE_TBF */
+		+ nla_total_size_64bit(sizeof(u64)) /* TCA_POLICE_RATE64 */
+		+ nla_total_size_64bit(sizeof(u64)) /* TCA_POLICE_PEAKRATE64 */
+		+ nla_total_size_64bit(sizeof(u64)) /* TCA_POLICE_PKTRATE64 */
+		+ nla_total_size_64bit(sizeof(u64)) /* TCA_POLICE_PKTBURST64 */
+		+ nla_total_size(sizeof(u32)) /* TCA_POLICE_RESULT */
+		+ nla_total_size(sizeof(u32)); /* TCA_POLICE_AVRATE */
+}
+
 MODULE_AUTHOR("Alexey Kuznetsov");
 MODULE_DESCRIPTION("Policing actions");
 MODULE_LICENSE("GPL");
@@ -497,6 +508,7 @@ static struct tc_action_ops act_police_ops = {
 	.dump		=	tcf_police_dump,
 	.init		=	tcf_police_init,
 	.cleanup	=	tcf_police_cleanup,
+	.get_fill_size	=	tcf_police_get_fill_size,
 	.offload_act_setup =	tcf_police_offload_act_setup,
 	.size		=	sizeof(struct tcf_police),
 };
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 2ceb4d141b71..44319a159b55 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -315,6 +315,14 @@ static int tcf_sample_offload_act_setup(struct tc_action *act, void *entry_data,
 	return 0;
 }
 
+static size_t tcf_sample_get_fill_size(const struct tc_action *act)
+{
+	return nla_total_size(sizeof(struct tc_sample)) /* TCA_SAMPLE_PARMS */
+		+ nla_total_size(sizeof(u32)) /* TCA_SAMPLE_RATE */
+		+ nla_total_size(sizeof(u32)) /* TCA_SAMPLE_TRUNC_SIZE */
+		+ nla_total_size(sizeof(u32)); /* TCA_SAMPLE_PSAMPLE_GROUP */
+}
+
 static struct tc_action_ops act_sample_ops = {
 	.kind	  = "sample",
 	.id	  = TCA_ID_SAMPLE,
@@ -324,6 +332,7 @@ static struct tc_action_ops act_sample_ops = {
 	.dump	  = tcf_sample_dump,
 	.init	  = tcf_sample_init,
 	.cleanup  = tcf_sample_cleanup,
+	.get_fill_size = tcf_sample_get_fill_size,
 	.get_psample_group = tcf_sample_get_group,
 	.offload_act_setup    = tcf_sample_offload_act_setup,
 	.size	  = sizeof(struct tcf_sample),
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index b14807761d82..ff401ace4f3d 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -835,6 +835,85 @@ static int tcf_tunnel_key_offload_act_setup(struct tc_action *act,
 	return 0;
 }
 
+static size_t
+tunnel_key_geneve_opts_fill_size(const struct ip_tunnel_info *info)
+{
+	const u8 *src = ip_tunnel_info_opts(info);
+	int len = info->options_len;
+	size_t size = 0;
+
+	while (len > 0) {
+		const struct geneve_opt *opt = (const struct geneve_opt *)src;
+
+		/* TCA_TUNNEL_KEY_ENC_OPT_GENEVE_{CLASS,TYPE,DATA} */
+		size += nla_total_size(2)
+			+ nla_total_size(1)
+			+ nla_total_size(opt->length * 4);
+
+		len -= sizeof(struct geneve_opt) + opt->length * 4;
+		src += sizeof(struct geneve_opt) + opt->length * 4;
+	}
+
+	return size;
+}
+
+static size_t tunnel_key_opts_fill_size(const struct ip_tunnel_info *info)
+{
+	size_t size;
+
+	if (!info->options_len)
+		return 0;
+
+	/* TCA_TUNNEL_KEY_ENC_OPTS and the per-protocol nest inside it */
+	size = nla_total_size(0) + nla_total_size(0);
+
+	if (test_bit(IP_TUNNEL_GENEVE_OPT_BIT, info->key.tun_flags)) {
+		size += tunnel_key_geneve_opts_fill_size(info);
+	} else if (test_bit(IP_TUNNEL_VXLAN_OPT_BIT, info->key.tun_flags)) {
+		/* TCA_TUNNEL_KEY_ENC_OPT_VXLAN_GBP */
+		size += nla_total_size(sizeof(u32));
+	} else if (test_bit(IP_TUNNEL_ERSPAN_OPT_BIT, info->key.tun_flags)) {
+		/* TCA_TUNNEL_KEY_ENC_OPT_ERSPAN_{VER,INDEX,DIR,HWID} */
+		size += nla_total_size(sizeof(u8))
+			+ nla_total_size(sizeof(__be32))
+			+ nla_total_size(sizeof(u8))
+			+ nla_total_size(sizeof(u8));
+	}
+
+	return size;
+}
+
+static size_t tunnel_key_get_fill_size(const struct tc_action *act)
+{
+	struct tcf_tunnel_key *t = to_tunnel_key(act);
+	const struct tcf_tunnel_key_params *params;
+	/* TCA_TUNNEL_KEY_PARMS */
+	size_t size = nla_total_size(sizeof(struct tc_tunnel_key));
+
+	rcu_read_lock();
+	params = rcu_dereference(t->params);
+	if (params->tcft_action == TCA_TUNNEL_KEY_ACT_SET) {
+		const struct ip_tunnel_info *info =
+			&params->tcft_enc_metadata->u.tun_info;
+
+		/* In dump order: TCA_TUNNEL_KEY_ENC_KEY_ID, the IPv6 address
+		 * pair (larger than the IPv4 one), ..._ENC_DST_PORT,
+		 * ..._NO_CSUM, ..._NO_FRAG, the options and ..._ENC_{TOS,TTL}.
+		 */
+		size += nla_total_size(sizeof(__be32))
+			+ 2 * nla_total_size(sizeof(struct in6_addr))
+			+ nla_total_size(sizeof(__be16))
+			+ nla_total_size(sizeof(u8))
+			+ nla_total_size(0)
+			+ tunnel_key_opts_fill_size(info)
+			+ nla_total_size(sizeof(u8))
+			+ nla_total_size(sizeof(u8));
+	}
+	rcu_read_unlock();
+
+	return size;
+}
+
 static struct tc_action_ops act_tunnel_key_ops = {
 	.kind		=	"tunnel_key",
 	.id		=	TCA_ID_TUNNEL_KEY,
@@ -843,6 +922,7 @@ static struct tc_action_ops act_tunnel_key_ops = {
 	.dump		=	tunnel_key_dump,
 	.init		=	tunnel_key_init,
 	.cleanup	=	tunnel_key_release,
+	.get_fill_size	=	tunnel_key_get_fill_size,
 	.offload_act_setup =	tcf_tunnel_key_offload_act_setup,
 	.size		=	sizeof(struct tcf_tunnel_key),
 };
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net/sched: add get_fill_size callbacks for actions missing them
  2026-08-10 16:43 [PATCH net] net/sched: add get_fill_size callbacks for actions missing them Victor Nogueira
@ 2026-08-12  1:19 ` Jakub Kicinski
  2026-08-12 17:53   ` Victor Nogueira
  0 siblings, 1 reply; 4+ messages in thread
From: Jakub Kicinski @ 2026-08-12  1:19 UTC (permalink / raw)
  To: Victor Nogueira
  Cc: davem, edumazet, pabeni, jhs, jiri, daniel, john.fastabend, sdf,
	martin.lau, ast, andrii, eddyz87, memxor, song, jolsa, emil,
	horms, vega, netdev, bpf

On Mon, 10 Aug 2026 13:43:57 -0300 Victor Nogueira wrote:
> When an RTM_NEWACTION request with NLM_F_ECHO (or an RTNLGRP_TC
> listener) creates several actions, tcf_add_notify_msg() allocates the
> echo skb from this underestimated size. act_bpf is the clearest case:
> TCA_ACT_BPF_OPS alone reaches 32KB per action (BPF_MAXINSNS), so two
> actions are enough to overrun the allocation. act_pedit overruns with
> 32 actions of four munge keys each,

"overruns" makes it sounds like we overflow a buffer.
AFAICT we'd at worse hit a WARN somewhere due to EMSGSIZE?

> act_police with 32 policers once the
> optional rate/peakrate/result/avrate attributes are present, and
> act_tunnel_key with a single action carrying a maximum-sized geneve
> option blob, whose 63 class/type/data attribute triplets expand to about
> 1.2KB. tca_get_fill() then fails and the request returns -EINVAL, but
> tcf_action_init() has already committed the actions via
> tcf_idr_insert_many(), so they stay installed even though userspace is
> told the request failed.

Please TAL at Clashiko review. 

One thing that'd be good to clarify is which WARN gets hit (if my
assumption is correct that it's a WARN) and that the path where it
hits has a lock covering the get_size and the rest of the operation.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net/sched: add get_fill_size callbacks for actions missing them
  2026-08-12  1:19 ` Jakub Kicinski
@ 2026-08-12 17:53   ` Victor Nogueira
  2026-08-13  0:46     ` Jakub Kicinski
  0 siblings, 1 reply; 4+ messages in thread
From: Victor Nogueira @ 2026-08-12 17:53 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, edumazet, pabeni, jhs, jiri, daniel, john.fastabend, sdf,
	martin.lau, ast, andrii, eddyz87, memxor, song, jolsa, emil,
	horms, vega, netdev, bpf

On Tue, Aug 11, 2026 at 10:20 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 10 Aug 2026 13:43:57 -0300 Victor Nogueira wrote:
> > When an RTM_NEWACTION request with NLM_F_ECHO (or an RTNLGRP_TC
> > listener) creates several actions, tcf_add_notify_msg() allocates the
> > echo skb from this underestimated size. act_bpf is the clearest case:
> > TCA_ACT_BPF_OPS alone reaches 32KB per action (BPF_MAXINSNS), so two
> > actions are enough to overrun the allocation. act_pedit overruns with
> > 32 actions of four munge keys each,
>
> "overruns" makes it sounds like we overflow a buffer.

Fair enough, it doesn't actually overflow a buffer.
I can change that.

> AFAICT we'd at worse hit a WARN somewhere due to EMSGSIZE?

The issue is more subtle.
The kernel might create the actions, but fail to notify userspace
because the notification message is too large for the allocated skb.
So it's not a crash or a WARN, but rather a correctness issue.

> > act_police with 32 policers once the
> > optional rate/peakrate/result/avrate attributes are present, and
> > act_tunnel_key with a single action carrying a maximum-sized geneve
> > option blob, whose 63 class/type/data attribute triplets expand to about
> > 1.2KB. tca_get_fill() then fails and the request returns -EINVAL, but
> > tcf_action_init() has already committed the actions via
> > tcf_idr_insert_many(), so they stay installed even though userspace is
> > told the request failed.
>
> Please TAL at Clashiko review.

Yes, I belive it's correct - the code should acquire the lock in act_bpf.
Also yes, the GET path suffers from the same issue, I can send a fix
for it as a follow up.

cheers,
Victor

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net/sched: add get_fill_size callbacks for actions missing them
  2026-08-12 17:53   ` Victor Nogueira
@ 2026-08-13  0:46     ` Jakub Kicinski
  0 siblings, 0 replies; 4+ messages in thread
From: Jakub Kicinski @ 2026-08-13  0:46 UTC (permalink / raw)
  To: Victor Nogueira
  Cc: davem, edumazet, pabeni, jhs, jiri, daniel, john.fastabend, sdf,
	martin.lau, ast, andrii, eddyz87, memxor, song, jolsa, emil,
	horms, vega, netdev, bpf

On Wed, 12 Aug 2026 14:53:15 -0300 Victor Nogueira wrote:
> > On Mon, 10 Aug 2026 13:43:57 -0300 Victor Nogueira wrote:  
> > > When an RTM_NEWACTION request with NLM_F_ECHO (or an RTNLGRP_TC
> > > listener) creates several actions, tcf_add_notify_msg() allocates the
> > > echo skb from this underestimated size. act_bpf is the clearest case:
> > > TCA_ACT_BPF_OPS alone reaches 32KB per action (BPF_MAXINSNS), so two
> > > actions are enough to overrun the allocation. act_pedit overruns with
> > > 32 actions of four munge keys each,  
> >
> > "overruns" makes it sounds like we overflow a buffer.  
> 
> Fair enough, it doesn't actually overflow a buffer.
> I can change that.
> 
> > AFAICT we'd at worse hit a WARN somewhere due to EMSGSIZE?  
> 
> The issue is more subtle.
> The kernel might create the actions, but fail to notify userspace
> because the notification message is too large for the allocated skb.
> So it's not a crash or a WARN, but rather a correctness issue.
> 
> > > act_police with 32 policers once the
> > > optional rate/peakrate/result/avrate attributes are present, and
> > > act_tunnel_key with a single action carrying a maximum-sized geneve
> > > option blob, whose 63 class/type/data attribute triplets expand to about
> > > 1.2KB. tca_get_fill() then fails and the request returns -EINVAL, but
> > > tcf_action_init() has already committed the actions via
> > > tcf_idr_insert_many(), so they stay installed even though userspace is
> > > told the request failed.  
> >
> > Please TAL at Clashiko review.  
> 
> Yes, I belive it's correct - the code should acquire the lock in act_bpf.
> Also yes, the GET path suffers from the same issue, I can send a fix
> for it as a follow up.

to be clear - im expecting a respin with a much better commit msg

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-08-13  0:46 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-10 16:43 [PATCH net] net/sched: add get_fill_size callbacks for actions missing them Victor Nogueira
2026-08-12  1:19 ` Jakub Kicinski
2026-08-12 17:53   ` Victor Nogueira
2026-08-13  0:46     ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox