[PATCH net-next 0/3] net/sched: get_fill_size() budget fixes for act_gact and act_vlan

[PATCH net-next 0/3] net/sched: get_fill_size() budget fixes for act_gact and act_vlan

[Linkui Xiao]

The get_fill_size() cleanup began with commit 8e2efb3f45a5
("net/sched: add get_fill_size callbacks for actions missing them")
[1], which fixed the actions the bug was reproduced with, pointed at
act_gact/act_skbedit/act_vlan as the pattern to follow, and left the
other pre-existing issues to upcoming patches.  It continued with
commit 4ba61bfd4998 ("net/sched: add get_fill_size() to the five
actions that lack one"), which spelled out the rule the callbacks have
to obey: size every optional attribute unconditionally, because reading
the current flags to size it conditionally races a concurrent replace
that can flip the attributes between sizing and dumping.

Auditing the remaining callbacks against their dumps turned up two gaps
in the very pattern 8e2efb3f45a5 held up:

  - act_gact reads tcfg_ptype with neither tcfg_lock nor READ_ONCE() and
    budgets TCA_GACT_PROB only when that read happens to be non-zero
    (patch 1);

  - act_vlan never budgets the two PUSH_ETH addresses its dump emits,
    and only escapes overflowing because the three PUSH_VLAN_*
    attributes that a PUSH_ETH instance does not emit happen to add up
    to exactly the same 24 bytes (patch 2).

Neither can overflow today.  All four consumers of the value --
tcf_get_notify(), tcf_del_notify_msg(), tcf_add_notify_msg() and
tcf_reoffload_del_notify_msg() -- allocate at least
max(attr_size, NLMSG_GOODSIZE), and NLMSG_GOODSIZE is
SKB_WITH_OVERHEAD(PAGE_SIZE), so a batch has to carry enough actions
for attr_size to grow past roughly 3.7 KiB before a 12- or 24-byte
shortfall matters.  A replace cannot interleave between sizing and
dumping either: RTM_NEWACTION, RTM_DELACTION and RTM_GETACTION all take
rtnl, and the classifier path, which can run without rtnl, computes
attr_size in tcf_exts_validate_ex() and discards it.  Both patches are
therefore accounting-correctness fixes against the rule above; each
carries a Fixes: tag for whoever wants it in stable.

For completeness, act_ct, act_tunnel_key and act_pedit also size some
attributes conditionally.  They do it from a single rcu_dereference() of
a params pointer that replace swaps rather than mutates, so the budget
stays consistent with the snapshot it was computed from.  act_gact
instead reads tcfg_ptype, a field mutated in place on the action itself,
with no lock and no READ_ONCE() -- that is what patch 1 fixes.  act_bpf
sizes conditionally too, but takes tcf_lock to do it; patch 1 removes
the read instead, which needs no lock.

Patch 3 adds TDC coverage for the previously untested gact dump and
get paths.

Tested by building net/sched with CONFIG_GACT_PROB=y and W=1, which
produced no new warnings, and by running the tdc gact and vlan suites
(38 and 36 cases, including the three added by patch 3) against a
CONFIG_GACT_PROB=y kernel with this series applied; all passed.

[1] https://patch.msgid.link/20260816201327.2435335-1-victor@mojatatu.com

[PATCH net-next 1/3] net/sched: act_gact: size TCA_GACT_PROB unconditionally

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

tcf_gact_get_fill_size() reads tcfg_ptype with neither tcfg_lock nor
READ_ONCE(), and budgets TCA_GACT_PROB only when that read happens to be
non-zero.  Both halves are wrong.

The bare read is a data race.  tcf_gact_init() writes tcfg_ptype under
spin_lock_bh(&gact->tcf_lock) when an action is replaced,
tcf_gact_dump() re-reads it under the same lock, and tcf_gact_act()
reads it with READ_ONCE().  get_fill_size() is the only accessor of the
field with neither.  act_bpf hit the same problem and solved it by
taking tcf_lock in tcf_bpf_get_fill_size(), with a comment explaining
why.

The conditional budget breaks the rule that commit 4ba61bfd4998
("net/sched: add get_fill_size() to the five actions that lack one")
spells out: "Size every optional attribute
unconditionally: reading the current flags to size it conditionally
would race a concurrent replace, which can flip the attributes between
sizing and dumping."  A replace that turns tcfg_ptype from zero into
non-zero between the two calls leaves tcf_gact_dump() writing a
TCA_GACT_PROB attribute -- nla_total_size(sizeof(struct tc_gact_p)) ==
12 bytes -- that was never budgeted.

Size TCA_GACT_PROB unconditionally.  The budget becomes an upper bound
again and the bare read goes away, so unlike act_bpf this needs no lock.

Nothing overflows today, so this is locking hygiene plus conformance to
the rule above rather than a live bug.  All four consumers of the value
-- tcf_get_notify(), tcf_del_notify_msg(), tcf_add_notify_msg() and
tcf_reoffload_del_notify_msg() -- allocate at least
max(attr_size, NLMSG_GOODSIZE), and the delete and add paths add
tcf_action_warn_attr_size(extack) on top.  NLMSG_GOODSIZE is
SKB_WITH_OVERHEAD(PAGE_SIZE), so a batch has to carry enough actions for
attr_size to grow past roughly 3.7 KiB before a 12-byte shortfall
matters -- which is the failure mode that commit 8e2efb3f45a5
("net/sched: add get_fill_size callbacks for actions missing them")
fixed for other actions.  A replace cannot interleave between sizing and
dumping either: RTM_NEWACTION, RTM_DELACTION and RTM_GETACTION all take
rtnl, and the classifier path, which can run without rtnl, computes
attr_size in tcf_exts_validate_ex() and discards it.

Tested by building with CONFIG_GACT_PROB=y so that the code being
changed is actually compiled; net/sched builds with W=1 and no new
warnings.  Patch 3 adds TDC cases for the gact dump and get paths.

Fixes: 9c5c9c573700 ("net sched actions: implement get_fill_size routine in act_gact")
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 net/sched/act_gact.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/sched/act_gact.c b/net/sched/act_gact.c
index 565860cccba6..ec9a65dfc442 100644
--- a/net/sched/act_gact.c
+++ b/net/sched/act_gact.c
@@ -233,9 +233,10 @@ static size_t tcf_gact_get_fill_size(const struct tc_action *act)
 	size_t sz = nla_total_size(sizeof(struct tc_gact)); /* TCA_GACT_PARMS */
 
 #ifdef CONFIG_GACT_PROB
-	if (to_gact(act)->tcfg_ptype)
-		/* TCA_GACT_PROB */
-		sz += nla_total_size(sizeof(struct tc_gact_p));
+	/* tcfg_ptype may be set by a concurrent replace after this budget has
+	 * been computed, so account for TCA_GACT_PROB unconditionally.
+	 */
+	sz += nla_total_size(sizeof(struct tc_gact_p));
 #endif
 
 	return sz;
-- 
2.25.1

[PATCH net-next 2/3] net/sched: act_vlan: size push_eth in get_fill_size

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

tcf_vlan_dump() emits TCA_VLAN_PUSH_ETH_DST and TCA_VLAN_PUSH_ETH_SRC for
every action installed with TCA_VLAN_ACT_PUSH_ETH, but
tcf_vlan_get_fill_size() budgets only TCA_VLAN_PARMS plus the push VLAN
id, protocol and priority.  Commit 19fbcb36a39e ("net/sched: act_vlan:
Add {POP,PUSH}_ETH actions") put the two Ethernet addresses in the dump
without putting them here.

get_fill_size() is the per-action term of the budget that
tcf_action_fill_size() returns to the core, which uses it to size the
netlink skb carrying the action, so it has to bound what dump() can write
rather than describe one particular instance.  Commit 4ba61bfd4998
("net/sched: add get_fill_size() to the five actions that lack one")
states the convention: "Size every optional attribute unconditionally".
act_vlan already follows it for TCA_VLAN_PUSH_VLAN_ID,
TCA_VLAN_PUSH_VLAN_PROTOCOL and TCA_VLAN_PUSH_VLAN_PRIORITY, all of which
are sized even though a PUSH_ETH instance emits none of them; the
push_eth attributes are simply missing.

This does not overflow today, and only by coincidence.  A PUSH_ETH dump
writes nla_total_size(sizeof(struct tc_vlan)) + 2 *
nla_total_size(ETH_ALEN) == 28 + 24 == 52 bytes, and the current budget
is also exactly 52 bytes, because the three PUSH_VLAN_* attributes that
a PUSH_ETH instance never emits happen to add up to the same 24 bytes the
two MAC addresses need.  That leaves zero slack, and nothing keeps it
that way: the moment any PUSH_VLAN_* attribute is made conditional --
precisely the change the convention quoted above forbids -- a push_eth
dump no longer fits the budget, and building the notification fails with
-EMSGSIZE.

Account for the two attributes the way the rest of the function already
does.  Nothing changes at runtime: the budget already equals or exceeds
what dump() writes for every instance, and all four consumers of it --
tcf_get_notify(), tcf_del_notify_msg(), tcf_add_notify_msg() and
tcf_reoffload_del_notify_msg() -- allocate at least
max(attr_size, NLMSG_GOODSIZE), which is SKB_WITH_OVERHEAD(PAGE_SIZE)
and dwarfs a 24-byte difference.  The RTM_GETACTION dump path sizes its
skb from the netlink dump machinery instead.

Tested by building with CONFIG_NET_ACT_VLAN=y; net/sched builds with W=1
and no new warnings.

Fixes: 19fbcb36a39e ("net/sched: act_vlan: Add {POP,PUSH}_ETH actions")
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 net/sched/act_vlan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 6a375fdc63dd..e666004bb919 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -348,7 +348,8 @@ static size_t tcf_vlan_get_fill_size(const struct tc_action *act)
 	return nla_total_size(sizeof(struct tc_vlan))
 		+ nla_total_size(sizeof(u16)) /* TCA_VLAN_PUSH_VLAN_ID */
 		+ nla_total_size(sizeof(u16)) /* TCA_VLAN_PUSH_VLAN_PROTOCOL */
-		+ nla_total_size(sizeof(u8)); /* TCA_VLAN_PUSH_VLAN_PRIORITY */
+		+ nla_total_size(sizeof(u8)) /* TCA_VLAN_PUSH_VLAN_PRIORITY */
+		+ 2 * nla_total_size(ETH_ALEN); /* TCA_VLAN_PUSH_ETH_{DST,SRC} */
 }
 
 static int tcf_vlan_offload_act_setup(struct tc_action *act, void *entry_data,
-- 
2.25.1

[PATCH net-next 3/3] selftests: tc-testing: add gact probability dump tests

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

gact probability attributes are emitted by tcf_gact_dump() only when
tcfg_ptype is set, and budgeted for by tcf_gact_get_fill_size().  The
only gact cases that look at a probability today are 8e47 and ca89, and
both check it through `tc actions list`, which is served by the netlink
dump machinery and never consumes the get_fill_size() budget.  The one
case that uses `tc actions get`, 525f, runs against an action with no
probability, so the RTM_GETACTION reply path -- tcf_get_notify(), which
does size its skb from get_fill_size() -- is never exercised with a
probability, and the netrand random type is not exercised anywhere.

Add three cases: adding an action with a netrand probability and getting
it back; replacing a probability-less action with a deterministic one
and getting it back, covering the tcfg_ptype 0 -> non-zero transition
that the unconditional TCA_GACT_PROB budget in patch 1 has to cover; and
getting a probability-less action, whose dump must report no probability.

These are coverage for the dump and sizing paths that nothing tested
before, not reproducers: RTM_NEWACTION, RTM_DELACTION and RTM_GETACTION
all take
rtnl, so sizing and dumping always run in the same critical section
and a replace cannot interleave between them from userspace.  They pass
both before and after patches 1 and 2.

Verified by running the tdc gact suite (38 cases, including these
three) against a kernel with CONFIG_GACT_PROB=y and this series
applied; all passed.

Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 tools/testing/selftests/tc-testing/tc-tests/actions/gact.json | 83 +++++++++++
 1 file changed, 83 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/gact.json b/tools/testing/selftests/tc-testing/tc-tests/actions/gact.json
index 0fcd52742939..94f032aeab68 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/actions/gact.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/actions/gact.json
@@ -761,5 +761,88 @@
                 1
             ]
         ]
+    },
+    {
+        "id": "b4f8",
+        "name": "Add gact action with random netrand and get it",
+        "category": [
+            "actions",
+            "gact"
+        ],
+        "plugins": {
+           "requires": "nsPlugin"
+        },
+        "setup": [
+            [
+                "$TC actions flush action gact",
+                0,
+                1,
+                255
+            ]
+        ],
+        "cmdUnderTest": "$TC actions add action pass random netrand drop 20 index 91",
+        "expExitCode": "0",
+        "verifyCmd": "$TC actions get action gact index 91",
+        "matchPattern": "action order [0-9]*: gact action pass.*random type netrand drop val 20.*index 91 ref",
+        "matchCount": "1",
+        "teardown": [
+            "$TC actions flush action gact"
+        ]
+    },
+    {
+        "id": "e3d7",
+        "name": "Replace gact action without probability with random determ and get it",
+        "category": [
+            "actions",
+            "gact"
+        ],
+        "plugins": {
+           "requires": "nsPlugin"
+        },
+        "setup": [
+            [
+                "$TC actions flush action gact",
+                0,
+                1,
+                255
+            ],
+            "$TC actions add action pass index 92"
+        ],
+        "cmdUnderTest": "$TC actions replace action pass random determ drop 5 index 92",
+        "expExitCode": "0",
+        "verifyCmd": "$TC actions get action gact index 92",
+        "matchPattern": "action order [0-9]*: gact action pass.*random type determ drop val 5.*index 92 ref",
+        "matchCount": "1",
+        "teardown": [
+            "$TC actions flush action gact"
+        ]
+    },
+    {
+        "id": "c2a9",
+        "name": "Get gact action without probability",
+        "category": [
+            "actions",
+            "gact"
+        ],
+        "plugins": {
+           "requires": "nsPlugin"
+        },
+        "setup": [
+            [
+                "$TC actions flush action gact",
+                0,
+                1,
+                255
+            ],
+            "$TC actions add action pass index 93"
+        ],
+        "cmdUnderTest": "$TC actions get action gact index 93",
+        "expExitCode": "0",
+        "verifyCmd": "$TC actions get action gact index 93",
+        "matchPattern": "action order [0-9]*: gact action pass.*random type none.*index 93 ref",
+        "matchCount": "1",
+        "teardown": [
+            "$TC actions flush action gact"
+        ]
     }
 ]
-- 
2.25.1