Re: [PATCH net-next v4 05/13] 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

Creates a queue for the exception path to handle packets with unknown MAC
addresses. These packets are sent to the CPU for MAC learning and broadcast.

> diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
> index e05c42db5f4c8..bf26c1a3384aa 100644
> --- a/drivers/net/ethernet/renesas/rswitch_main.c
> +++ b/drivers/net/ethernet/renesas/rswitch_main.c
> @@ -682,6 +682,34 @@ static int rswitch_rxdmac_init(struct rswitch_private *priv, unsigned int index)
[ ... ]
> +static int rswitch_shared_rx_queue_alloc(struct rswitch_private *priv)
> +{
[ ... ]
> +	err = rswitch_gwca_queue_alloc(NULL, priv, gwca->l2_shared_rx_queue, false, RX_RING_SIZE);
> +	if (err < 0) {
> +		rswitch_gwca_put(priv, gwca->l2_shared_rx_queue);
> +		return err;
> +	}
> +
> +	return rswitch_gwca_queue_ext_ts_format(dev, priv, gwca->l2_shared_rx_queue);
> +}

If rswitch_gwca_queue_ext_ts_format() fails, does this leak the DMA resources
allocated by rswitch_gwca_queue_alloc() and the queue bitmask index acquired
via rswitch_gwca_get()?

[ ... ]
> @@ -954,15 +993,40 @@ static int rswitch_poll(struct napi_struct *napi, int budget)
[ ... ]
> -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 int rx_q_index;
> +	u32 spn;
> +
> +	rdev = netdev_priv(gq->ndev);

Since gq->ndev is initialized to NULL for the shared exception queue, does
this unconditionally dereference a NULL pointer?

> +	rx_q_index = rdev->rx_queue->index;

And compute an invalid memory address here?

> +
> +/* 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, desc->info1);

Because desc->info1 is a little-endian 64-bit field (__le64), will applying a
host-endian bitfield mask without converting it via le64_to_cpu() first yield
garbage values on big-endian architectures?

> +		ndev = priv->rdev[spn]->ndev;

Can a malformed or byte-swapped descriptor result in an out-of-bounds memory
read (spn >= RSWITCH_NUM_PORTS) here?

> +		rdev = netdev_priv(ndev);
> +		gq->ndev = ndev;
> +		/* store original rx_queue */
> +		rdev->rx_old_queue = rdev->rx_queue;
> +		rdev->rx_queue = gq;

Does this lockless swap of queue pointers in hardirq context create a data
race with rswitch_poll(), which reads and restores them in NAPI softirq
context?

Furthermore, if napi_schedule_prep() returns false because NAPI is already
running, the interrupt is not disabled. If a subsequent interrupt for the same
port executes this swap again, will it overwrite rdev->rx_old_queue with the
shared queue pointer (gq) and permanently lose the normal RX queue?

Also, l2_shared_rx_queue aggregates packets from all physical ports. Since
this reads the source port (spn) of only the first pending packet and
schedules NAPI for that specific network interface, won't the NAPI polling
loop consume subsequent packets in the batch and incorrectly attribute them
to the first packet's interface?

> +	}
>  
>  	if (napi_schedule_prep(&rdev->napi)) {
[ ... ]
> @@ -1646,6 +1721,7 @@ static int rswitch_stop(struct net_device *ndev)
[ ... ]
>  	spin_lock_irqsave(&rdev->priv->lock, flags);
>  	rswitch_enadis_data_irq(rdev->priv, rdev->tx_queue->index, false);
>  	rswitch_enadis_data_irq(rdev->priv, rdev->rx_queue->index, false);
> +	rswitch_enadis_data_irq(rdev->priv, rdev->priv->gwca.l2_shared_rx_queue->index, false);

Since l2_shared_rx_queue is a global resource used for MAC learning exception
packets across all ports, does stopping a single network interface break the
globally shared exception path for all other active interfaces?