netdev - Re: [PATCH v2 net-next 3/9] net: mvneta: update mb bit before passing the xdp buffer to eBPF layer

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 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]

Date:   Sun, 6 Sep 2020 10:33:16 +0300
From:   Shay Agroskin <shayagr@...zon.com>
To:     Lorenzo Bianconi <lorenzo@...nel.org>
CC:     <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
        <davem@...emloft.net>, <lorenzo.bianconi@...hat.com>,
        <brouer@...hat.com>, <echaudro@...hat.com>, <sameehj@...zon.com>,
        <kuba@...nel.org>, <john.fastabend@...il.com>,
        <daniel@...earbox.net>, <ast@...nel.org>
Subject: Re: [PATCH v2 net-next 3/9] net: mvneta: update mb bit before passing the xdp buffer to eBPF layer


Lorenzo Bianconi <lorenzo@...nel.org> writes:

> Update multi-buffer bit (mb) in xdp_buff to notify XDP/eBPF 
> layer and
> XDP remote drivers if this is a "non-linear" XDP buffer. Access
> skb_shared_info only if xdp_buff mb is set
>
> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 37 
>  +++++++++++++++------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvneta.c 
> b/drivers/net/ethernet/marvell/mvneta.c
> index 832bbb8b05c8..4f745a2b702a 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -2027,11 +2027,11 @@ mvneta_xdp_put_buff(struct mvneta_port 
> *pp, struct mvneta_rx_queue *rxq,
>  		    struct xdp_buff *xdp, int sync_len, bool napi)
>  {
>  	struct skb_shared_info *sinfo = 
>  xdp_get_shared_info_from_buff(xdp);
> -	int i;
> +	int i, num_frames = xdp->mb ? sinfo->nr_frags : 0;
>  
>  	page_pool_put_page(rxq->page_pool, 
>  virt_to_head_page(xdp->data),
>  			   sync_len, napi);
> -	for (i = 0; i < sinfo->nr_frags; i++)
> +	for (i = 0; i < num_frames; i++)
>  		page_pool_put_full_page(rxq->page_pool,
>  					skb_frag_page(&sinfo->frags[i]), 
>  napi);
>  }
> ...
> @@ -2175,6 +2175,7 @@ mvneta_run_xdp(struct mvneta_port *pp, 
> struct mvneta_rx_queue *rxq,
>  
>  	len = xdp->data_end - xdp->data_hard_start - 
>  pp->rx_offset_correction;
>  	data_len = xdp->data_end - xdp->data;
> -	sinfo = xdp_get_shared_info_from_buff(xdp);
> -	sinfo->nr_frags = 0;
> +	xdp->mb = 0;
>  
>  	*size = rx_desc->data_size - len;
>  	rx_desc->buf_phys_addr = 0;
> @@ -2269,7 +2267,7 @@ mvneta_swbm_add_rx_fragment(struct 
> mvneta_port *pp,
>  			    struct mvneta_rx_desc *rx_desc,
>  			    struct mvneta_rx_queue *rxq,
>  			    struct xdp_buff *xdp, int *size,
> -			    struct page *page)
> +			    int *nfrags, struct page *page)
>  {
>  	struct skb_shared_info *sinfo = 
>  xdp_get_shared_info_from_buff(xdp);
>  	struct net_device *dev = pp->dev;
> @@ -2288,13 +2286,18 @@ mvneta_swbm_add_rx_fragment(struct 
> mvneta_port *pp,
>  				rx_desc->buf_phys_addr,
>  				len, dma_dir);
>  
> -	if (data_len > 0 && sinfo->nr_frags < MAX_SKB_FRAGS) {
> -		skb_frag_t *frag = &sinfo->frags[sinfo->nr_frags];
> +	if (data_len > 0 && *nfrags < MAX_SKB_FRAGS) {
> +		skb_frag_t *frag = &sinfo->frags[*nfrags];
>  
>  		skb_frag_off_set(frag, pp->rx_offset_correction);
>  		skb_frag_size_set(frag, data_len);
>  		__skb_frag_set_page(frag, page);
> -		sinfo->nr_frags++;
> +		*nfrags = *nfrags + 1;

nit, why do you use nfrags variable instead of sinfo->nr_frags (as 
it was before this patch) ?
                You doesn't seem to use the nfrags variable in the 
                caller function and you update nr_frags as well.
                If it's used in a different patch (haven't 
                reviewed it all yet), maybe move it to that patch 
                ? (sorry in advance if I missed the logic here)

> +
> +		if (rx_desc->status & MVNETA_RXD_LAST_DESC) {
> +			sinfo->nr_frags = *nfrags;
> +			xdp->mb = true;
> +		}
>  
>  		rx_desc->buf_phys_addr = 0;
>               ...