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

[Jamal Hadi Salim]

On Wed, Nov 4, 2026 at 3:10 AM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
>
> 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.

This is a bit of an overstatement (Sashiko is infecting me with its
pedancity); you should qualify it with "needs CAP_NET_ADMIN in a
userns" or something like that. In general you should provide for
conditions on how to reproduce this in the commit (looks like straight
tc commands).

>> 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@xxxxxxxxxxxxxxx
> > Cc: AutonomousCodeSecurity@xxxxxxxxxxxxx
> > Signed-off-by: Cen Zhang (Microsoft) <cenzhang@xxxxxxxxxxxxxxxxxxx>
> > ---
> >
> > 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.
> >

Good note - this fix is not safe in older kernels before bc50835e83f6.
Is this the protocol to signal backporters? My gut feel is they use
automated tools and would not be reading commit logs to backport?

> >   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);
> > -

Please leave the above lines alone; they seem like a separate
unnecessary "fix." Follow up with net-next to remove this hunk.
On current main it is dead code, so it does not help you fix this
graft issue and for older kernels the backporters wouldnt have to
worry about the dependency on bc50835e83f6

The rest looks reasonable.

cheers,
jamal

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