netdev - Re: [PATCH net v6 1/2] net/ps3_gelic_net: Fix RX sk

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]

Message-ID: <188a8527-23ce-5a5a-b1ca-3d45b03f9086@infradead.org>
Date:   Sun, 26 Mar 2023 13:38:30 -0700
From:   Geoff Levand <geoff@...radead.org>
To:     Alexander Lobakin <aleksander.lobakin@...el.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net v6 1/2] net/ps3_gelic_net: Fix RX sk_buff length

Hi,

I've gone back to setting up the napi routines.

On 2/28/23 08:12, Alexander Lobakin wrote:
> From: Geoff Levand <geoff@...radead.org>
> Date: Sun, 26 Feb 2023 02:25:42 +0000
> 
>> The Gelic Ethernet device needs to have the RX sk_buffs aligned to
>> GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a
>> multiple of GELIC_NET_RXBUF_ALIGN.

>> +
>> +	napi_buff = napi_alloc_frag_align(GELIC_NET_MAX_MTU,
>> +		GELIC_NET_RXBUF_ALIGN);
>> +
>> +	descr->skb = napi_build_skb(napi_buff, GELIC_NET_MAX_MTU);
> 
> You're mixing two, no, three things here.
> 
> 1. MTU. I.e. max L3+ payload len. It doesn't include Eth header, VLAN
>    and FCS.
> 2. Max frame size. It is MTU + the abovementioned. Usually it's
>    something like `some power of two - 1`.
> 3. skb truesize.
>    It is: frame size (i.e. 2) + headroom (usually %NET_SKB_PAD when
>    !XDP, %XDP_PACKET_HEADROOM otherwise, plus %NET_IP_ALIGN) + tailroom
>    (SKB_DATA_ALIGN(sizeof(struct skb_shared_info), see
>     SKB_WITH_OVERHEAD() and adjacent macros).
> 
> I'm not sure whether your definition is the first or the second... or
> maybe third? You had 1522, but then changed to 1920? You must pass the
> third to napi_build_skb().
> So you should calculate the truesize first, then allocate a frag and
> build an skb. Then skb->data will point to the free space with the
> length of your max frame size.
> And the truesize calculation might be not just a hardcoded value, but an
> expression where you add all those head- and tailrooms, so that they
> will be calculated depending on the platform's configuration.
> 
> Your current don't reserve any space as headroom, so that frames / HW
> visible part starts at the very beginning of a frag. It's okay, I mean,
> there will be reallocations when the stack needs more headroom, but
> definitely not something to kill the system. You could leave it to an
> improvement series in the future*.
> But it either way *must* include tailroom, e.g.
> SKB_DATA_ALIGN(see_above), otherwise your HW might overwrite kernel
> structures.
> 
> * given that the required HW alignment is 128, I'd just allocate 128
> bytes more and then do `skb_reserve(skb, RXBUF_HW_ALIGN` right after
> napi_build_skb() to avoid reallocations.

Looking at the docs for the PS3's gelic network device I found
that the DMA buffer it uses has a fixed layout:

  VLAN Data   2 bytes
  Dest MAC    6 bytes
  Source MAC  6 bytes
  Type/Length 2 bytes
  DATA        46-2294 bytes

So, the max DMA buffer size is 2310, which I guess is the same
as the MAX_FRAME size, which is given as 2312.  That's about
18.05*128.  So if the napi_buff size is 19*128 = 2432 and the
start aligned to 128, that should give me what I need:

  #define GELIC_NET_RXBUF_ALIGN 128
  static const unsigned int napi_buff_size = 19 * GELIC_NET_RXBUF_ALIGN;

  napi_buff = napi_alloc_frag_align(napi_buff_size, GELIC_NET_RXBUF_ALIGN);

  descr->skb = napi_build_skb(napi_buff, napi_buff_size);

  cpu_addr = dma_map_single(dev, napi_buff, napi_buff_size, DMA_FROM_DEVICE);

You can find the actual patch here:

  https://git.kernel.org/pub/scm/linux/kernel/git/geoff/ps3-linux.git/commit/?h=ps3-queue-v6.3--gelic-work&id=629de5a5d2875354c5d48fca7f5c1d24f4bf3a8e

I did some rigorous testing with this and didn't have any
problems.

-Geoff