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

On Mon, 15 Nov 2021 23:33:14 +0100 Lorenzo Bianconi wrote:

quoted

+	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...

quoted

+	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

quoted

+			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

quoted

+			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;