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

[Michael Dege]

Hello Jakub,

Thank you for your review.


> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: Monday, May 25, 2026 10:41 PM
> To: Michael Dege <michael.dege@xxxxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>; Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>;
> andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx; niklas.soderlund
> <niklas.soderlund@xxxxxxxxxxxx>; paul@xxxxxxxxxxx; richardcochran@xxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> linux-renesas-soc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next v5 05/14] net: renesas: rswitch: add exception path for packets with
> unknown dst MAC
> 
> 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.

No, this is not the case. The exception path and therefore also the shared queue is only used cases 
when the HW cannot find the MAC address in the hash table. The HW sorts these packets into this special
queue. Even if a packet with an unknown MAC address is received from a different port, it will take the
same path as the previous. Currently we only support a single GWCA (CPU) port.

All other packets are sorted into the correct queues by the HW. And don't land in the shared queue.

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

Does this mean we cannot trust the GENMASK and FIELD_GET macros to return the current bit fields? 

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

Only packets with previously unlearned MAC address are inserted into the shared queue. All other packets
are sorted into the per port queue if they are aimed at the GWCA (CPU). All other packets aimed at 
physical ports are forwarded in hardware and not seen by the CPU.

> 
> > +		/* 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.

There is only one shared 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?

As far as I see the situation, no more so, than for the other queues. 

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

This is an error. I will fix this.

Best regards,

Michael

> 
> > +		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);