Re: [PATCH net] net/sched: sch_teql: restore skb->dev on the slave failure path

On Mon, Aug 10, 2026 at 8:11 PM Jakub Kicinski <[email protected]> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net/sched: sch_teql: restore skb->dev on the slave failure path
>
> teql_master_xmit() used to leave skb->dev pointing at a slave after that
> slave's ndo_start_xmit() failed, so the skb could end up queued on a later
> slave's neighbour arp_queue while referring to the earlier slave.  The
> patch restores skb->dev to the master device at the end of each slave
> iteration.
>

Let's please apply this patch. The patch correctly fixes the reported
UAF (namespace-reachable, slave deleted while skb queued on another
slave's neighbour).

What sashikos are claiming is partly followup material (the rmmod
issue requires root where this UAF requires CAP_NET_ADMIN) and
partially hallucinations. So, let's address those issues in follow-up
patches.

cheers,
jamal

> > diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
> > index 5c42a29a981c..9e52afc2d980 100644
> > --- a/net/sched/sch_teql.c
> > +++ b/net/sched/sch_teql.c
> > @@ -377,6 +377,7 @@ static netdev_tx_t teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
> >                       nores = 1;
> >                       break;
> >               }
> > +             skb->dev = dev;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does it leave a smaller
> version of the same dangling skb->dev behind, with the master netdev in
> place of the previous slave?
>
> After the restore, skb->dev is the teql master, and the skb can still be
> parked on the arp_queue of a neighbour that belongs to a slave device.  On
> the second pass (skb_res == skb after the goto restart), teql_resolve() ->
> __teql_resolve() creates the neighbour on the slave because dst->dev is the
> master:
>
> net/sched/sch_teql.c:__teql_resolve() {
>         ...
>         if (dst->dev != dev) {
>                 struct neighbour *mn;
>
>                 mn = __neigh_lookup_errno(n->tbl, n->primary_key, dev);
>         ...
>         if (neigh_event_send(n, skb_res) == 0) {
>         ...
>         } else {
>                 err = (skb_res == NULL) ? -EAGAIN : 1;
>         }
> }
>
> __neigh_event_send() then keeps the skb:
>
> net/core/neighbour.c:__neigh_event_send() {
>         ...
>                 skb_dst_force(skb);
>                 __skb_queue_tail(&neigh->arp_queue, skb);
>         ...
> }
>
> teql_master_xmit() returns NETDEV_TX_OK for the err == 1 case and loses
> ownership.  At that point the queued skb's skb->dev (master) is not the
> neighbour's device (slave), so the arp_queue flushing done by
> arp_ifdown()/neigh_ifdown() for the master's device never sees this skb.
> skb->dev also takes no netdev reference.
>
> Is the held dst enough to pin the master here?  On NETDEV_UNREGISTER,
> rt_flush_dev() and dst_dev_put() swap dst->dev to blackhole_netdev via
> netdev_ref_replace(), which drops the master's refcount, so
> netdev_wait_allrefs() does not hold off free_netdev().  The master has no
> rtnl_link_ops, so the free comes from teql_exit():
>
> net/sched/sch_teql.c:teql_exit() {
>         ...
>                 unregister_qdisc(&master->qops);
>                 unregister_netdev(master->dev);
>                 free_netdev(master->dev);
>         ...
> }
>
> which means rmmod sch_teql after the last teql qdisc is deleted, racing a
> pending neighbour resolution (the retransmit window is multiple seconds by
> default).  The next use of the queued skb dereferences the freed master
> through the same path as the KASAN report quoted in the commit message,
> neigh_invalidate() -> arp_error_report() -> dst_link_failure() ->
> ipv4_link_failure() -> __icmp_send():
>
> net/ipv4/icmp.c:__icmp_send() {
>         ...
>         if (!(skb_in->dev && (skb_in->dev->flags&IFF_LOOPBACK)) &&
>               !icmpv4_global_allow(net, type, code, &apply_ratelimit))
>         ...
> }
>
> and on the successful resolution path,
> neigh_update_process_arp_queue() -> n1->output() -> dev_queue_xmit() uses
> the freed device's netdev_ops and _tx.
>
> Would setting skb->dev to the device of the neighbour that actually takes
> the skb (the slave used by __teql_resolve()), or declining to hand the skb
> to a neighbour whose device differs from skb->dev, close the remaining
> window?
>
> > \t\t__skb_pull(skb, skb_network_offset(skb));
> >       } while ((q = rcu_dereference(NEXT_SLAVE(q))) != start);
> >
> --
> pw-bot: cr