Re: [PATCH net-next v5 05/14] net: renesas: rswitch: add exception path for packets with unknown dst MAC

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: renesas: rswitch: add exception path for packets with unknown dst MAC

Packets with unknown MAC address cannot be handled by the HW forwarding.
Creates an exception path with a shared queue to forward these packets to
the network driver for MAC learning.

> diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
> index 9510bcdf9e830..1f4ec65f6f9bd 100644
> --- a/drivers/net/ethernet/renesas/rswitch_main.c
> +++ b/drivers/net/ethernet/renesas/rswitch_main.c
[ ... ]
> @@ -736,6 +772,8 @@ static int rswitch_gwca_hw_deinit(struct rswitch_private *priv)
>  	if (err < 0)
>  		return err;
>  

[Severity: Medium]
Will this leak the shared RX queue memory if rswitch_gwca_change_mode() fails
during teardown and returns early?

> +	rswitch_shared_rx_queue_free(priv);
> +
>  	return rswitch_gwca_change_mode(priv, GWMC_OPC_DISABLE);
>  }
[ ... ]
> @@ -944,10 +982,15 @@ static int rswitch_poll(struct napi_struct *napi, int budget)
>  
>  	if (napi_complete_done(napi, budget - quota)) {
>  		spin_lock_irqsave(&priv->lock, flags);
> +		if (rdev->rx_old_queue) {
> +			rdev->rx_queue = rdev->rx_old_queue;
> +			rdev->rx_old_queue = NULL;
> +		}
>  		if (test_bit(rdev->port, priv->opened_ports)) {
>  			rswitch_enadis_data_irq(priv, rdev->tx_queue->index, true);
>  			rswitch_enadis_data_irq(priv, rdev->rx_queue->index, true);
>  		}
> +		rswitch_enadis_data_irq(priv, priv->gwca.l2_shared_rx_queue->index, true);

[Severity: Critical]
Does unmasking the shared RX queue globally here create a race condition?

If Port A completes NAPI while Port B's NAPI is still actively polling the
shared queue, couldn't another interrupt fire, scheduling another NAPI and
resulting in multiple NAPIs running concurrently on the same shared queue?

This might cause lockless concurrent modifications to gq->cur and gq->dirty,
leading to list corruption.

>  		spin_unlock_irqrestore(&priv->lock, flags);
>  	}
>  
[ ... ]
> @@ -960,15 +1003,41 @@ static int rswitch_poll(struct napi_struct *napi, int budget)
>  	return 0;
>  }
>  
> -static void rswitch_queue_interrupt(struct net_device *ndev)
> +static void rswitch_queue_interrupt(struct rswitch_private *priv, struct rswitch_gwca_queue *gq)
>  {
> -	struct rswitch_device *rdev = netdev_priv(ndev);
> +	struct rswitch_ext_ts_desc *desc;
> +	struct rswitch_device *rdev;
> +	struct net_device *ndev;
> +	unsigned long flags;
> +	u32 spn;
> +
> +	/* If we receive a shared queue through the exception path, it will be missing the ndev
> +	 * pointer. This needs to be added to be able to determine from which port the packet was
> +	 * received. Then we temporarily exchange the rx_queue pointer in rdev. This will be
> +	 * restored after the packet has been processed.
> +	 */
> +
> +	if (gq->index == priv->gwca.l2_shared_rx_queue->index) {
> +		desc = &gq->rx_ring[gq->cur];
> +		spn = FIELD_GET(SPN, le64_to_cpu(desc->info1));

[Severity: Medium]
Could this read stale or garbage data from the descriptor?

It appears to read the DMA descriptor without verifying that the hardware
has handed ownership back to the CPU (e.g., checking DT_FEMPTY) and without
a dma_rmb() barrier before the read.

[Severity: High]
Is it possible for the hardware to provide an out-of-bounds SPN value here?

Since spn is used to index priv->rdev[] on the next line, an unexpected or
corrupted value might cause an out-of-bounds memory access.

> +		ndev = priv->rdev[spn]->ndev;
> +		rdev = netdev_priv(ndev);
> +		gq->ndev = ndev;

[Severity: High]
Since the shared queue collects exception packets from all ports, will this
assign the entire queue to the NAPI of the first packet's port?

When NAPI polls and drains the queue, it seems subsequent packets might be
injected into the network stack under the first packet's ndev, even if they
originated from different physical ports.

> +		/* store original rx_queue */
> +		spin_lock_irqsave(&priv->lock, flags);
> +		rdev->rx_old_queue = rdev->rx_queue;
> +		rdev->rx_queue = gq;

[Severity: High]
Can rdev->rx_old_queue be permanently overwritten with the shared queue
pointer here?

If a second shared queue interrupt fires before NAPI completes and restores
the pointer, the handler will execute this path again and overwrite the
saved normal queue pointer with gq, leaking the normal RX queue.

> +		spin_unlock_irqrestore(&priv->lock, flags);
> +	} else {
> +		rdev = netdev_priv(gq->ndev);
> +	}
>  
>  	if (napi_schedule_prep(&rdev->napi)) {
> -		spin_lock(&rdev->priv->lock);
> -		rswitch_enadis_data_irq(rdev->priv, rdev->tx_queue->index, false);
> -		rswitch_enadis_data_irq(rdev->priv, rdev->rx_queue->index, false);
> -		spin_unlock(&rdev->priv->lock);
> +		spin_lock(&priv->lock);
> +		rswitch_enadis_data_irq(priv, rdev->tx_queue->index, false);
> +		rswitch_enadis_data_irq(priv, rdev->rx_queue->index, false);
> +		rswitch_enadis_data_irq(priv, priv->gwca.l2_shared_rx_queue->index, false);

[Severity: Critical]
If napi_schedule_prep() returns false (because the target NAPI is already
running), hardware interrupts are not disabled. Could this lead to an
immediate interrupt storm for the shared RX queue?

> +		spin_unlock(&priv->lock);
>  		__napi_schedule(&rdev->napi);
>  	}
>  }
[ ... ]
> @@ -1653,6 +1733,11 @@ static int rswitch_stop(struct net_device *ndev)
>  	rswitch_enadis_data_irq(rdev->priv, rdev->tx_queue->index, false);
>  	rswitch_enadis_data_irq(rdev->priv, rdev->rx_queue->index, false);
>  	bitmap_clear(rdev->priv->opened_ports, rdev->port, 1);
> +	if (bitmap_empty(rdev->priv->opened_ports, 1))

[Severity: High]
Does this check use the wrong size argument?

The second argument to bitmap_empty() is nbits. By passing 1, it seems it
only checks if bit 0 is empty. If port 0 is closed while other ports are
open, this would evaluate to true and prematurely disable the shared RX
queue interrupt for the remaining active ports.

> +		rswitch_enadis_data_irq(rdev->priv,
> +					rdev->priv->gwca.l2_shared_rx_queue->index,
> +					false);
> +
>  	spin_unlock_irqrestore(&rdev->priv->lock, flags);
>  
>  	napi_disable(&rdev->napi);