From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <stable-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id F1375460C5
for <public@inbox.dpdk.org>; Mon, 20 Jan 2025 16:28:56 +0100 (CET)
Received: from mails.dpdk.org (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id CDDCC4021F;
Mon, 20 Jan 2025 16:28:56 +0100 (CET)
Received: from us-smtp-delivery-124.mimecast.com
(us-smtp-delivery-124.mimecast.com [170.10.129.124])
by mails.dpdk.org (Postfix) with ESMTP id 0CFC74021F
for <stable@dpdk.org>; Mon, 20 Jan 2025 16:28:54 +0100 (CET)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
s=mimecast20190719; t=1737386934;
h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
to:to:cc:cc:mime-version:mime-version:content-type:content-type:
content-transfer-encoding:content-transfer-encoding:
in-reply-to:in-reply-to:references:references;
bh=kq3AUDn0+1CJaG4jdlk+lr5eS3KMaxxCdLUUpNmQ4ac=;
b=hjxG3+LyYUwp0y6/QnH8qQFSnwag0rbVudZL74qdjim9ZpLGdNcSWyi8rkIS2CWTUCrogH
rlsLpE0HI+6VndjSe6hpaehwjtQWQMfnB1RYd/+exmEC6naspzJ0i2H5DGFiXM5jllkxY9
2no5RXQ2apds57zw/c2GSfcsvw00qUw=
Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com
[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS
(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id
us-mta-213-2_skNjRkMu6UovvvjWXCdw-1; Mon, 20 Jan 2025 10:28:53 -0500
X-MC-Unique: 2_skNjRkMu6UovvvjWXCdw-1
X-Mimecast-MFC-AGG-ID: 2_skNjRkMu6UovvvjWXCdw
Received: by mail-wr1-f70.google.com with SMTP id
ffacd0b85a97d-385e27c5949so3056194f8f.3
for <stable@dpdk.org>; Mon, 20 Jan 2025 07:28:53 -0800 (PST)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20230601; t=1737386932; x=1737991732;
h=content-transfer-encoding:in-reply-to:from:content-language
:references:cc:to:subject:user-agent:mime-version:date:message-id
:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;
bh=kq3AUDn0+1CJaG4jdlk+lr5eS3KMaxxCdLUUpNmQ4ac=;
b=onuelXSyJOiQ0eVdOBde8a6XfGKfVOhRmF+hGRC0PgmUIa21a4chY+q2/WdD52UM8F
nG/T2tJ6hTSqjDWRPWXP6KZIKdd+w8MXY2I8VkasZbcXY3mtNgmS6kOG7hqWD1tsuv7/
DSGeXRS14bqw9LPKaWAzG5IKSF7byDYsrLobZM+gwlJe0Os1aXrJcUiZtYM7+aCV497r
tAl9tIKhXMoAVwM8ubZjaf1+QL6LyxZjx4WEj9P+5FCoLVUZIl4G06r1Ayhmkoel9smL
nUtMoafJSXRt9Vk5wlghscjY5rUcuNbfA1sMuc98JINEggaHWrngQ5At1dHXGkwkdd7r
+rIg==
X-Gm-Message-State: AOJu0YxBqHmi2NYu6pnXpCiBQzDCWmjPSY9GY75nEd1Ek33QKsPt1rVX
frBR+n2w/4Xx5FryOT5eiJe43xDmrrPgRysnruWl9uG4ogGaJqDtiW6z72+Nap7kAt9ZZZ5acCf
JemL58o5uWdMr76h96RC5MKjAAGvPxnHj+q4/3rqv2oVH
X-Gm-Gg: ASbGncs8lCwdUCQdi34tltrVqV1UYSDzFY77+hp4AxVv0N6rZ6ccHTMajdEBP9uccT6
xESrpuFlMOcOGEwqYqhj8mBdvpJ+gQho3ZQTLUXn/tChQBBNzrMZRPo67lsgd3cjDI8HlArYUGn
Gq9rDqIN9y80/kDjqcfiMPD/5MboDWKPMsXNW1fHcHN086taAUgAhqIES6YmIZgQ6WMPi78IuEX
H0Reb6ow22TkVACGPCAnubXg6edQhKayOu1layuBl4ZcuTN8FuGFCPFPAgNt0oZyNIw1xJXGQ9X
GPnp8oT+ER87ukDWE4Y9+9PvpN8nx9s1BMtH8fcme2m4dCmCbdTqwlCmQElY/Cl7Ww==
X-Received: by 2002:a5d:598d:0:b0:38a:5122:5a10 with SMTP id
ffacd0b85a97d-38bf5662800mr13710410f8f.15.1737386931975;
Mon, 20 Jan 2025 07:28:51 -0800 (PST)
X-Google-Smtp-Source: AGHT+IEN7G1IB7Xd1K/vNe5CICN3+hiER5jy9nZSJ6e7jJFWILNKGZw+3gpE2FC40O9adfnkxBK0Gg==
X-Received: by 2002:a5d:598d:0:b0:38a:5122:5a10 with SMTP id
ffacd0b85a97d-38bf5662800mr13710384f8f.15.1737386931610;
Mon, 20 Jan 2025 07:28:51 -0800 (PST)
Received: from ?IPV6:2a06:5900:c00c:9000:45fa:7c02:baca:2de9?
([2a06:5900:c00c:9000:45fa:7c02:baca:2de9])
by smtp.gmail.com with ESMTPSA id
5b1f17b1804b1-438a3805614sm81544355e9.22.2025.01.20.07.28.50
(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);
Mon, 20 Jan 2025 07:28:51 -0800 (PST)
Message-ID: <44ffb73b-427d-4ddf-a195-900e05241050@redhat.com>
Date: Mon, 20 Jan 2025 15:28:50 +0000
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/2] net/af_xdp: Refactor af_xdp_tx_zc()
To: Ariel Otilibili <ariel.otilibili@6wind.com>, dev@dpdk.org
Cc: stable@dpdk.org, Stephen Hemminger <stephen@networkplumber.org>,
Thomas Monjalon <thomas@monjalon.net>,
David Marchand <david.marchand@redhat.com>,
Ciara Loftus <ciara.loftus@intel.com>
References: <20250116195640.68885-1-ariel.otilibili@6wind.com>
<20250116225151.188214-1-ariel.otilibili@6wind.com>
<20250116225151.188214-3-ariel.otilibili@6wind.com>
From: Maryam Tahhan <mtahhan@redhat.com>
In-Reply-To: <20250116225151.188214-3-ariel.otilibili@6wind.com>
X-Mimecast-Spam-Score: 0
X-Mimecast-MFC-PROC-ID: -yCF6f0kxXGRPCMDhnciz5RDNAfo7EZP32vZYGmktbg_1737386932
X-Mimecast-Originator: redhat.com
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
X-BeenThere: stable@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: patches for DPDK stable branches <stable.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/stable>,
<mailto:stable-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/stable/>
List-Post: <mailto:stable@dpdk.org>
List-Help: <mailto:stable-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/stable>,
<mailto:stable-request@dpdk.org?subject=subscribe>
Errors-To: stable-bounces@dpdk.org
On 16/01/2025 17:51, Ariel Otilibili wrote:
> Both legs of the loop share the same logic: either to go out of the
> function, or to fall through to the next instructions.
>
> Depends-on: patch-1 ("net/af_xdp: fix use after free in af_xdp_tx_zc()")
> Bugzilla ID: 1440
> Signed-off-by: Ariel Otilibili <ariel.otilibili@6wind.com>
> ---
> drivers/net/af_xdp/rte_eth_af_xdp.c | 47 +++++++++++++----------------
> 1 file changed, 21 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index 4326a29f7042..b1de47a41eb3 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -551,6 +551,7 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> uint64_t addr, offset;
> struct xsk_ring_cons *cq = &txq->pair->cq;
> uint32_t free_thresh = cq->size >> 1;
> + struct rte_mbuf *local_mbuf = NULL;
>
> if (xsk_cons_nb_avail(cq, free_thresh) >= free_thresh)
> pull_umem_cq(umem, XSK_RING_CONS__DEFAULT_NUM_DESCS, cq);
> @@ -565,21 +566,8 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> &idx_tx))
> goto out;
> }
> - desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
> - desc->len = mbuf->pkt_len;
> - addr = (uint64_t)mbuf - (uint64_t)umem->buffer -
> - umem->mb_pool->header_size;
> - offset = rte_pktmbuf_mtod(mbuf, uint64_t) -
> - (uint64_t)mbuf +
> - umem->mb_pool->header_size;
> - offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
> - desc->addr = addr | offset;
> - tx_bytes += mbuf->pkt_len;
> - count++;
> } else {
> - struct rte_mbuf *local_mbuf =
> - rte_pktmbuf_alloc(umem->mb_pool);
> - void *pkt;
> + local_mbuf = rte_pktmbuf_alloc(umem->mb_pool);
>
> if (local_mbuf == NULL)
> goto out;
> @@ -588,24 +576,31 @@ af_xdp_tx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
> rte_pktmbuf_free(local_mbuf);
> goto out;
> }
> + }
>
> - desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
> - desc->len = mbuf->pkt_len;
> + struct rte_mbuf *tmp;
> + void *pkt;
> + tmp = mbuf->pool == umem->mb_pool ? mbuf : local_mbuf;
> +
> + desc = xsk_ring_prod__tx_desc(&txq->tx, idx_tx);
> + desc->len = mbuf->pkt_len;
>
> - addr = (uint64_t)local_mbuf - (uint64_t)umem->buffer -
> - umem->mb_pool->header_size;
> - offset = rte_pktmbuf_mtod(local_mbuf, uint64_t) -
> - (uint64_t)local_mbuf +
> - umem->mb_pool->header_size;
> + addr = (uint64_t)tmp - (uint64_t)umem->buffer
> + - umem->mb_pool->header_size;
> + offset = rte_pktmbuf_mtod(tmp, uint64_t) - (uint64_t)tmp
> + + umem->mb_pool->header_size;
> + offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
> + desc->addr = addr | offset;
> +
> + if (mbuf->pool == umem->mb_pool) {
> + tx_bytes += mbuf->pkt_len;
> + } else {
> pkt = xsk_umem__get_data(umem->buffer, addr + offset);
> - offset = offset << XSK_UNALIGNED_BUF_OFFSET_SHIFT;
> - desc->addr = addr | offset;
> - rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *),
> - desc->len);
> + rte_memcpy(pkt, rte_pktmbuf_mtod(mbuf, void *), desc->len);
> tx_bytes += mbuf->pkt_len;
> rte_pktmbuf_free(mbuf);
> - count++;
> }
> + count++;
> }
>
> out:
This ends up duplicating the if condition `if (mbuf->pool ==
umem->mb_pool) {` twice in `af_xdp_tx_zc`. Which is messy to read tbh...
I think it would be better to create an inline function for the
duplicate code that setting desc, addr and offset. These three things
could be pointers passed to the new inline function and that way their
values can be used in `af_xdp_tx_zc()` after they are set. I think that
would cleanup the `af_xdp_tx_zc()` function in a neater way.