netdev - Re: [PATCH v21 bpf-next 06/23] net: marvell: rely on xdp_update_skb_shared

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YdxgrP1YDMyWXmqL@C02YVCJELVCG>
Date:   Mon, 10 Jan 2022 11:37:00 -0500
From:   Andy Gospodarek <andrew.gospodarek@...adcom.com>
To:     Lorenzo Bianconi <lorenzo@...nel.org>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        lorenzo.bianconi@...hat.com, davem@...emloft.net, kuba@...nel.org,
        ast@...nel.org, daniel@...earbox.net, shayagr@...zon.com,
        john.fastabend@...il.com, dsahern@...nel.org, brouer@...hat.com,
        echaudro@...hat.com, jasowang@...hat.com,
        alexander.duyck@...il.com, saeed@...nel.org,
        maciej.fijalkowski@...el.com, magnus.karlsson@...el.com,
        tirthendu.sarkar@...el.com, toke@...hat.com, andy@...yhouse.net
Subject: Re: [PATCH v21 bpf-next 06/23] net: marvell: rely on
 xdp_update_skb_shared_info utility routine

On Sat, Jan 08, 2022 at 12:53:09PM +0100, Lorenzo Bianconi wrote:
> Rely on xdp_update_skb_shared_info routine in order to avoid
> resetting frags array in skb_shared_info structure building
> the skb in mvneta_swbm_build_skb(). Frags array is expected to
> be initialized by the receiving driver building the xdp_buff
> and here we just need to update memory metadata.
> 
> Acked-by: Toke Hoiland-Jorgensen <toke@...hat.com>
> Acked-by: John Fastabend <john.fastabend@...il.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> ---
>  drivers/net/ethernet/marvell/mvneta.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 775ffd91b741..267a306d9c75 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -2332,8 +2332,12 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
>  		skb_frag_size_set(frag, data_len);
>  		__skb_frag_set_page(frag, page);
>  
> -		if (!xdp_buff_is_mb(xdp))
> +		if (!xdp_buff_is_mb(xdp)) {
> +			sinfo->xdp_frags_size = *size;
>  			xdp_buff_set_mb(xdp);
> +		}
> +		if (page_is_pfmemalloc(page))
> +			xdp_buff_set_frag_pfmemalloc(xdp);
>  	} else {
>  		page_pool_put_full_page(rxq->page_pool, page, true);
>  	}
> @@ -2347,7 +2351,6 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
>  	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
>  	struct sk_buff *skb;
>  	u8 num_frags;
> -	int i;
>  
>  	if (unlikely(xdp_buff_is_mb(xdp)))
>  		num_frags = sinfo->nr_frags;
> @@ -2362,18 +2365,12 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
>  	skb_put(skb, xdp->data_end - xdp->data);
>  	skb->ip_summed = mvneta_rx_csum(pp, desc_status);
>  
> -	if (likely(!xdp_buff_is_mb(xdp)))
> -		goto out;
> -
> -	for (i = 0; i < num_frags; i++) {
> -		skb_frag_t *frag = &sinfo->frags[i];
> -
> -		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> -				skb_frag_page(frag), skb_frag_off(frag),
> -				skb_frag_size(frag), PAGE_SIZE);

Maybe I'm missing something but I'm not sure you have a suitable
replacement for the 3 lines above this in your proposed change.

> -	}
> +	if (unlikely(xdp_buff_is_mb(xdp)))
> +		xdp_update_skb_shared_info(skb, num_frags,
> +					   sinfo->xdp_frags_size,
> +					   num_frags * xdp->frame_sz,
> +					   xdp_buff_is_frag_pfmemalloc(xdp));
>  

When I did an implementation of this on a different driver I also needed
to add:

	for (i = 0; i < num_frags; i++)
		skb_frag_set_page(skb, i, skb_frag_page(&sinfo->frags[i]));

to make sure that frames that were given XDP_PASS were formatted
correctly so they could be handled by the stack.  Don't you need
something similar to make sure frags are properly set?

Thanks,

-andy

P.S.  Sorry for noticing this so late in the process; I realize this version
was just a rebase of v20 and this would have been useful information
earlier if I'm correct.

> -out:
>  	return skb;
>  }
>  
> -- 
> 2.33.1
> 

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4222 bytes)