Re: [PATCH v18 bpf-next 20/23] net: xdp: introduce bpf_xdp_pointer utility routine

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3300 bytes --]

> On Mon, 15 Nov 2021 23:33:14 +0100 Lorenzo Bianconi wrote:
> > +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > +	u32 headsize = xdp->data_end - xdp->data;
> > +	u32 count = 0, frame_offset = headsize;
> > +	int i;
> > +
> > +	if (offset < headsize) {
> > +		int size = min_t(int, headsize - offset, len);
> > +		void *src = flush ? buf : xdp->data + offset;
> > +		void *dst = flush ? xdp->data + offset : buf;
> > +
> > +		memcpy(dst, src, size);
> > +		count = size;
> > +		offset = 0;
> > +	}
> 
> is this missing
> 	else
> 		offset -= headsize;
> ?
> 
> I'm struggling to understand this. Say
> 	headsize = 400
> 	frag[0].size = 200
> 
> 	offset = 500
> 	len = 50
> 
> we enter the loop having missed the previous if...
> 
> > +	for (i = 0; i < sinfo->nr_frags; i++) {
> > +		skb_frag_t *frag = &sinfo->frags[i];
> > +		u32 frag_size = skb_frag_size(frag);
> > +
> > +		if (count >= len)
> > +			break;
> > +
> > +		if (offset < frame_offset + frag_size) {
> 
> 		500 < 400 + 200 => true
> 
> > +			int size = min_t(int, frag_size - offset, len - count);
> 
> 			size = min(200 - 500, 50 - 0)
> 			size = -300 ??

ack, you are right. Sorry for the issue.
I did not trigger the problem with xdp-mb self-tests since we will not run
bpf_xdp_copy_buf() in this specific case, but just the memcpy()
(but what you reported is a bug and must be fixed). I will add more
self-tests.
Moreover, reviewing the code I guess we can just update bpf_xdp_copy() for our case.
Something like:

static void bpf_xdp_copy_buf(struct xdp_buff *xdp, unsigned long off,
			     void *buf, unsigned long len, bool flush)
{
	unsigned long ptr_len, ptr_off = 0;
	skb_frag_t *next_frag, *end_frag;
	struct skb_shared_info *sinfo;
	void *src, *dst;
	u8 *ptr_buf;

	if (likely(xdp->data_end - xdp->data >= off + len)) {
		src = flush ? buf : xdp->data + off;
		dst = flush ? xdp->data + off : buf;
		memcpy(dst, src, len);
		return;
	}

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

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

	while (true) {
		if (off < ptr_off + ptr_len) {
			unsigned long copy_off = off - ptr_off;
			unsigned long copy_len = min(len, ptr_len - copy_off);

			src = flush ? buf : ptr_buf + copy_off;
			dst = flush ? ptr_buf + copy_off : buf;
			memcpy(dst, src, copy_len);

			off += copy_len;
			len -= copy_len;
			buf += 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++;
	}
}

...

static unsigned long bpf_xdp_copy(void *dst, const void *ctx,
				  unsigned long off, unsigned long len)
{
	struct xdp_buff *xdp = (struct xdp_buff *)ctx;

	bpf_xdp_copy_buf(xdp, off, dst, len, false);
	return 0;
}

What do you think?

Regards,
Lorenzo


> 
> > +			void *addr = skb_frag_address(frag);
> > +			void *src = flush ? buf + count : addr + offset;
> > +			void *dst = flush ? addr + offset : buf + count;
> > +
> > +			memcpy(dst, src, size);
> > +			count += size;
> > +			offset = 0;
> > +		}
> > +		frame_offset += frag_size;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]