netdev - Re: [PATCH net 1/3] dpaa_eth: reserve space for the xdp

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: <20210204085158.GA2580@ranger.igk.intel.com>
Date:   Thu, 4 Feb 2021 09:51:58 +0100
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     Camelia Groza <camelia.groza@....com>
Cc:     kuba@...nel.org, davem@...emloft.net, madalin.bucur@....nxp.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net 1/3] dpaa_eth: reserve space for the xdp_frame under
 the A050385 erratum

On Tue, Feb 02, 2021 at 07:34:42PM +0200, Camelia Groza wrote:
> When the erratum workaround is triggered, the newly created xdp_frame
> structure is stored at the start of the newly allocated buffer. Avoid
> the structure from being overwritten by explicitly reserving enough
> space in the buffer for storing it.
> 
> Account for the fact that the structure's size might increase in time by
> aligning the headroom to DPAA_FD_DATA_ALIGNMENT bytes, thus guaranteeing
> the data's alignment.
> 
> Fixes: ae680bcbd06a ("dpaa_eth: implement the A050385 erratum workaround for XDP")
> Signed-off-by: Camelia Groza <camelia.groza@....com>
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 4360ce4d3fb6..e1d041c35ad9 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2182,6 +2182,7 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
>  	struct xdp_frame *new_xdpf, *xdpf = *init_xdpf;
>  	void *new_buff;
>  	struct page *p;
> +	int headroom;
>  
>  	/* Check the data alignment and make sure the headroom is large
>  	 * enough to store the xdpf backpointer. Use an aligned headroom
> @@ -2197,19 +2198,31 @@ static int dpaa_a050385_wa_xdpf(struct dpaa_priv *priv,
>  		return 0;
>  	}
>  
> +	/* The new xdp_frame is stored in the new buffer. Reserve enough space
> +	 * in the headroom for storing it along with the driver's private
> +	 * info. The headroom needs to be aligned to DPAA_FD_DATA_ALIGNMENT to
> +	 * guarantee the data's alignment in the buffer.
> +	 */
> +	headroom = ALIGN(sizeof(*new_xdpf) + priv->tx_headroom,
> +			 DPAA_FD_DATA_ALIGNMENT);
> +
> +	/* Assure the extended headroom and data fit in a one-paged buffer */
> +	if (headroom + xdpf->len > DPAA_BP_RAW_SIZE)

This check might make more sense if you would be accounting for
skb_shared_info as well I suppose, so that you know you'll still provide
enough tailroom for future xdp multibuf support. Didn't all the previous
code path make sure that there's a room for that?

> +		return -ENOMEM;
> +
>  	p = dev_alloc_pages(0);
>  	if (unlikely(!p))
>  		return -ENOMEM;
>  
>  	/* Copy the data to the new buffer at a properly aligned offset */
>  	new_buff = page_address(p);
> -	memcpy(new_buff + priv->tx_headroom, xdpf->data, xdpf->len);
> +	memcpy(new_buff + headroom, xdpf->data, xdpf->len);
>  
>  	/* Create an XDP frame around the new buffer in a similar fashion
>  	 * to xdp_convert_buff_to_frame.
>  	 */
>  	new_xdpf = new_buff;
> -	new_xdpf->data = new_buff + priv->tx_headroom;
> +	new_xdpf->data = new_buff + headroom;
>  	new_xdpf->len = xdpf->len;
>  	new_xdpf->headroom = priv->tx_headroom;
>  	new_xdpf->frame_sz = DPAA_BP_RAW_SIZE;
> -- 
> 2.17.1
>