Re: [PATCH net] net: lan966x: release the in-flight slot of a stale TX skb

[Daniel Machon]

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 (1) · High: 0 (5) · Medium: 1 (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] After this patch, slots are reclaimed only by
>   lan966x_ptp_txtstamp_old_release(), which ages only the queue of the…
> 
> Pre-existing issues:
> - [Critical] This problem wasn't introduced by this patch, but when the
>   shared 512-entry FDMA TX ring is full, lan966x_fdma_xmit() returns…
> - [High] This problem wasn't introduced by this patch, but the
>   sparx5/lan969x driver has the same leak this patch fixes for lan966x.
> - [High] This problem wasn't introduced by this patch, but
>   lan966x_ptp_txtstamp_request() makes the skb visible on port->tx_skbs…
> - [High] This problem wasn't introduced by this patch, but when
>   lan966x_ptp_txtstamp_request() returns -EBUSY, lan966x_port_xmit()…
> - [High] This problem wasn't introduced by this patch, but in FDMA mode
>   lan966x_fdma_xmit() leaves a two-step skb DMA-mapped and referenced by…
> - [High] This problem wasn't introduced by this patch, but
>   lan966x_fdma_xmit() can free a two-step skb that is still on…
> - [Medium] This problem wasn't introduced by this patch, but
>   lan966x_ptp_txtstamp_old_release() frees two-step skbs after…
> 
> --
> 
> net: lan966x: release the in-flight slot of a stale TX skb
> 
> lan966x_ptp_txtstamp_old_release() now returns how many stale two-step TX
> skbs it freed. lan966x_ptp_txtstamp_request() subtracts that count from
> lan966x->ptp_skbs under ptp_ts_id_lock. Before this, a slot whose
> timestamp never arrived was never released, so two-step TX eventually
> failed on every port.
> 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
> > index 8c40db90ee8f..9be2cfe6feeb 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
> 
> [ ... ]
> 
> > @@ -389,14 +390,18 @@ static void lan966x_ptp_txtstamp_old_release(struct lan966x_port *port)
> >
> >               __skb_unlink(skb, &port->tx_skbs);
> >               dev_kfree_skb_any(skb);
> > +             purged++;
> >       }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can this free a two-step
> skb that the FDMA engine is still reading?
> 
> In FDMA mode, lan966x_fdma_xmit() leaves the skb DMA-mapped and
> referenced by the shared TX ring, and hands its lifetime to the PTP code:
> 
> drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c:lan966x_fdma_xmit() {
>     ...
>         next_dcb_buf->data.skb = skb;
>     ...
>         if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
>             LAN966X_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
>                 next_dcb_buf->ptp = true;
>     ...
> }
> 
> On completion, lan966x_fdma_tx_clear_buf() only unmaps it:
> 
>                         if (!dcb_buf->ptp)
>                                 napi_consume_skb(dcb_buf->data.skb, weight);
> 
> lan966x_ptp_txtstamp_old_release() frees by age alone.
> LAN966X_PTP_TIMEOUT is msecs_to_jiffies(10), which is a single jiffy at
> HZ=100. Nothing checks whether the FDMA descriptor has completed.
> 
> The PTP frame can sit behind other frames in the 512-entry ring that all
> ports share, or egress can be slow (10/100 link, PAUSE, shapers). In
> either case, the next two-step request on the same port can hand the skb
> data back to the allocator while the device can still DMA-read it.
> 
> Could the device then send freed or reused memory onto the wire? And
> would the late stamp then hit WARN_ON(!skb_match)?
> 
> [Severity: Medium]
> This is also a pre-existing issue, but because aging here is based only
> on time, what happens to a stamp that arrives after its skb has been
> aged out?
> 
> lan966x_ptp_irq_handler() matches FIFO entries only by the per-port
> 0..511 id:
> 
>                 skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
>                         if (LAN966X_SKB_CB(skb)->ts_id != id)
>                                 continue;
>                 ...
>                 if (WARN_ON(!skb_match))
>                         continue;
> 
> A late stamp either hits the WARN_ON(), or it gets attributed to a newer
> skb that reused the id, if the port's ts_id has wrapped in the meantime.
> 
> Can userspace then receive a wrong TX hardware timestamp, with the newer
> skb's own stamp tripping the WARN later? Frames held in egress past the
> timeout would trigger this, for example with taprio/tbf/cbs offload,
> PAUSE or a slow link.
> 
> ptp_skbs never prevented id reuse, so this patch does not change the
> behaviour.
> 
> >       spin_unlock_irqrestore(&port->tx_skbs.lock, flags);
> > +
> > +     return purged;
> >  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does sparx5/lan969x need
> the same fix? sparx5_ptp_txtstamp_old_release() in
> drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c matches the pre-patch
> lan966x version, and it never decrements sparx5->ptp_skbs:
> 
>         skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
>                 if time_after(SPARX5_SKB_CB(skb)->jiffies + SPARX5_PTP_TIMEOUT,
>                               jiffies)
>                         break;
> 
>                 __skb_unlink(skb, &port->tx_skbs);
>                 dev_kfree_skb_any(skb);
>         }
> 
> There are only two other decrements. The IRQ handlers in sparx5_ptp.c
> and lan969x.c decrement it for a matched skb, and
> sparx5_ptp_txtstamp_release() does so on the xmit busy path.
> 
> After SPARX5_MAX_PTP_ID (512) lost stamps,
> sparx5_ptp_txtstamp_request() rejects every two-step request:
> 
>         if (sparx5->ptp_skbs == SPARX5_MAX_PTP_ID) {
>                 spin_unlock_irqrestore(&sparx5->ptp_ts_id_lock, flags);
>                 return -EBUSY;
>         }
> 
> sparx5_port_xmit_impl() then returns NETDEV_TX_BUSY without stopping the
> queue:
> 
>                 if (sparx5_ptp_txtstamp_request(port, skb) < 0)
>                         return NETDEV_TX_BUSY;
> 
> Wouldn't the qdisc then requeue and retry the same PTP skb forever, and
> block all traffic behind it on that TX queue?
> 
> A companion patch for sparx5 seems needed. It would return the purged
> count and subtract it under ptp_ts_id_lock.
> 
> [ ... ]
> 
> > @@ -408,9 +413,10 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
> >       if (rew_op != IFH_REW_OP_TWO_STEP_PTP)
> >               return 0;
> >
> > -     lan966x_ptp_txtstamp_old_release(port);
> > +     purged = lan966x_ptp_txtstamp_old_release(port);
> >
> >       spin_lock_irqsave(&lan966x->ptp_ts_id_lock, flags);
> > +     lan966x->ptp_skbs -= purged;
> >       if (lan966x->ptp_skbs == LAN966X_MAX_PTP_ID) {
> 
> [Severity: Medium]
> The ptp_skbs limit is switch-wide. Slots are only reclaimed by aging the
> transmitting port's queue, and only when that port sends a two-step
> frame. Can stale skbs on one port still use up the limit for every other
> port?

Yes, but this is a separate, pre-existing limitation, and the commit message
even says so.

> 
> The commit message says idle ports keep their stale skbs. Nothing else
> releases them either. lan966x_port_stop() does not purge port->tx_skbs,
> and HWTSTAMP_TX_OFF in lan966x_ptp_hwtstamp_set() only does:
> 
>         case HWTSTAMP_TX_OFF:
>                 port->ptp_tx_cmd = IFH_REW_OP_NOOP;
>                 break;
> 
> After that the port can never send a two-step frame again, so its stale
> skbs are never aged. Only lan966x_ptp_deinit() purges tx_skbs.
> 
> Lost stamps can pile up on a port that then goes quiet: ifdown, TX
> timestamping turned off, ptp4l stopped, or a boundary clock port in
> passive state. Two-step TX on the other ports is then throttled, and
> returns -EBUSY once the total reaches LAN966X_MAX_PTP_ID. That is the
> failure this patch sets out to fix.
> 
> Would any of these close the gap: aging all ports before rejecting,
> aging from a timer or worker, or purging and uncounting on ndo_stop and
> HWTSTAMP_TX_OFF?

The bug fixed here is that every lost timestamp leaked its slot for good, so the
limit was eventually hit on any system that lost timestamps.  Aging the queues
of all ports on each request, so an idle port cannot hold slots, I believe is a
change for net-next.

> 
> >               spin_unlock_irqrestore(&lan966x->ptp_ts_id_lock, flags);
> >               return -EBUSY;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this -EBUSY path
> leak the skb? lan966x_port_xmit() returns the errno directly as its
> netdev_tx_t:
> 
>                 err = lan966x_ptp_txtstamp_request(port, skb);
>                 if (err)
>                         return err;
> 
> dev_xmit_complete() treats any rc < NET_XMIT_MASK, negative values
> included, as meaning the driver consumed the skb. The core therefore
> neither requeues nor frees it. The skb is not on port->tx_skbs either,
> because this return happens before skb_queue_tail().
> 
> So every two-step frame sent while the slot table is full would leak an
> skb, and its sk_wmem_alloc charge would keep the sending socket pinned.
> 
> Once an admin has enabled HW TX timestamping on a port, could an
> unprivileged user fill the table and keep leaking memory? Sending UDP to
> port 319 with SOF_TIMESTAMPING_TX_HARDWARE would be enough.
> 
> [Severity: High]
> This is a pre-existing issue, but further down in
> lan966x_ptp_txtstamp_request() the skb is published on port->tx_skbs
> before its control block is filled in:
> 
>         skb_queue_tail(&port->tx_skbs, skb);
>         LAN966X_SKB_CB(skb)->ts_id = port->ts_id;
>         LAN966X_SKB_CB(skb)->jiffies = jiffies;
> 
> Only ptp_ts_id_lock covers these writes. lan966x_ptp_irq_handler() walks
> the list holding only tx_skbs.lock and matches on
> LAN966X_SKB_CB(skb)->ts_id.
> 
> On PREEMPT_RT, ptp_ts_id_lock is a sleeping lock and the PTP handler is
> a threaded IRQ (devm_request_threaded_irq() in lan966x_probe()). The
> handler can therefore preempt the sender right after skb_queue_tail().
> 
> Suppose the FIFO holds an unmatched stamp, for example a late one for an
> aged-out skb, whose id equals the leftover ts_id bytes in the cb. The
> handler then unlinks the new skb, and once request() drops the lock it
> timestamps and frees it.
> 
> Wouldn't lan966x_port_xmit() then keep reading the cb and transmit the
> freed skb? Setting ts_id and jiffies before skb_queue_tail() would avoid
> this.
> 
> [Severity: High]
> This is another pre-existing issue. After request() has queued and
> counted the skb, lan966x_fdma_xmit() can free it without unlinking it
> from port->tx_skbs:
> 
> drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c:lan966x_fdma_xmit() {
>     ...
>         if (skb_put_padto(skb, ETH_ZLEN)) {
>                 dev->stats.tx_dropped++;
>                 return NETDEV_TX_OK;
>         }
>     ...
> }
> 
> skb_put_padto() passes free_on_error=true, so __skb_pad() frees the skb
> when pskb_expand_head(GFP_ATOMIC) or skb_linearize() fails. This return
> also skips the lan966x_ptp_txtstamp_release() call that the release:
> label makes.
> 
> L2 PTP event messages are 58 bytes, which is below ETH_ZLEN. They are
> cloned when an AF_PACKET tap is active, and that forces
> pskb_expand_head().
> 
> Under memory pressure, could a later old_release() or IRQ handler walk
> read the freed skb, __skb_unlink() it, and call dev_kfree_skb_any() on
> it a second time?
> 
> [Severity: Critical]
> This isn't introduced by this patch either, but the ring-full path in
> lan966x_fdma_xmit() also returns without undoing the reservation made
> here:
> 
>         next_to_use = lan966x_fdma_get_next_dcb(tx);
>         if (next_to_use < 0) {
>                 netif_stop_queue(dev);
>                 return NETDEV_TX_BUSY;
>         }
> 
> The register injection path does undo it, in lan966x_port_ifh_xmit():
> 
> err:
>         if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
>             LAN966X_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
>                 lan966x_ptp_txtstamp_release(port, skb);
> 
>         return NETDEV_TX_BUSY;
> 
> On NETDEV_TX_BUSY, dev_hard_start_xmit() overwrites skb->next, and
> dev_requeue_skb() links the skb into q->gso_skb. At that point the skb
> is still linked on port->tx_skbs and still counted in ptp_skbs.
> 
> Can this corrupt port->tx_skbs? A later old_release() or IRQ walk could
> follow the overwritten pointers, or free an skb the stack still owns.
> 
> When the qdisc retries, request() calls skb_queue_tail() on the same skb
> again. If that skb was the tail, skb->next ends up pointing to itself,
> and skb_queue_walk_safe() in lan966x_ptp_irq_handler() would loop
> forever with tx_skbs.lock held.
> 
> The 512-entry ring is shared by all ports, and netif_stop_queue() only
> stops TX queue 0 of the current netdev. Ordinary TX congestion plus
> two-step traffic on another port or queue looks like enough to reach
> this.
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026-lan966x-ptp-stale-tx-slot-v1-1-9f7c895eb1ff%40microchip.com