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

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@xxxxxxxxxx>

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@xxxxxxxxxx>
---
 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