linux-kernel - Re: [PATCH net 1/2] amd-xgbe: Fix data loss when RX checksum offloading is disabled

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <22ddcfd9-d3b8-204b-6ada-7f519143a261@amd.com>
Date: Mon, 14 Apr 2025 09:34:13 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Vishal Badole <Vishal.Badole@....com>, Shyam-sundar.S-k@....com,
 andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Cc: stable@...r.kernel.org
Subject: Re: [PATCH net 1/2] amd-xgbe: Fix data loss when RX checksum
 offloading is disabled

On 4/14/25 07:02, Vishal Badole wrote:
> To properly disable checksum offloading, the split header mode must also
> be disabled. When split header mode is disabled, the network device stores
> received packets (with size <= 1536 bytes) entirely in buffer1, leaving
> buffer2 empty. However, with the current DMA configuration, only 256 bytes
> from buffer1 are copied from the network device to system memory,
> resulting in the loss of the remaining packet data.
> 
> Address the issue by programming the ARBS field to 256 bytes, which aligns
> with the socket buffer size, and setting the SPH bit in the control
> register to disable split header mode. With this configuration, the
> network device stores the first 256 bytes of the received packet in
> buffer1 and the remaining data in buffer2. The DMA is then able to
> transfer the full packet from the network device to system memory without
> any data loss.
> 
> Cc: stable@...r.kernel.org
> Fixes: c5aa9e3b8156 ("amd-xgbe: Initial AMD 10GbE platform driver")

Arguably, this patch doesn't fix anything as all you've done is defined
some routines that aren't called by anything yet. You can probably
combine this with the actual fix in the next patch and make this a
single patch.

Split-header support wasn't added until commit 174fd2597b0b ("amd-xgbe:
Implement split header receive support") and receive side scaling wasn't
added until commit 5b9dfe299e55 ("amd-xgbe: Provide support for receive
side scaling"), so you'll want to double check your Fixes: tag.

Also, the ARBS field wasn't always present, so you might need to
investigate that this is truly backwards compatible and earlier versions
don't require a different fix.

Thanks,
Tom

> Signed-off-by: Vishal Badole <Vishal.Badole@....com>
> ---
>  drivers/net/ethernet/amd/xgbe/xgbe-common.h |  2 ++
>  drivers/net/ethernet/amd/xgbe/xgbe-dev.c    | 18 ++++++++++++++++++
>  drivers/net/ethernet/amd/xgbe/xgbe.h        |  5 +++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-common.h b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
> index bcb221f74875..d92453ee2505 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-common.h
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
> @@ -232,6 +232,8 @@
>  #define DMA_CH_IER_TIE_WIDTH		1
>  #define DMA_CH_IER_TXSE_INDEX		1
>  #define DMA_CH_IER_TXSE_WIDTH		1
> +#define DMA_CH_RCR_ARBS_INDEX		28
> +#define DMA_CH_RCR_ARBS_WIDTH		3
>  #define DMA_CH_RCR_PBL_INDEX		16
>  #define DMA_CH_RCR_PBL_WIDTH		6
>  #define DMA_CH_RCR_RBSZ_INDEX		1
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> index 7a923b6e83df..429c5e1444d8 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> @@ -292,6 +292,8 @@ static void xgbe_config_rx_buffer_size(struct xgbe_prv_data *pdata)
>  
>  		XGMAC_DMA_IOWRITE_BITS(pdata->channel[i], DMA_CH_RCR, RBSZ,
>  				       pdata->rx_buf_size);
> +		XGMAC_DMA_IOWRITE_BITS(pdata->channel[i], DMA_CH_RCR, ARBS,
> +				       XGBE_ARBS_SIZE);
>  	}
>  }
>  
> @@ -321,6 +323,18 @@ static void xgbe_config_sph_mode(struct xgbe_prv_data *pdata)
>  	XGMAC_IOWRITE_BITS(pdata, MAC_RCR, HDSMS, XGBE_SPH_HDSMS_SIZE);
>  }
>  
> +static void xgbe_disable_sph_mode(struct xgbe_prv_data *pdata)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < pdata->channel_count; i++) {
> +		if (!pdata->channel[i]->rx_ring)
> +			break;
> +
> +		XGMAC_DMA_IOWRITE_BITS(pdata->channel[i], DMA_CH_CR, SPH, 0);
> +	}
> +}
> +
>  static int xgbe_write_rss_reg(struct xgbe_prv_data *pdata, unsigned int type,
>  			      unsigned int index, unsigned int val)
>  {
> @@ -3910,5 +3924,9 @@ void xgbe_init_function_ptrs_dev(struct xgbe_hw_if *hw_if)
>  	hw_if->disable_vxlan = xgbe_disable_vxlan;
>  	hw_if->set_vxlan_id = xgbe_set_vxlan_id;
>  
> +	/* For Split Header*/
> +	hw_if->enable_sph = xgbe_config_sph_mode;
> +	hw_if->disable_sph = xgbe_disable_sph_mode;
> +
>  	DBGPR("<--xgbe_init_function_ptrs\n");
>  }
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
> index db73c8f8b139..1b9c679453fb 100755
> --- a/drivers/net/ethernet/amd/xgbe/xgbe.h
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
> @@ -166,6 +166,7 @@
>  #define XGBE_RX_BUF_ALIGN	64
>  #define XGBE_SKB_ALLOC_SIZE	256
>  #define XGBE_SPH_HDSMS_SIZE	2	/* Keep in sync with SKB_ALLOC_SIZE */
> +#define XGBE_ARBS_SIZE	        3
>  
>  #define XGBE_MAX_DMA_CHANNELS	16
>  #define XGBE_MAX_QUEUES		16
> @@ -902,6 +903,10 @@ struct xgbe_hw_if {
>  	void (*enable_vxlan)(struct xgbe_prv_data *);
>  	void (*disable_vxlan)(struct xgbe_prv_data *);
>  	void (*set_vxlan_id)(struct xgbe_prv_data *);
> +
> +	/* For Split Header */
> +	void (*enable_sph)(struct xgbe_prv_data *pdata);
> +	void (*disable_sph)(struct xgbe_prv_data *pdata);
>  };
>  
>  /* This structure represents implementation specific routines for an