From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 27CF0460C5;
	Mon, 20 Jan 2025 16:28:58 +0100 (CET)
Received: from mails.dpdk.org (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id 0ED60410E7;
	Mon, 20 Jan 2025 16:28:58 +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 88A914021F
 for <dev@dpdk.org>; Mon, 20 Jan 2025 16:28:55 +0100 (CET)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
 s=mimecast20190719; t=1737386935;
 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=a24YWrEU4v2SRsNxuA/8Q41J28326+K65jMQPraUZF1o5sVfO9Le4pVpzl35AhFsdXm8U9
 K/0Re6lUHXE57egJkjK55E2PQN33D75IA6RiriZMwbwe8+NkltvXqi05L2vw6YEJtbfCrN
 2oT2NjTGGK0uIGGZUJSkUdCNkyyX1Bk=
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-211-lDT09bb5NMOYU3goolaXOg-1; Mon, 20 Jan 2025 10:28:53 -0500
X-MC-Unique: lDT09bb5NMOYU3goolaXOg-1
X-Mimecast-MFC-AGG-ID: lDT09bb5NMOYU3goolaXOg
Received: by mail-wr1-f70.google.com with SMTP id
 ffacd0b85a97d-38bee9ae3b7so3137965f8f.1
 for <dev@dpdk.org>; Mon, 20 Jan 2025 07:28:52 -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=JxODDuW52ZCArZAUYyFXYoggVhbtcnCfjr5iXcxiR90pkoGKqIPMwFDnICtlFK+oys
 ZsCNNs2VmNwcJcyeaBOVtt7GHPixbj025N+H7YnDncv14pLKaBX1H+tSkyJM2k6Ab4cu
 9vooLTMnC0hAwYCW4Uw0EN3VbDBLErVGSzEOyUfa/008pT1BlJsS9Nwf2C00hNYGl2DP
 zARNvwcq+aI7ncj+wNPyWAF09l4NUUp+xWSGaI9xS8dDhRIO0VvCIZCRybKJ5TinO+Av
 wPTwBwRQgGvhxju/Rzx8qVSFeYfkDiJqlCVudRkP/hAKkfHMjVOj7jKmJWwYJhF9GW8j
 njGQ==
X-Forwarded-Encrypted: i=1;
 AJvYcCVYlqV2c98jbq09ekDtJamzboyiFwPkrbFIsDAZpW4WiT8q+rlo+t1aH6GH2FxK5ISRbT0=@dpdk.org
X-Gm-Message-State: AOJu0Yyq5fmHhD4gYCEYAAIOInaMzooelTqfN1fmJ31xyo/VCLbmurL+
 Z9ePUWorT263XCaoA5Vk7oWLxWcisC0oA5+lmRgIisDFW84BgvomBabeElh8umke1hxo/wqgb8Z
 ksN24b8E5xag5GmJb7UqsA5wDe88dAMF6HFoRrJa6
X-Gm-Gg: ASbGncvut+QiP37c/TexrZCH0OzwsuKp3GjrChgaVnABhhW0hjqrmOfMfjJ1pglmU/W
 l8P48YNcjlmjO9tvhUuCf4UuaKrpnMv2GkTLxWN7R1sBUuHJNKojbeAKP1fwKWDGaiQsptqf6WF
 qGqN+FdDFwHQBCVhF6i3Dk/QBD+CKzHzTDpD4ZAIl7ye8HzoPdze4grsAfGjxeGcJWGRgSYMqjt
 jHq66/uUwg0iwjSKTaM4F4e7CUttU6gr7i/TKQNr9YqLBfzyE0zryQjTQZVNcb1mOpHaCc8dWVh
 PTt5hFFZk9jOL9GrjwyLx58v6jne3vYzdNKv5Lro1Rw2SmdEhIZA/TcC/41WI7CCmg==
X-Received: by 2002:a5d:598d:0:b0:38a:5122:5a10 with SMTP id
 ffacd0b85a97d-38bf5662800mr13710407f8f.15.1737386931974; 
 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: istDtPPDmPbLHfrkd3KD6flvXJGaPcf_LlHURSiusG0_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: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-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.