netdev - Re: [PATCH iwl-next 11/12] idpf: convert header split mode to libeth + napi_build

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 for Android: free password hash cracker in your pocket

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

Date: Thu, 13 Jun 2024 12:58:48 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <intel-wired-lan@...ts.osuosl.org>, Tony Nguyen
	<anthony.l.nguyen@...el.com>, "David S. Miller" <davem@...emloft.net>, "Eric
 Dumazet" <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Mina Almasry
	<almasrymina@...gle.com>, <nex.sw.ncis.osdt.itp.upstreaming@...el.com>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH iwl-next 11/12] idpf: convert header split mode to libeth
 + napi_build_skb()

From: Jakub Kicinski <kuba@...nel.org>
Date: Wed, 29 May 2024 18:40:12 -0700

> On Tue, 28 May 2024 15:48:45 +0200 Alexander Lobakin wrote:
>> Currently, idpf uses the following model for the header buffers:
>>
>> * buffers are allocated via dma_alloc_coherent();
>> * when receiving, napi_alloc_skb() is called and then the header is
>>   copied to the newly allocated linear part.
>>
>> This is far from optimal as DMA coherent zone is slow on many systems
>> and memcpy() neutralizes the idea and benefits of the header split. Not
>> speaking of that XDP can't be run on DMA coherent buffers, but at the
>> same time the idea of allocating an skb to run XDP program is ill.
>> Instead, use libeth to create page_pools for the header buffers, allocate
>> them dynamically and then build an skb via napi_build_skb() around them
>> with no memory copy. With one exception...
>> When you enable header split, you except you'll always have a separate
> 
>                                     accept

"expect" :D Thanks for spotting, nice catch.

> 
>> header buffer, so that you could reserve headroom and tailroom only
>> there and then use full buffers for the data. For example, this is how
>> TCP zerocopy works -- you have to have the payload aligned to PAGE_SIZE.
>> The current hardware running idpf does *not* guarantee that you'll
>> always have headers placed separately. For example, on my setup, even
>> ICMP packets are written as one piece to the data buffers. You can't
>> build a valid skb around a data buffer in this case.
>> To not complicate things and not lose TCP zerocopy etc., when such thing
>> happens, use the empty header buffer and pull either full frame (if it's
>> short) or the Ethernet header there and build an skb around it. GRO
>> layer will pull more from the data buffer later. This W/A will hopefully
>> be removed one day.
> 
> Hopefully soon, cause it will prevent you from mapping data buffers to
> user space or using DMABUF memory :(

Correct. The HW team is informed and some work on it is happening right
now. I told them that stuff like devmem etc., i.e. when the kernel
doesn't have access to the data buffers, will just choke on this.

I mean, I don't care about unknown packet types since it's very unlikely
and it's almost always some garbage, as well as when the header doesn't
fit into 256 bytes (I can't imagine such situation with known protocols,
but if it can happen, I can bump to 512 or so), but currently, and it's
a shame, idpf does header split only for TCP/UDP/SCTP, on my setup it
didn't want to split even regular ICMP, although it parsed the type
correctly =\ I dunno why they did it this way.
There are some configuration bits which in theory allow you to enable
hsplit for other types of frames, but enabling them didn't change
anything unfortunately.

Thanks,
Olek