netdev - 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: Mon, 17 Jun 2024 15:20:41 +0100
From: Paul Barker <paul.barker.ct@...renesas.com>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
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

On 15/06/2024 14:04, Niklas Söderlund wrote:
> 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?> 

If my understanding of MTU & MRU are correct, even with a single
descriptor we can always accept the same number of bytes regardless of
the current MTU.

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

For the RZ SoCs I have looked at, this isn't an issue. We support
transmitting frames of slightly over 1500 bytes on the SoCs with GbEth
IP, or 2047 bytes on the SoCs with R-Car AVB IP (e.g. RZ/G2H). Given
that a single descriptor can cover up to 4095 bytes of data (with its 12
bit DS field), we don't need split TX descriptors for either of these.

Thanks,

-- 
Paul Barker
Download attachment "OpenPGP_0x27F4B3459F002257.asc" of type "application/pgp-keys" (3521 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)