Re: sch_qfq: race conditon on qfq_aggregate (net/sched/sch_qfq.c)

[Cong Wang <xiyou.wangcong@gmail.com>]

On Wed, Jul 02, 2025 at 12:41:36PM -0700, Xiang Mei wrote:
> On Mon, Jun 30, 2025 at 4:09 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > Hi Xiang,
> >
> > On Mon, Jun 30, 2025 at 11:49:02AM -0700, Xiang Mei wrote:
> > > Thank you very much for your time. We've re-tested the PoC and
> > > confirmed it works on the latest kernels (6.12.35, 6.6.95, and
> > > 6.16-rc4).
> > >
> > > To help with reproduction, here are a few notes that might be useful:
> > > 1. The QFQ scheduler needs to be compiled into the kernel:
> > >     $ scripts/config --enable CONFIG_NET_SCHED
> > >     $ scripts/config --enable CONFIG_NET_SCH_QFQ
> > > 2. Since this is a race condition, the test environment should have at
> > > least two cores (e.g., -smp cores=2 for QEMU).
> > > 3. The PoC was compiled using: `gcc ./poc.c -o ./poc  -w --static`
> > > 4. Before running the PoC, please check that the network interface
> > > "lo" is in the "up" state.
> > >
> > > Appreciate your feedback and patience.
> >
> > Thanks for your detailed report and efforts on reproducing it on the
> > latest kernel.
> >
> > I think we may have a bigger problem here, the sch_tree_lock() is to lock
> > the datapath, I doubt we really need to use sch_tree_lock() for
> > qfq->agg. _If_ it is only for control path, using RTNL lock + RCU lock
> > should be sufficient. We need a deeper review on the locking there.
> 
> My experience focused on vulnerability exploitation, and I am very new
> to RCU. I have some questions about the possible RCU solution to this
> vulnerability:
> 
> qfq->agg is used in both data path (qfq_change_agg was called in
> qfq_enqueue) and control path, which is not protected by RTNL lock.
> Does that mean we should use rcu_dereference_bh instead of
> rcu_dereference_rtnl or rcu_dereference?

Good finding! I think this is probably the reason why we are using
sch_tree_lock().

I have to say updating agg in enqueue() looks weird and anti-pattern,
but changing this requires more efforts, so we may have to defer it to
long term.

So, if we have to take sch_tree_lock(), what does your final patch look
like?

Thanks.