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

[Paolo Abeni]

On 11/6/26 07:41, Linkui Xiao wrote:
> 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")

As nothing really was broken, this and the next patch do not need a
fixes tag. I'll strip it when applying. Also next time please try
produce more terse changelogs.

/P