netdev - Re: [PATCH v17 bpf-next 13/23] bpf: add multi-buffer support to xdp copy helpers

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: <20211105162933.113ce3c5@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 5 Nov 2021 16:29:33 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Lorenzo Bianconi <lorenzo@...nel.org>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        lorenzo.bianconi@...hat.com, davem@...emloft.net, 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
Subject: Re: [PATCH v17 bpf-next 13/23] bpf: add multi-buffer support to xdp
 copy helpers

On Thu,  4 Nov 2021 18:35:33 +0100 Lorenzo Bianconi wrote:
> -static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
> +static unsigned long bpf_xdp_copy(void *dst_buff, const void *ctx,
>  				  unsigned long off, unsigned long len)
>  {
> -	memcpy(dst_buff, src_buff + off, len);
> +	unsigned long base_len, copy_len, frag_off_total;
> +	struct xdp_buff *xdp = (struct xdp_buff *)ctx;
> +	struct skb_shared_info *sinfo;
> +	int i;
> +
> +	if (likely(!xdp_buff_is_mb(xdp))) {

Would it be better to do

	if (xdp->data_end - xdp->data >= off + len)

?

> +		memcpy(dst_buff, xdp->data + off, len);
> +		return 0;
> +	}
> +
> +	base_len = xdp->data_end - xdp->data;
> +	frag_off_total = base_len;
> +	sinfo = xdp_get_shared_info_from_buff(xdp);
> +
> +	/* If we need to copy data from the base buffer do it */
> +	if (off < base_len) {
> +		copy_len = min(len, base_len - off);
> +		memcpy(dst_buff, xdp->data + off, copy_len);
> +
> +		off += copy_len;
> +		len -= copy_len;
> +		dst_buff += copy_len;
> +	}
> +
> +	/* Copy any remaining data from the fragments */
> +	for (i = 0; len && i < sinfo->nr_frags; i++) {
> +		skb_frag_t *frag = &sinfo->frags[i];
> +		unsigned long frag_len, frag_off;
> +
> +		frag_len = skb_frag_size(frag);
> +		frag_off = off - frag_off_total;
> +		if (frag_off < frag_len) {
> +			copy_len = min(len, frag_len - frag_off);
> +			memcpy(dst_buff,
> +			       skb_frag_address(frag) + frag_off, copy_len);
> +
> +			off += copy_len;
> +			len -= copy_len;
> +			dst_buff += copy_len;
> +		}
> +		frag_off_total += frag_len;
> +	}
> +

nit: can't help but feel that you can merge base copy and frag copy:

	sinfo = xdp_get_shared_info_from_buff(xdp);
	next_frag = &sinfo->frags[0];
	end_frag = &sinfo->frags[sinfo->nr_frags];

	ptr_off = 0;
	ptr_buf = xdp->data;
	ptr_len = xdp->data_end - xdp->data;

	while (true) {
		if (off < ptr_off + ptr_len) {
			copy_off = ptr_off - off;
			copy_len = min(len, ptr_len - copy_off);
			memcpy(dst_buff, ptr_buf + copy_off, copy_len);

			off += copy_len;
			len -= copy_len;
			dst_buff += copy_len;
		}

		if (!len || next_frag == end_frag)
			break;
	
		ptr_off += ptr_len;
		ptr_buf = skb_frag_address(next_frag);
		ptr_len = skb_frag_size(next_frag);
		next_frag++;
	}

Up to you.