netdev - Re: [PATCH 3/3] net: fec: align IP header in hardware

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]

Message-ID: <5cf173c4-84e0-f309-f356-35b114cc166e@nelint.com>
Date:   Wed, 28 Sep 2016 10:14:52 -0700
From:   Eric Nelson <eric@...int.com>
To:     David Laight <David.Laight@...LAB.COM>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     "linux@....linux.org.uk" <linux@....linux.org.uk>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "fugang.duan@....com" <fugang.duan@....com>,
        "otavio@...ystems.com.br" <otavio@...ystems.com.br>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "troy.kisky@...ndarydevices.com" <troy.kisky@...ndarydevices.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH 3/3] net: fec: align IP header in hardware

Thanks David,

On 09/28/2016 09:42 AM, David Laight wrote:
> From: Eric Nelson
>> Sent: 26 September 2016 19:40
>> Hi David,
>>
>> On 09/26/2016 02:26 AM, David Laight wrote:
>>> From: Eric Nelson
>>>> Sent: 24 September 2016 15:42
>>>> The FEC receive accelerator (RACC) supports shifting the data payload of
>>>> received packets by 16-bits, which aligns the payload (IP header) on a
>>>> 4-byte boundary, which is, if not required, at least strongly suggested
>>>> by the Linux networking layer.
>>> ...
>>>> +		/* align IP header */
>>>> +		val |= FEC_RACC_SHIFT16;
>>>
>>> I can't help feeling that there needs to be corresponding
>>> changes to increase the buffer size by 2 (maybe for large mtu)
>>> and to discard two bytes from the frame length.
>>>
>>
>> In the normal case, the fec driver over-allocates all receive packets to
>> be of size FEC_ENET_RX_FRSIZE (2048) minus the value of rx_align,
>> which is either 0x0f (ARM) or 0x03 (PPC).
>>
>> If the frame length is less than rx_copybreak (typically 256), then
>> the frame length from the receive buffer descriptor is used to
>> control the allocation size for a copied buffer, and this will include
>> the two bytes of padding if RACC_SHIFT16 is set.
>>
>>> If probably ought to be predicated on NET_IP_ALIGN as well.
>>>
>> Can you elaborate?
> 
> From reading this it seems that the effect of FEC_RACC_SHIFT16 is to
> add two bytes of 'junk' to the start of every receive frame.
> 

That's right. Two bytes of junk between the MAC header and the
IP header.

> In the 'copybreak' case the new skb would need to be 2 bytes shorter
> than the length reported by the hardware, and the data copied from
> 2 bytes into the dma buffer.
> 

As it stands, the skb allocated by the copybreak routine will include
the two bytes of padding, and the call to skb_pull_inline will ignore
them.

> The extra 2 bytes also mean the that maximum mtu that can be received
> into a buffer is two bytes less.
>

Right, but I think the max is already high enough that this isn't a
problem.

> If someone sets the mtu to (say) 9k for jumbo frames this might matter.
> Even with fixed 2048 byte buffers it reduces the maximum value the mtu
> can be set to by 2.
> 

As far as I can tell, the fec driver doesn't support jumbo frames, and
the max frame length is currently hard-coded at PKT_MAXBUF_SIZE (1522).

This is well within the 2048-byte allocation, even with optional headers
for VLAN etc.

> Now if NET_IP_ALIGN is zero then it is fine for the rx frame to start
> on a 4n boundary, and the skb are likely to be allocated that way.
> In this case you don't want to extra two bytes of 'junk'.
> 
NET_IP_ALIGN is defaulting to 2 by the conditional in skbuff.h

> OTOH if NET_IP_ALIGN is 2 then you need to 'fiddle' things so that
> the data is dma'd to offset -2 in the skb and then ensure that the
> end of frame is set correctly.
> 

That's what the RACC SHIFT16 bit does.

The FEC hardware isn't capable of DMA'ing to an un-aligned address.
On ARM, it requires 64-bit alignment, but suggests 128-bit alignment.

On other (PPC?) architectures, it requires 32-bit alignment. This is
handled by the rx_align field.

Regards,


Eric