Re: [PATCH net] net: gen_estimator: protect seqcount updates from hardirq readers

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message describes a bug that nothing in the tree
  can trigger, yet the patch carries Fixes: and Cc: stable.

--

net: gen_estimator: protect seqcount updates from hardirq readers

This commit replaces preempt_disable_nested() with local_irq_save() around
the est->seq write section in est_timer() in net/core/gen_estimator.c. The
aim is to stop a hardirq reader of gen_estimator_read() on the same CPU
from spinning on an odd sequence count while the timer is interrupted.

> est_timer updates the estimator seqcount with preemption disabled, which
> does not exclude hardirq readers on non-PREEMPT_RT kernels.  A reader can
> observe an odd sequence and spin while the timer is interrupted.

[ ... ]

> The hardirq context is deliberately injected to
> exercise this interleaving; ordinary userspace traffic does not guarantee
> it.
>
> Fixes: 1c0d32fde5bd ("net_sched: gen_estimator: complete rewrite of rate estimators")
> Cc: stable@vger.kernel.org

[Severity: Medium]
Is there an in-tree hardirq or NMI caller of gen_estimator_read()?

struct net_rate_estimator is private to net/core/gen_estimator.c. That
means gen_estimator_read() is the only reader of est->seq. Its callers
appear to be:

  gnet_stats_copy_rate_est() in net/core/gen_stats.c, reached from
  netlink dump paths in process context: tcf_action_copy_stats(),
  tc_fill_qdisc(), and the drr/hfsc/htb/qfq dump_class_stats callbacks.

  xt_rateest_mt() in net/netfilter/xt_rateest.c, which runs from
  netfilter hooks in softirq context or with BH disabled.

  tcf_police_act() in net/sched/act_police.c, which runs in the tc
  datapath in softirq context or under rcu_read_lock_bh().

Netpoll transmits with IRQs off, but it calls netdev_start_xmit()
directly. It never reaches qdiscs, tc actions or netfilter.

On !PREEMPT_RT, est_timer() runs from the timer softirq. If the writer
interrupts a process or BH reader, that reader just retries. On
PREEMPT_RT, preempt_disable_nested() already expanded to a real
preempt_disable().

The commit message also says the reproducer relies on a test-only
hardirq injection.

If no real hardirq reader exists, could this be presented as hardening,
with the Fixes: and Cc: stable tags dropped? If one does exist, could the
commit message name the hardirq path that reaches gen_estimator_read()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923032936.2020902-1-runyu.xiao%40seu.edu.cn