> 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:
signature.asc
Description: PGP signature
