Received-SPF: pass (google.com: domain of linux-kernel+bounces-71285-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 19 Feb 2024 13:13:04 +0100
From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
To: Florian Kauer <florian.kauer@linutronix.de>
CC: Jesse Brandeburg <jesse.brandeburg@intel.com>, Tony Nguyen
	<anthony.l.nguyen@intel.com>, "David S. Miller" <davem@davemloft.net>, "Eric
 Dumazet" <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
	<pabeni@redhat.com>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann
	<daniel@iogearbox.net>, Jesper Dangaard Brouer <hawk@kernel.org>, "John
 Fastabend" <john.fastabend@gmail.com>, Jithu Joseph <jithu.joseph@intel.com>,
	Andre Guedes <andre.guedes@intel.com>, Vedang Patel <vedang.patel@intel.com>,
	<kurt@linutronix.de>, <intel-wired-lan@lists.osuosl.org>,
	<netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<bpf@vger.kernel.org>
Subject: Re: [PATCH net 1/1] igc: avoid returning frame twice in XDP_REDIRECT
Message-ID: <ZdNF0ArChUkGzk42@boxer>
References: <20240219090843.9307-1-florian.kauer@linutronix.de>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240219090843.9307-1-florian.kauer@linutronix.de>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?tqwmGu7yZhp8xAXwJe8PnNrveWwha7ZJus4BJ9jGVuvhK1LucWSoqRdPqqoN?=
 =?us-ascii?Q?Ngge7Vol1IYIR3QsyBwchFEn2jOvRfCp6SchSLx8DOZoP1Sw72Sj9WT0CYiw?=
 =?us-ascii?Q?86PfzVLjl0i1q5Lla32MYR9y/Mcj6sVy8Gc2shHzzWSjsxRs60ZK+7whtbK5?=
 =?us-ascii?Q?Z0oZg2L+6p0/RKRAwu2tzSbGFsMmb+ex2iYVd/+SkNHpN4TCM/jDLmWTIhB0?=
 =?us-ascii?Q?NHbsReCQQ/oUjwTznXsErOZPJMVdbfcbkuw3E+nk5QsHNxQCYTJ0mBZHPuLK?=
 =?us-ascii?Q?JIvVi2KL88toMpCdkyKj3zZyuC3I5I2Muqe21Tzxud2e5wdB2E9yFh6rbLKj?=
 =?us-ascii?Q?7T2xn1mJcitUNRPMgH1mcaoOQIUTI8i7bGH2fxIHP8JixWt3FmF4v2VW9Ipu?=
 =?us-ascii?Q?+2iOAAmvFqfaKmQzjHPExvZYcU0ewe2b1dsmIg+nXfG2DjHngbYrlMe5E0Hr?=
 =?us-ascii?Q?Hz6aY64u0x2sPktb9jjGorDhl5nTmrIrmKCjV2ndGnZJsUvXHWj3ESIh3itV?=
 =?us-ascii?Q?l2qeS/ifIT8fILUCo0U/NgQxUaVcC7THvosoUzCr4ODUiRY9Z3d16NvowxK6?=
 =?us-ascii?Q?SivnC3k+fDpNudituNYT5InT43xPONxKR/cgBmTO8eOzDep7ZyHymJVMeoKu?=
 =?us-ascii?Q?dQnPOQRN0XmGtg+KCemmrqwMlpooW/Ukges62kDZ2mxzLUiZ4CP1kJCLkAlm?=
 =?us-ascii?Q?JBvYnm1+LNlt4rputZyzyR3VSGBLVQMqCTmzB7W4k4aIdllPlcBCJ1ZiaRTY?=
 =?us-ascii?Q?qTOx/CrfZtfqCwSor14GsbxPlglnN+1FcU4fjPXHCfrrSkmC21X3/Qoo6Plz?=
 =?us-ascii?Q?6TvXCDtaU3IRcfLOqNc7kAMvPekc2n6+fZsi+sBR4IC9OO6Mv6o4CREhSMsr?=
 =?us-ascii?Q?IRRnRP1z3e6CEtfrFPMFF+wyJaBf+Mu3jACZh0eO1NziPl/ayyV+L3GROyqI?=
 =?us-ascii?Q?hOS7CoKdlxBhvVNWZ93GVqnLxNf7v/AMoIgWgKJktrxEHPb1S7q/yOke0pJ3?=
 =?us-ascii?Q?0S1m+7W6rzQDJ/oUCKBMUElxUr/T+9MK2rVbXMiwfjklCbUGcy0BPaYUpBKP?=
 =?us-ascii?Q?Cdob3fo9rfXpnqzvgnh8+ZPIdrIAJk04Sf6EVrVEpXyWpjxYUqCs32V7ZxbT?=
 =?us-ascii?Q?TNkB9gFcKSvkqSUQiEIqT1GzX65Odm/lxFNz5oqh1o5n6jPcdipj6XxN0ksb?=
 =?us-ascii?Q?zXuMHuE+GaQlxREgaeE9niN/sn8nnfFNP6BLHOac51EQAkHpYauVDVtdvY5/?=
 =?us-ascii?Q?Em56yQSqIxvrPLikDCzi8pSJJGbUSWzhVcVTNMXbCucAioQwLCE/ztiK7vfQ?=
 =?us-ascii?Q?fBQraq6X0UNRM/D5j9CzPH82fLWcyBZryGp0Gz5N3a8I1kQtSHQL8ZGaf7rc?=
 =?us-ascii?Q?Emj6NA9wvKsMcxjFmtAlNCeOtGl29q4GsBnpmEU2Iul1Jiar9keIHd9gf+8o?=
 =?us-ascii?Q?RHtRBw3K7vZs5p/dWPBfK2mhv7otVbXJLRvKEqnvvaphXk+BVwCYV8oHfjul?=
 =?us-ascii?Q?xuTYcWsXQrRQrAFDUUaCa85RUmbnWqB5oofpldjeJqh+Q3s9Ekb+oO+fKldR?=
 =?us-ascii?Q?p7vTlt4ao2Ktxh/VDDxvMzmda/lUP9rXoJfuwYdmzdVZtCfaQXtyzZeDWylm?=
 =?us-ascii?Q?0Q=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2317593f-1a63-4e60-86df-08dc314423c4
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6117.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Feb 2024 12:13:10.8556
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: LI+0794D0d1WMK3eZQOz79gELSXQeSkmIfE6lmii1bJj/nCHqjJTJr3MSNIHG2fiA1DqZMECKaqQOqdT9BHbziQ0q21RXbI75oE/LNHB5MI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6392
X-OriginatorOrg: intel.com

On Mon, Feb 19, 2024 at 10:08:43AM +0100, Florian Kauer wrote:
> When a frame can not be transmitted in XDP_REDIRECT
> (e.g. due to a full queue), it is necessary to free
> it by calling xdp_return_frame_rx_napi.
> 
> However, this is the reponsibility of the caller of
> the ndo_xdp_xmit (see for example bq_xmit_all in
> kernel/bpf/devmap.c) and thus calling it inside
> igc_xdp_xmit (which is the ndo_xdp_xmit of the igc
> driver) as well will lead to memory corruption.
> 
> In fact, bq_xmit_all expects that it can return all
> frames after the last successfully transmitted one.
> Therefore, break for the first not transmitted frame,
> but do not call xdp_return_frame_rx_napi in igc_xdp_xmit.
> This is equally implemented in other Intel drivers
> such as the igb.
> 
> There are two alternatives to this that were rejected:
> 1. Return num_frames as all the frames would have been
>    transmitted and release them inside igc_xdp_xmit.
>    While it might work technically, it is not what
>    the return value is meant to repesent (i.e. the
>    number of SUCCESSFULLY transmitted packets).
> 2. Rework kernel/bpf/devmap.c and all drivers to
>    support non-consecutively dropped packets.
>    Besides being complex, it likely has a negative
>    performance impact without a significant gain
>    since it is anyway unlikely that the next frame
>    can be transmitted if the previous one was dropped.
> 
> The memory corruption can be reproduced with
> the following script which leads to a kernel panic
> after a few seconds.  It basically generates more
> traffic than a i225 NIC can transmit and pushes it
> via XDP_REDIRECT from a virtual interface to the
> physical interface where frames get dropped.
> 
>    #!/bin/bash
>    INTERFACE=enp4s0
>    INTERFACE_IDX=`cat /sys/class/net/$INTERFACE/ifindex`
> 
>    sudo ip link add dev veth1 type veth peer name veth2
>    sudo ip link set up $INTERFACE
>    sudo ip link set up veth1
>    sudo ip link set up veth2
> 
>    cat << EOF > redirect.bpf.c
> 
>    SEC("prog")
>    int redirect(struct xdp_md *ctx)
>    {
>        return bpf_redirect($INTERFACE_IDX, 0);
>    }
> 
>    char _license[] SEC("license") = "GPL";
>    EOF
>    clang -O2 -g -Wall -target bpf -c redirect.bpf.c -o redirect.bpf.o
>    sudo ip link set veth2 xdp obj redirect.bpf.o
> 
>    cat << EOF > pass.bpf.c
> 
>    SEC("prog")
>    int pass(struct xdp_md *ctx)
>    {
>        return XDP_PASS;
>    }
> 
>    char _license[] SEC("license") = "GPL";
>    EOF
>    clang -O2 -g -Wall -target bpf -c pass.bpf.c -o pass.bpf.o
>    sudo ip link set $INTERFACE xdp obj pass.bpf.o
> 
>    cat << EOF > trafgen.cfg
> 
>    {
>      /* Ethernet Header */
>      0xe8, 0x6a, 0x64, 0x41, 0xbf, 0x46,
>      0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
>      const16(ETH_P_IP),
> 
>      /* IPv4 Header */
>      0b01000101, 0,   # IPv4 version, IHL, TOS
>      const16(1028),   # IPv4 total length (UDP length + 20 bytes (IP header))
>      const16(2),      # IPv4 ident
>      0b01000000, 0,   # IPv4 flags, fragmentation off
>      64,              # IPv4 TTL
>      17,              # Protocol UDP
>      csumip(14, 33),  # IPv4 checksum
> 
>      /* UDP Header */
>      10,  0, 1, 1,    # IP Src - adapt as needed
>      10,  0, 1, 2,    # IP Dest - adapt as needed
>      const16(6666),   # UDP Src Port
>      const16(6666),   # UDP Dest Port
>      const16(1008),   # UDP length (UDP header 8 bytes + payload length)
>      csumudp(14, 34), # UDP checksum
> 
>      /* Payload */
>      fill('W', 1000),
>    }
>    EOF
> 
>    sudo trafgen -i trafgen.cfg -b3000MB -o veth1 --cpp
> 
> Fixes: 4ff320361092 ("igc: Add support for XDP_REDIRECT action")
> Signed-off-by: Florian Kauer <florian.kauer@linutronix.de>

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

Apparently fdc13979f91e ("bpf, devmap: Move drop error path to devmap for
XDP_REDIRECT") addressed all ndo_xdp_xmit callbacks but igc support was
added shortly after that...?

> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index ba8d3fe186ae..81c21a893ede 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -6487,7 +6487,7 @@ static int igc_xdp_xmit(struct net_device *dev, int num_frames,
>  	int cpu = smp_processor_id();
>  	struct netdev_queue *nq;
>  	struct igc_ring *ring;
> -	int i, drops;
> +	int i, nxmit;
>  
>  	if (unlikely(!netif_carrier_ok(dev)))
>  		return -ENETDOWN;
> @@ -6503,16 +6503,15 @@ static int igc_xdp_xmit(struct net_device *dev, int num_frames,
>  	/* Avoid transmit queue timeout since we share it with the slow path */
>  	txq_trans_cond_update(nq);
>  
> -	drops = 0;
> +	nxmit = 0;
>  	for (i = 0; i < num_frames; i++) {
>  		int err;
>  		struct xdp_frame *xdpf = frames[i];
>  
>  		err = igc_xdp_init_tx_descriptor(ring, xdpf);
> -		if (err) {
> -			xdp_return_frame_rx_napi(xdpf);
> -			drops++;
> -		}
> +		if (err)
> +			break;
> +		nxmit++;
>  	}
>  
>  	if (flags & XDP_XMIT_FLUSH)
> @@ -6520,7 +6519,7 @@ static int igc_xdp_xmit(struct net_device *dev, int num_frames,
>  
>  	__netif_tx_unlock(nq);
>  
> -	return num_frames - drops;
> +	return nxmit;
>  }
>  
>  static void igc_trigger_rxtxq_interrupt(struct igc_adapter *adapter,
> -- 
> 2.39.2
>