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

[sashiko-bot@kernel.org]

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

commit d2a821de33aec8b828ff820d968fcd13ce493790
Author: Bradley Morgan <brads@mainlining.org>

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.

> diff --git a/drivers/net/ethernet/microchip/encx24j600.c b/drivers/net/ethernet/microchip/encx24j600.c
> --- 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)
>  	/* 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.

[ ... ]

> @@ -1100,7 +1096,7 @@ static void encx24j600_spi_remove(struct spi_device *spi)
>  	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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788513591.git.brads@mainlining.org?part=2