Re: [PATCH] net: mvpp2: free pending TX timestamp skbs when the port is stopped

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 0 (1)

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

- [High] The new mvpp2_tx_hwtstamp_flush()
  (drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3422-3436) checks,…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a
  serious race on the same data.
- [Low] Baseline decision: not a valid finding against this patch.

--

net: mvpp2: free pending TX timestamp skbs when the port is stopped

This commit adds mvpp2_tx_hwtstamp_flush(). It frees any skbs still held
in port->tx_hwtstamp_queue[] that are waiting for a hardware TX timestamp.
It is called from mvpp2_stop() after the port interrupts have been
released, so the extra references taken by mvpp2_tx_hw_tstamp() are not
leaked.

> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 317af54369cfe..fc020b8c88301 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -3419,6 +3419,22 @@ static void mvpp2_isr_handle_ptp(struct mvpp2_port *port)
>  		mvpp2_isr_handle_ptp_queue(port, 1);
>  }
>  
> +static void mvpp2_tx_hwtstamp_flush(struct mvpp2_port *port)
> +{
> +	struct mvpp2_hwtstamp_queue *queue;
> +	int nq, i;
> +
> +	for (nq = 0; nq < ARRAY_SIZE(port->tx_hwtstamp_queue); nq++) {
> +		queue = &port->tx_hwtstamp_queue[nq];
> +		for (i = 0; i < ARRAY_SIZE(queue->skb); i++) {
> +			if (!queue->skb[i])
> +				continue;
> +			dev_kfree_skb_any(queue->skb[i]);
> +			queue->skb[i] = NULL;

[Severity: High]
Can this race with mvpp2_tx_hw_tstamp() on another CPU and free the same
skb twice?

mvpp2_stop() is not only reached through ndo_stop, where
__dev_close_many() has already quiesced TX. It is also called directly
on running devices:

mvpp2_xdp_setup():
	if (running && reset)
		mvpp2_stop(port->dev);

mvpp2_change_mtu()->mvpp2_bm_switch_buffers():
	for (i = 0; i < priv->port_count; i++) {
		port = priv->port_list[i];
		status[i] = netif_running(port->dev);
		if (status[i])
			mvpp2_stop(port->dev);
	}

The second path stops every running port on the controller, including
ports whose MTU did not change.

mvpp2_stop_dev() does set_bit(0, &port->state), which mvpp2_tx() never
checks. It also disables interrupts and NAPI and calls phylink_stop().

The only TX gating is netif_tx_stop_all_queues() in mvpp2_mac_link_down().
It does not wait for an mvpp2_tx() that is already running on another
CPU. On ports without phylink, the TX queues are not stopped at all.

Meanwhile the xmit path works on the same array:

mvpp2_tx_hw_tstamp():
	i = queue->next;
	queue->next = (i + 1) & 31;
	if (queue->skb[i])
		dev_kfree_skb_any(queue->skb[i]);
	queue->skb[i] = skb_get(skb);

Suppose the flush and an in-flight xmit both see the same skb in skb[i].
Both would then call dev_kfree_skb_any() on it. TX completion has already
dropped the stack's reference, so that skb only holds the skb_get()
reference.

On phylink ports the window is limited to an xmit that started before
netif_tx_stop_all_queues(). That window is not bounded on PREEMPT_RT.
mvpp2_cleanup_txqs() already has the same missing TX quiescence on these
paths, but the flush adds a new unsynchronized writer.

Should TX be quiesced before mvpp2_tx_hwtstamp_flush() runs on these
paths?

[Severity: High]
This isn't a bug introduced by this patch, but concurrent transmits also
do not appear to protect this per-port array.

port->tx_hwtstamp_queue[] and its next index are per-port. mvpp2 is not
LLTX, so the core holds only the per-txq lock. XPS maps each CPU to its
own txq, so mvpp2_tx() can run on several CPUs at once.

The only driver lock in mvpp2_tx() is conditional and per-thread:

	if (test_bit(thread, &port->priv->lock_map))
		spin_lock_irqsave(&port->tx_lock[thread], flags);

That lock does not serialize two threads in mvpp2_tx_hw_tstamp().

If two CPUs read the same queue->next value while skb[i] holds a pending
skb, could both call dev_kfree_skb_any() on it? If skb[i] is NULL, one
skb_get() reference would be overwritten, and that skb and its socket
write memory would be leaked.

The slot-reuse free in mvpp2_tx_hw_tstamp() also looks like it can race
with the consumer in mvpp2_isr_handle_ptp_queue():

	skb = queue->skb[id];
	queue->skb[id] = NULL;
	if (skb) {

Once TX HW timestamping is enabled on the port, a process without any
capability can set SO_TIMESTAMPING with SOF_TIMESTAMPING_TX_HARDWARE and
send PTP frames from several CPUs.

The new flush adds another unlocked accessor to this array.

> +		}
> +	}
> +}
> +

[ ... ]

> @@ -4914,6 +4930,7 @@ static int mvpp2_stop(struct net_device *dev)
>  		free_irq(port->port_irq, port);
>  
>  	mvpp2_irqs_deinit(port);
> +	mvpp2_tx_hwtstamp_flush(port);

[Severity: Low]
This is a pre-existing issue, but the stop path does not touch the
hardware PTP TX result FIFOs (MVPP22_PTP_TX_Q0_R0..R2 and the Q1
equivalents). It also does not reset queue->next.

A stale record for a reused slot could be matched to a new skb after
reopen. That happens the same way with or without this patch. In
practice, mvpp2_open() normally drains latched PTP causes before TX
restarts.

The effect specific to this patch is smaller. Some timestamps have
already been captured by the hardware but not yet read when the port
stops. Those records now find a NULL slot in mvpp2_isr_handle_ptp_queue()
and are dropped, instead of being delivered to the original skb.

Would it make sense to drain both hardware queues with
mvpp2_isr_handle_ptp_queue() before freeing the remaining skbs here?

>  	if (!port->has_tx_irqs) {
>  		for (thread = 0; thread < port->priv->nthreads; thread++) {
>  			port_pcpu = per_cpu_ptr(port->pcpu, thread);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103100535.1310088-1-vulab%40iscas.ac.cn