Re: [PATCH net] net: ibm: emac: fix TX skb double free in emac_full_tx_reset()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] This patch fixes only one of the two emac_clean_tx_ring()
  callers, so the same TX skb double free can still happen in…

--

net: ibm: emac: fix TX skb double free in emac_full_tx_reset()

This patch takes netif_tx_lock_bh() around emac_clean_tx_ring() and the
tx_cnt/tx_slot/ack_slot reset in emac_full_tx_reset(). This keeps
emac_poll_tx() from freeing skbs a second time after the ring has been
cleaned. mal_poll() still calls emac_poll_tx() while polling of this EMAC
is disabled.

> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
> index 640618c0a09e9..377755af835cc 100644
> --- a/drivers/net/ethernet/ibm/emac/core.c
> +++ b/drivers/net/ethernet/ibm/emac/core.c
> @@ -745,8 +745,15 @@ static void emac_full_tx_reset(struct emac_instance *dev)
>  
>  	emac_tx_disable(dev);
>  	mal_disable_tx_channel(dev->mal, dev->mal_tx_chan);
> +
> +	/* MAL NAPI runs emac_poll_tx() even while polling of this EMAC is
> +	 * disabled. A cleaned descriptor reads as completed, so clean the
> +	 * ring under the TX lock or emac_poll_tx() frees the skbs again.
> +	 */
> +	netif_tx_lock_bh(dev->ndev);
>  	emac_clean_tx_ring(dev);
>  	dev->tx_cnt = dev->tx_slot = dev->ack_slot = 0;
> +	netif_tx_unlock_bh(dev->ndev);

[Severity: High]
This covers emac_full_tx_reset(). Can the same TX skb double free still
happen through the other caller of emac_clean_tx_ring()? That caller is
emac_close() in drivers/net/ethernet/ibm/emac/core.c, and it still cleans
the ring without the TX lock:

drivers/net/ethernet/ibm/emac/core.c:emac_close() {
    ...
	mal_disable_tx_channel(dev->mal, dev->mal_tx_chan);
	mal_poll_del(dev->mal, &dev->commac);

	emac_clean_tx_ring(dev);
	emac_clean_rx_ring(dev);
    ...
}

On this path, the only NAPI synchronization is one napi_synchronize() call
in mal_poll_disable(), reached through emac_netif_stop(). The MAL NAPI
instance is shared, so mal_poll() can be scheduled again after that. It
could be triggered by:

  - TX EOB interrupts raised while emac_tx_disable() waits for in-flight
    frames
  - another EMAC on the same MAL
  - mal_poll_enable()
  - the rotting-packet path

mal_poll() then calls poll_tx for every commac on the list. It does this
without mal->lock and without checking MAL_COMMAC_POLL_DISABLED:

drivers/net/ethernet/ibm/emac/mal.c:mal_poll() {
    ...
	list_for_each(l, &mal->poll_list) {
		struct mal_commac *mc =
			list_entry(l, struct mal_commac, poll_list);
		mc->ops->poll_tx(mc->dev);
	}
    ...
}

mal_poll_del() only does a list_del() under mal->lock, and mal_poll()
never takes that lock. Nothing waits for an in-progress poll afterwards.

On SMP, this interleaving looks possible. One example is the dual-CPU
476GTR described in akebono.dts with an ibm,emac4sync EMAC:

CPU1 (emac_close)                  CPU2 (mal_poll()->emac_poll_tx())
mal_poll_del()
                                   netif_tx_lock_bh(dev->ndev);
                                   ctrl = dev->tx_desc[slot].ctrl;
                                   /* READY clear */
                                   skb = dev->tx_skb[slot];
emac_clean_tx_ring()
  dev_kfree_skb(dev->tx_skb[i]);
  dev->tx_skb[i] = NULL;
  dev->tx_desc[i].ctrl = 0;
                                   dev_kfree_skb(skb);

This is the same "cleaned descriptor reads as completed" double free that
the commit message and the new comment describe. Neither the commit message
nor the new comment mentions the close path.

There is also a related issue at the same spot. list_del() in
mal_poll_del() poisons commac->poll_list.next, and a concurrent
list_for_each() in mal_poll() may still follow that pointer.

Would it make sense to also take netif_tx_lock_bh() around
emac_clean_tx_ring() in emac_close()? Another option is to call
napi_synchronize() after the list_del() in mal_poll_del(). The second
option would also stop mal_poll() from following the poisoned pointer.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105231516.120321-1-rosenp%40gmail.com