| 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
| ||
|
Message-ID: <fe77a548-a248-95f3-f840-8cd6ee0c1c27@intel.com>
Date: Tue, 28 Feb 2023 17:12:35 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Geoff Levand <geoff@...radead.org>
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
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.
[...]
> static int gelic_descr_prepare_rx(struct gelic_card *card,
> struct gelic_descr *descr)
> {
> - int offset;
> - unsigned int bufsize;
> + struct device *dev = ctodev(card);
> + void *napi_buff;
>
> if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE)
> dev_info(ctodev(card), "%s: ERROR status\n", __func__);
> - /* we need to round up the buffer size to a multiple of 128 */
> - bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
> -
> - /* and we need to have it 128 byte aligned, therefore we allocate a
> - * bit more */
> - descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
> - if (!descr->skb) {
> - descr->buf_addr = 0; /* tell DMAC don't touch memory */
> - return -ENOMEM;
> - }
> - descr->buf_size = cpu_to_be32(bufsize);
> +
> descr->dmac_cmd_status = 0;
> descr->result_size = 0;
> descr->valid_size = 0;
> descr->data_error = 0;
> + descr->buf_size = cpu_to_be32(GELIC_NET_MAX_MTU);
> +
> + napi_buff = napi_alloc_frag_align(GELIC_NET_MAX_MTU,
> + GELIC_NET_RXBUF_ALIGN);
Must be aligned to the opening brace.
> +
Redundant newline.
> + if (unlikely(!napi_buff)) {
> + descr->skb = NULL;
> + descr->buf_addr = 0;
> + descr->buf_size = 0;
> + return -ENOMEM;
> + }
> +
> + 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.
> +
This is also redundant.
> + if (unlikely(!descr->skb)) {
> + skb_free_frag(napi_buff);
> + descr->skb = NULL;
> + descr->buf_addr = 0;
> + descr->buf_size = 0;
> + return -ENOMEM;
> + }
> +
> + descr->buf_addr = cpu_to_be32(dma_map_single(dev, napi_buff,
> + GELIC_NET_MAX_MTU, DMA_FROM_DEVICE));
>
> - offset = ((unsigned long)descr->skb->data) &
> - (GELIC_NET_RXBUF_ALIGN - 1);
> - if (offset)
> - skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
> - /* io-mmu-map the skb */
> - descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
> - descr->skb->data,
> - GELIC_NET_MAX_MTU,
> - DMA_FROM_DEVICE));
> if (!descr->buf_addr) {
> - dev_kfree_skb_any(descr->skb);
> + skb_free_frag(napi_buff);
> descr->skb = NULL;
> - dev_info(ctodev(card),
> - "%s:Could not iommu-map rx buffer\n", __func__);
> + descr->buf_addr = 0;
> + descr->buf_size = 0;
> + dev_err_once(dev, "%s:Could not iommu-map rx buffer\n",
> + __func__);
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> return -ENOMEM;
> - } else {
> - gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> - return 0;
> }
> +
> + gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> + return 0;
An empty newline is preferred before any `return`.
> }
>
> /**
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> index 68f324ed4eaf..d3868eb7234c 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> @@ -19,7 +19,7 @@
> #define GELIC_NET_RX_DESCRIPTORS 128 /* num of descriptors */
> #define GELIC_NET_TX_DESCRIPTORS 128 /* num of descriptors */
>
> -#define GELIC_NET_MAX_MTU VLAN_ETH_FRAME_LEN
> +#define GELIC_NET_MAX_MTU 1920
> #define GELIC_NET_MIN_MTU VLAN_ETH_ZLEN
> #define GELIC_NET_RXBUF_ALIGN 128
> #define GELIC_CARD_RX_CSUM_DEFAULT 1 /* hw chksum */
Thanks,
Olek
Powered by blists - more mailing lists