Re: [PATCH net 1/4] net/sched: act_api: budget all shared attributes in notify skbs
From: Simon Horman <horms@kernel.org>
Date: 2026-08-27 08:45:00
On Mon, Aug 24, 2026 at 12:39:00PM -0300, Victor Nogueira wrote:
quoted hunk ↗ jump to hunk
tcf_action_shared_attrs_size() is supposed to return an upper bound on the netlink attributes every action dump emits outside of TCA_ACT_OPTIONS, so that tcf_add_notify_msg(), tcf_del_notify_msg() and friends can allocate an skb large enough for the reply. It has fallen behind the dump path and is now an underestimate for every single action. Attributes, such as, TCA_ACT_IN_HW_COUNT and TCA_STATS_BASIC_HW are emitted unconditionally and never accounted for. TCA_STATS_PKT64, TCA_ACT_USED_HW_STATS, TCA_STATS_RATE_EST, TCA_STATS_RATE_EST64 require specific conditions, but are also not accounted for. Fix the issue by budgeting all of them so that we have a legitimate upper bound. Even tough for of them require specific conditions, they are cheap so, to avoid overcomplicating, we opted to account for them unconditionally as well to account for a real worst case scenario. Fixes: 4e76e75d6aba ("net sched actions: calculate add/delete event message size") Reported-by: Sashiko <sashiko-bot@kernel.org> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810164357.1653956-1-victor%40mojatatu.com Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> Signed-off-by: Victor Nogueira <redacted> --- net/sched/act_api.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)diff --git a/net/sched/act_api.c b/net/sched/act_api.c index b4415d358c91..766162b0b810 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c@@ -443,12 +443,21 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act) + nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */ + cookie_len /* TCA_ACT_COOKIE */ + nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_HW_STATS */ + /* TCA_ACT_USED_HW_STATS */ + + nla_total_size(sizeof(struct nla_bitfield32)) + + nla_total_size(sizeof(u32)) /* TCA_ACT_IN_HW_COUNT */ + nla_total_size(0) /* TCA_ACT_STATS nested */ + nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_FLAGS */ /* TCA_STATS_BASIC */ + nla_total_size_64bit(sizeof(struct gnet_stats_basic)) - /* TCA_STATS_PKT64 */ - + nla_total_size_64bit(sizeof(u64)) + /* TCA_STATS_BASIC_HW */ + + nla_total_size_64bit(sizeof(struct gnet_stats_basic)) + /* TCA_STATS_PKT64, emitted by both of the basic copies above */ + + 2 * nla_total_size_64bit(sizeof(u64)) + /* TCA_STATS_RATE_EST */ + + nla_total_size_64bit(sizeof(struct gnet_stats_rate_est)) + /* TCA_STATS_RATE_EST64 */ + + nla_total_size_64bit(sizeof(struct gnet_stats_rate_est64)) /* TCA_STATS_QUEUE */ + nla_total_size_64bit(sizeof(struct gnet_stats_queue)) + nla_total_size(0) /* TCA_ACT_OPTIONS nested */
Hi Victor, As you are no doubt aware there is an AI-generated review of this patchset available at https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824153903.4143642-1-victor%40mojatatu.com And while I trust you will look over it, I did feel the following issue was worth raising here for you to comment on. I do see that it doesn't strictly effect this patch. But at the same time, perhaps it ought to be fixed, either as part of this patch or as a follow-up. Text of the AI-generated review: This isn't a bug introduced by this patch, the TCA_ACT_STATS line is unchanged, but while the surrounding terms are being made exact, should this container use nla_total_size_64bit(0)? It is opened through nla_put_64bit(): net/sched/act_api.c:tcf_action_copy_stats() { err = gnet_stats_start_copy(skb, TCA_ACT_STATS, &p->tcfa_lock, &d, TCA_ACT_PAD); } which reaches gnet_stats_start_copy_compat() -> gnet_stats_copy(d, type, NULL, 0, padattr), and on !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS a 4-byte TCA_ACT_PAD attribute can precede the nest. gen_stats.c even fixes up d->tail for exactly that case: net/core/gen_stats.c:gnet_stats_start_copy_compat() { int ret = gnet_stats_copy(d, type, NULL, 0, padattr); /* The initial attribute added in gnet_stats_copy() may be * preceded by a padding attribute, ... */ if (ret == 0 && d->tail->nla_type == padattr) } No overrun follows from this today: TCA_ACT_KIND is budgeted nla_total_size(IFNAMSIZ) = 20 bytes while the longest registered in-tree kind is "tunnel_key", emitted as 16, so every action carries at least 4 bytes of slack that absorbs the missing pad. It is only the term-by-term upper bound property that is lost.