linux-kernel - Re: [net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sat, 15 Jun 2024 15:04:39 +0200
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: Paul Barker <paul.barker.ct@...renesas.com>
Cc: Sergey Shtylyov <s.shtylyov@....ru>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Biju Das <biju.das.jz@...renesas.com>,
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>,
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
	Mitsuhiro Kimura <mitsuhiro.kimura.kc@...esas.com>,
	netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH 2/2] net: ravb: Fix R-Car RX frame size limit

Hi Paul,

Thanks for your work.

On 2024-06-15 11:30:38 +0100, Paul Barker wrote:
> The RX frame size limit should not be based on the current MTU setting.
> Instead it should be based on the hardware capabilities.
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

I agree with the change the RFLR.RFL setting should not be connected to 
the MTU setting. And this likely comes from the early days of the driver 
where neither Rx or Tx supported multiple descriptors for each packet.  
In those days the single descriptor used for each packet was tied to the 
MTU setting. So likely the fixes tag should point to a later commit?

This is a great find and shows a flaw in the driver. But limiting the 
size of each descriptor used for Tx is only half the solution right? As 
the driver now supports multiple descriptors for Tx (on GbEth) the 
driver allows setting an MTU larger then the maximum size for single 
descriptor on those devices. But the xmit function of the driver still 
hardcode the maximum of 2 descriptors for each Tx packet. And it only 
uses the two descriptors to align the data to hardware constrains.

Is it not incorrect for the driver to accept an MTU larger then the 
maximum size of a single descriptor with the current Tx implementation?  
The driver can only support larger MTU settings for Rx, but not Tx ATM.

I think the complete fix is to extend ravb_start_xmit() to fully support 
split descriptors for packets larger then the maximum single descriptor 
size. Not just to align the packet between a DT_FSTART and DT_FEND 
descriptor when needed.

> Signed-off-by: Paul Barker <paul.barker.ct@...renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 02cbf850bd85..481c854cb305 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -555,8 +555,10 @@ static void ravb_emac_init_gbeth(struct net_device *ndev)
>  
>  static void ravb_emac_init_rcar(struct net_device *ndev)
>  {
> +	struct ravb_private *priv = netdev_priv(ndev);
> +
>  	/* Receive frame limit set register */
> -	ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
> +	ravb_write(ndev, priv->info->rx_max_frame_size + ETH_FCS_LEN, RFLR);
>  
>  	/* EMAC Mode: PAUSE prohibition; Duplex; RX Checksum; TX; RX */
>  	ravb_write(ndev, ECMR_ZPF | ECMR_DM |
> -- 
> 2.39.2
> 

-- 
Kind Regards,
Niklas Söderlund