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

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

[Rosen Penev]

emac_full_tx_reset() cleans the TX ring without the TX lock.
emac_netif_stop() only keeps MAL NAPI from polling RX for this EMAC,
mal_poll() still calls emac_poll_tx() for it. A descriptor cleaned by
emac_clean_tx_ring() has MAL_TX_CTRL_READY clear and looks completed,
so emac_poll_tx() can free the same skb a second time and both race on
tx_cnt and ack_slot.

On a Cisco Meraki MX60, each MTU change to or from a jumbo MTU does a full
TX reset. Toggling the MTU while forwarding traffic corrupted skbs within
minutes (list_del corruption in GRO). With the ring cleaned under the TX
lock it survived 10 minutes of the same test.

Fixes: 1d3bb996481e ("Device tree aware EMAC driver")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/net/ethernet/ibm/emac/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index b676b286e339..75e51ace16f7 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);
 
 	emac_configure(dev);
 
-- 
2.56.0

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