linux-kernel - Re: [PATCH net v2] net: lantiq_xrx200: increase buffer reservation

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] [day] [month] [year] [list]

Date:   Mon, 13 Dec 2021 07:43:47 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Aleksander Bajkowski <olek2@...pl>
Cc:     hauke@...ke-m.de, davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2] net: lantiq_xrx200: increase buffer reservation

On Mon, 13 Dec 2021 00:05:16 +0100 Aleksander Bajkowski wrote:
> On 12/8/21 5:54 AM, Jakub Kicinski wrote:
> > On Mon,  6 Dec 2021 23:39:09 +0100 Aleksander Jan Bajkowski wrote:  
> >> +static int xrx200_max_frame_len(int mtu)
> >> +{
> >> +	return VLAN_ETH_HLEN + mtu + ETH_FCS_LEN;  
> > 
> > You sure the problem is not that this doesn't include ETH_HLEN? 
> > MTU is the length of the L2 _payload_.
> 
> VLAN_ETH_HLEN (14 + 4) contains ETH_HLEN (14). This function returns
> the length of the frame that is written to the RX descriptor. Maybe
> I don't understand the question and you are asking something else? 

Ah, right, misread that as VLAN_HLEN.

> >> +}
> >> +
> >> +static int xrx200_buffer_size(int mtu)
> >> +{
> >> +	return round_up(xrx200_max_frame_len(mtu) - 1, 4 * XRX200_DMA_BURST_LEN);  
> > 
> > Why the - 1 ? 🤔
> >   
> 
> This is how the hardware behaves. I don't really know where the -1
> comes from. Unfortunately, I do not have access to TRM. 
>
> > For a frame size 101 => max_frame_len 109 you'll presumably want 
> > the buffer to be 116, not 108?
> 
> For a frame size 101 => max_frame_len is 123 (18 + 101 + 4).

You get my point, tho right?

> Infact, PMAC strips FCS and ETH_FCS_LEN may not be needed. This behavior
> is controlled by the PMAC_HD_CTL_RC bit. This bit is enabled from
> the beginning of this driver. Ethtool has the option to enable
> FCS reception, but the ethtool interface is not yet supported
> by this driver. 
> 
> >> +}
> >> +  
> 
> Experiments show that the hardware starts to split the frame at
> max_frame_len() - 1. Some examples:
> 
> pkt len	MTU	max_frame_size()	buffer_size()	desc1	desc2	desc3	desc4
> ----------------------------------------------------------------------------------------------
> 1506		1483		1505		1504		1502	4	X	X
> 1505		1483		1505		1504		1502	3	X	X
> 1504		1483		1505		1504		1504	X	X	X
> 1503		1483		1505		1504		1503	X	X	X
> 1502		1483		1505		1504		1502	X	X	X
> 1501		1483		1505		1504		1501	X	X	X
> ----------------------------------------------------------------------------------------------
> 1249		380		402		416		414	416	416	3
> 1248		380		402		416		414	416	416	2
> 1247		380		402		416		414	416	416	1
> 1246		380		402		416		414	416	416	X
> 1245		380		402		416		414	416	415	X
> ----------------------------------------------------------------------------------------------

Hm, doesn't the former example prove that the calculation in this
patch is incorrect? Shouldn't we be able to receive a 1505B frame 
into a single buffer for the MTU of 1483?

The HW doesn't split at max_frame_len - 1 in the latter example.
Maybe to save one bit of state the HW doesn't register the lowest 
bit of the buffer length? I thinks the buffer is 1504.

I'd lean towards dropping the -1 and letting the DMA alignment
calculation round the whole length up.

Also should we not take NET_IP_ALIGN into account?

Judging by the length of the first buffer when split happens
NET_IP_ALIGN is 2, and HW rounds off (2 + pkt-len) to the DMA
burst size. This makes me think we overrun the end of the buffer
by NET_IP_ALIGN.

> In fact, this patch is a preparation for SG DMA support, which
> I wrote some time ago.