Re: [PATCH v2 2/5] net: encx24j600: convert to kthread_run_worker

On 5 September 2026 10:40:38 BST, sashiko-bot@kernel.org wrote:

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

Pre-existing issues:
- [High] Missing state cleanup for `priv->tx_skb` in TX timeout, interface stop, and device removal paths leads to memory leaks, data races, and potential BUG() panics.
- [High] Hardware memory structures read over SPI are not converted from little-endian to CPU endianness.

Meh. No thanks.

--

commit d2a821de33aec8b828ff820d968fcd13ce493790
Author: Bradley Morgan [off-list ref]

net: encx24j600: convert to kthread_run_worker
   
Convert encx24j600 to use kthread_run_worker() instead of the
deprecated kthread_run(kthread_worker_fn) pattern. The new API sets
worker->task before the worker starts.

quoted

diff --git a/drivers/net/ethernet/microchip/encx24j600.c

b/drivers/net/ethernet/microchip/encx24j600.c

quoted

--- a/drivers/net/ethernet/microchip/encx24j600.c
+++ b/drivers/net/ethernet/microchip/encx24j600.c

@@ -884,7 +883,7 @@ static netdev_tx_t encx24j600_tx(struct sk_buff

*skb, struct net_device *dev)

quoted

 	/* Remember the skb for deferred processing */
 	priv->tx_skb = skb;
 
-	kthread_queue_work(&priv->kworker, &priv->tx_work);
+	kthread_queue_work(priv->kworker, &priv->tx_work);

[Severity: High]
This is a pre-existing issue, but is there a missing state cleanup for
priv->tx_skb in the TX timeout path?

If a TX timeout occurs, encx24j600_tx_timeout() wakes the queue without
cleaning up priv->tx_skb or clearing pending TX state. The next TX here
unconditionally overwrites priv->tx_skb, leaking the previous packet.

Furthermore, a delayed hardware interrupt for the previous packet can
arrive
and trigger encx24j600_tx_complete(), which frees the new priv->tx_skb and
sets it to NULL. When encx24j600_tx_complete() subsequently fires for the
new packet, it sees priv->tx_skb is NULL and calls BUG():

drivers/net/ethernet/microchip/encx24j600.c:encx24j600_tx_complete() {
   if (!priv->tx_skb) {
       BUG();
       return;
   }
...

Alternatively, encx24j600_hw_tx() may run concurrently with the delayed
interrupt, reading priv->tx_skb->len when it has been set to NULL, causing
a NULL pointer dereference.

[ ... ]

quoted

@@ -1100,7 +1096,7 @@ static void encx24j600_spi_remove(struct

spi_device *spi)

quoted

 	struct encx24j600_priv *priv = dev_get_drvdata(&spi->dev);
 
 	unregister_netdev(priv->ndev);
-	kthread_stop(priv->kworker_task);
+	kthread_destroy_worker(priv->kworker);
 
 	free_netdev(priv->ndev);
 }

[Severity: High]
This is also a pre-existing issue, but does device removal strand the
un-freed priv->tx_skb?

During driver removal in encx24j600_spi_remove(), the kworker is
destroyed,
and then the driver private data is freed without cleaning up
priv->tx_skb,
which results in a memory leak.

[Severity: High]
This is another pre-existing issue, but are the hardware memory structures
read over SPI properly converted from little-endian to CPU endianness?

The 8-byte Receive Status Vector is read directly from the SPI interface
into a struct rsv using encx24j600_raw_read() in encx24j600_rx_packets():

encx24j600_raw_read(priv, RRXDATA, (u8 *)&rsv, sizeof(rsv));

However, struct rsv in drivers/net/ethernet/microchip/encx24j600_hw.h uses
native CPU types without using explicit __le16 and __le32 types:

struct rsv {
   u16 next_packet;
   u16 len;
   u32 rxstat;
};

On a big-endian system, won't the raw bytes be misinterpreted? For
example,
a packet length of 60 (0x003C) would be read as 0x3C00 (15360), which
exceeds
MAX_FRAMELEN and causes the packet to be dropped. The corrupted
next_packet
pointer would then be written back to the device's ERXTAIL register,
destroying the receive ring buffer state.

--- Thanks!

https://lore.kernel.org/all/EE579805-42F2-4C58-B752-F28779EEB717@grrlz.net/ (local)