[bpf-next] xsk: fix destination buffer address when copying with metadata

Commit Message

Stanislav Fomichev Nov. 4, 2022, 3:23 a.m. UTC

While working on a simplified test for [0] it occurred to me that
the following looks fishy:

	data = xsk_umem__get_data(xsk->umem_area, rx_desc->addr);
	data_meta = data - sizeof(my metadata);

Since the data points to umem frame at addr X, data_mem points to
the end of umem frame X-1.

I don't think it's by design?

0: https://lore.kernel.org/bpf/[e-mail]/T/#t

Cc: Björn Töpel <[e-mail]>
Cc: Magnus Karlsson <[e-mail]>
Cc: Maciej Fijalkowski <[e-mail]>
Cc: Jonathan Lemon <[e-mail]>
Signed-off-by: Stanislav Fomichev <[e-mail]>
---
 net/xdp/xsk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Björn Töpel Nov. 4, 2022, 8:22 a.m. UTC | #1

Stanislav Fomichev <[e-mail]> writes:

> While working on a simplified test for [0] it occurred to me that
> the following looks fishy:
>
> 	data = xsk_umem__get_data(xsk->umem_area, rx_desc->addr);
> 	data_meta = data - sizeof(my metadata);
>
> Since the data points to umem frame at addr X, data_mem points to
> the end of umem frame X-1.
>
> I don't think it's by design?

It is by design. :-)

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 9f0561b67c12..0547fe37ba7e 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -163,7 +163,7 @@ static void xsk_copy_xdp(struct xdp_buff *to, struct xdp_buff *from, u32 len)
>  	} else {
>  		from_buf = from->data_meta;
>  		metalen = from->data - from->data_meta;
> -		to_buf = to->data - metalen;

This is to include the XDP meta data in the receive buffer. Note that
AF_XDP descriptor that you get back on the RX ring points to the *data*
not the metadata.

For the unaligned mode you can pass any address (umem offset) into the
fill ring, and the kernel will simply mask it and setup headroom
accordingly.

The buffer allocator guarantees that there's XDP_PACKET_HEADROOM
available.

IOW your example userland code above is correct.


Björn

Stanislav Fomichev Nov. 4, 2022, 6:21 p.m. UTC | #2

On Fri, Nov 4, 2022 at 1:22 AM Björn Töpel <[e-mail]> wrote:
>
> Stanislav Fomichev <[e-mail]> writes:
>
> > While working on a simplified test for [0] it occurred to me that
> > the following looks fishy:
> >
> >       data = xsk_umem__get_data(xsk->umem_area, rx_desc->addr);
> >       data_meta = data - sizeof(my metadata);
> >
> > Since the data points to umem frame at addr X, data_mem points to
> > the end of umem frame X-1.
> >
> > I don't think it's by design?
>
> It is by design. :-)

Noted, thanks for clarifying!

> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 9f0561b67c12..0547fe37ba7e 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -163,7 +163,7 @@ static void xsk_copy_xdp(struct xdp_buff *to, struct xdp_buff *from, u32 len)
> >       } else {
> >               from_buf = from->data_meta;
> >               metalen = from->data - from->data_meta;
> > -             to_buf = to->data - metalen;
>
> This is to include the XDP meta data in the receive buffer. Note that
> AF_XDP descriptor that you get back on the RX ring points to the *data*
> not the metadata.
>
> For the unaligned mode you can pass any address (umem offset) into the
> fill ring, and the kernel will simply mask it and setup headroom
> accordingly.

Thanks for the details! And what happens in the aligned case?

Looking purely from the user side:

tx_desc = xsk_ring_prod__tx_desc(&xsk->tx, idx);
tx_desc->addr = idx * UMEM_FRAME_SIZE; /* this has to be aligned to
the frame size? */
data = xsk_umem__get_data(xsk->umem_area, tx_desc->addr);

data here is basically = umem_area + idx * UMEM_FRAM_SIZE, right? How
do I make sure metadata is placed in the same umem chunk? Will passing
umem headroom do the trick?




> The buffer allocator guarantees that there's XDP_PACKET_HEADROOM
> available.
>
> IOW your example userland code above is correct.
>
>
> Björn
>
>

Stanislav Fomichev Nov. 4, 2022, 9:47 p.m. UTC | #3

On Fri, Nov 4, 2022 at 11:21 AM Stanislav Fomichev <[e-mail]> wrote:
>
> On Fri, Nov 4, 2022 at 1:22 AM Björn Töpel <[e-mail]> wrote:
> >
> > Stanislav Fomichev <[e-mail]> writes:
> >
> > > While working on a simplified test for [0] it occurred to me that
> > > the following looks fishy:
> > >
> > >       data = xsk_umem__get_data(xsk->umem_area, rx_desc->addr);
> > >       data_meta = data - sizeof(my metadata);
> > >
> > > Since the data points to umem frame at addr X, data_mem points to
> > > the end of umem frame X-1.
> > >
> > > I don't think it's by design?
> >
> > It is by design. :-)
>
> Noted, thanks for clarifying!
>
> > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > index 9f0561b67c12..0547fe37ba7e 100644
> > > --- a/net/xdp/xsk.c
> > > +++ b/net/xdp/xsk.c
> > > @@ -163,7 +163,7 @@ static void xsk_copy_xdp(struct xdp_buff *to, struct xdp_buff *from, u32 len)
> > >       } else {
> > >               from_buf = from->data_meta;
> > >               metalen = from->data - from->data_meta;
> > > -             to_buf = to->data - metalen;
> >
> > This is to include the XDP meta data in the receive buffer. Note that
> > AF_XDP descriptor that you get back on the RX ring points to the *data*
> > not the metadata.
> >
> > For the unaligned mode you can pass any address (umem offset) into the
> > fill ring, and the kernel will simply mask it and setup headroom
> > accordingly.
>
> Thanks for the details! And what happens in the aligned case?
>
> Looking purely from the user side:
>
> tx_desc = xsk_ring_prod__tx_desc(&xsk->tx, idx);
> tx_desc->addr = idx * UMEM_FRAME_SIZE; /* this has to be aligned to
> the frame size? */
> data = xsk_umem__get_data(xsk->umem_area, tx_desc->addr);
>
> data here is basically = umem_area + idx * UMEM_FRAM_SIZE, right? How
> do I make sure metadata is placed in the same umem chunk? Will passing
> umem headroom do the trick?

Ignore me. I do see now that there is always XDP_PACKET_HEADROOM bytes
of headroom in rx_desc.



>
> > The buffer allocator guarantees that there's XDP_PACKET_HEADROOM
> > available.
> >
> > IOW your example userland code above is correct.
> >
> >
> > Björn
> >
> >

Patch

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 9f0561b67c12..0547fe37ba7e 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -163,7 +163,7 @@  static void xsk_copy_xdp(struct xdp_buff *to, struct xdp_buff *from, u32 len)
 	} else {
 		from_buf = from->data_meta;
 		metalen = from->data - from->data_meta;
-		to_buf = to->data - metalen;
+		to_buf = to->data;
 	}
 
 	memcpy(to_buf, from_buf, len + metalen);