On Thu,  4 Nov 2021 18:35:33 +0100 Lorenzo Bianconi wrote:

quoted

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

?

Hi Jakub,

I am fine with the patch (just a typo inline), thx :)
I will let Eelco to comment since he wrote the original code.
If there is no objections, I will integrate it in v18.

Regards,
Lorenzo

quoted

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

I guess here should be:
			copy_off = off - ptr_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.