Re: [PATCH net] net/sched: fix rate estimator use-after-free when clearing NOLOCK

[Paolo Abeni <pabeni@redhat.com>]

On 10/30/26 04:04, Cen Zhang (Microsoft) wrote:
> qdisc_create() creates the rate estimator requested by TCA_RATE before
> qdisc_graft() converts a lockless child of a locking parent. The
> conversion in qdisc_clear_nolock() frees the per-CPU statistics, but
> the estimator keeps its own copy of the cpu_bstats pointer and its
> timer keeps reading the freed per-CPU memory on every estimator tick.
> 
> An unprivileged local user can trigger this. The estimator timer can
> access freed per-CPU statistics in softirq context and panic the kernel:
> 
>    BUG: unable to handle page fault for address: ff9d8b20ffa47920
>    RIP: 0010:gnet_stats_add_basic+0x77/0xc0
>    Call Trace:
>     <IRQ>
>     est_fetch_counters+0x31/0x50
>     est_timer+0x52/0x170
>     call_timer_fn+0x9f/0x210
>     run_timer_softirq+0x6e/0xb0
>     </IRQ>
>    Kernel panic - not syncing: Fatal exception in interrupt
> 
> Fix by doing the NOLOCK conversion in qdisc_create(), right before
> the estimator is created, instead of in qdisc_graft(), so that the
> estimator captures the final statistics pointer. Removing the
> conversion from qdisc_graft() is safe: a new qdisc is created by
> qdisc_create() with the parent it is grafted under, and since
> commit bc50835e83f6 ("net: sched: Disallow replacing of child qdisc
> from one parent to another") an existing qdisc can only be grafted
> again under its original parent. Either way the qdisc was already
> converted for that parent at creation.
> 
> Fixes: 8a53e616de29 ("net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS, too")
> Cc: stable@vger.kernel.org
> Cc: AutonomousCodeSecurity@microsoft.com
> Signed-off-by: Cen Zhang (Microsoft) <cenzhang@linux.microsoft.com>
> ---
> 
> The bug was found and this fix was developed with the assistance of an
> LLM-based code analysis tool.
> 
> Backport note: trees without commit bc50835e83f6 (v6.14) still allow
> regrafting an existing child qdisc under a different parent and need
> an additional fix for that path, for example cherry-picking that
> commit as well.
> 
>   net/sched/sch_api.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index f36c41224bcc..d89f3f0a3c5f 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -1247,10 +1247,6 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>   		unsigned long cl;
>   		int err;
>   
> -		/* Only support running class lockless if parent is lockless */
> -		if (new && (new->flags & TCQ_F_NOLOCK) && !(parent->flags & TCQ_F_NOLOCK))
> -			qdisc_clear_nolock(new);
> -
>   		if (!cops || !cops->graft)
>   			return -EOPNOTSUPP;
>   
> @@ -1327,6 +1323,7 @@ static int qdisc_block_indexes_set(struct Qdisc *sch, struct nlattr **tca,
>   
>   static struct Qdisc *qdisc_create(struct net_device *dev,
>   				  struct netdev_queue *dev_queue,
> +				  struct Qdisc *parent_qdisc,
>   				  u32 parent, u32 handle,
>   				  struct nlattr **tca, int *errp,
>   				  struct netlink_ext_ack *extack)
> @@ -1405,6 +1402,11 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
>   			goto err_out4;
>   	}
>   
> +	/* Only support running class lockless if parent is lockless */
> +	if ((sch->flags & TCQ_F_NOLOCK) && parent_qdisc &&
> +	    !(parent_qdisc->flags & TCQ_F_NOLOCK))
> +		qdisc_clear_nolock(sch);
> +
>   	if (tca[TCA_RATE]) {
>   		err = -EOPNOTSUPP;
>   		if (sch->flags & TCQ_F_MQROOT) {
> @@ -1817,7 +1819,7 @@ static int __tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
>   create_n_graft2:
>   	if (clid == TC_H_INGRESS) {
>   		if (dev_ingress_queue(dev)) {
> -			q = qdisc_create(dev, dev_ingress_queue(dev),
> +			q = qdisc_create(dev, dev_ingress_queue(dev), NULL,
>   					 tcm->tcm_parent, tcm->tcm_parent,
>   					 tca, &err, extack);
>   		} else {
> @@ -1834,7 +1836,7 @@ static int __tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n,
>   		else
>   			dev_queue = netdev_get_tx_queue(dev, 0);
>   
> -		q = qdisc_create(dev, dev_queue,
> +		q = qdisc_create(dev, dev_queue, p,
>   				 tcm->tcm_parent, tcm->tcm_handle,
>   				 tca, &err, extack);
>   	}

Jamal: could you please have a look?

Side note: I'm not sure if you are aware but we process the pending patches
in this order:

https://netdev.bots.linux.dev/suie.html?delegate=netdev

And you can restrict the list to the tc subsystem selecting the appropriate
filter with the funnel icon in the top right.

/P