On 24/04/2022 14:18, Xuan Zhuo wrote: > On Sun, 24 Apr 2022 13:56:17 +0300, Nikolay Aleksandrov <razor@xxxxxxxxxxxxx> wrote: >> On 24/04/2022 13:42, Xuan Zhuo wrote: >>> On Sun, 24 Apr 2022 13:21:21 +0300, Nikolay Aleksandrov <razor@xxxxxxxxxxxxx> wrote: [snip] >>>> >>>> CC: stable@xxxxxxxxxxxxxxx >>>> CC: Jason Wang <jasowang@xxxxxxxxxx> >>>> CC: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> >>>> CC: Daniel Borkmann <daniel@xxxxxxxxxxxxx> >>>> CC: "Michael S. Tsirkin" <mst@xxxxxxxxxx> >>>> CC: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx >>>> Fixes: 8fb7da9e9907 ("virtio_net: get build_skb() buf by data ptr") >>>> Signed-off-by: Nikolay Aleksandrov <razor@xxxxxxxxxxxxx> >>>> --- >>>> v2: Recalculate headroom based on data, data_hard_start and data_meta >>>> >>>> drivers/net/virtio_net.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>>> index 87838cbe38cf..a12338de7ef1 100644 >>>> --- a/drivers/net/virtio_net.c >>>> +++ b/drivers/net/virtio_net.c >>>> @@ -1005,6 +1005,12 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, >>>> * xdp.data_meta were adjusted >>>> */ >>>> len = xdp.data_end - xdp.data + vi->hdr_len + metasize; >>>> + >>>> + /* recalculate headroom if xdp.data or xdp.data_meta >>>> + * were adjusted >>>> + */ >>>> + headroom = xdp.data - xdp.data_hard_start - metasize; >>> >>> >>> This is incorrect. >>> >>> >>> data = page_address(xdp_page) + offset; >>> xdp_init_buff(&xdp, frame_sz - vi->hdr_len, &rq->xdp_rxq); >>> xdp_prepare_buff(&xdp, data - VIRTIO_XDP_HEADROOM + vi->hdr_len, >>> VIRTIO_XDP_HEADROOM, len - vi->hdr_len, true); >>> >>> so: xdp.data_hard_start = page_address(xdp_page) + offset - VIRTIO_XDP_HEADROOM + vi->hdr_len >>> >>> (page_address(xdp_page) + offset) points to virtio-net hdr. >>> (page_address(xdp_page) + offset - VIRTIO_XDP_HEADROOM) points to the allocated buf. >>> >>> xdp.data_hard_start points to buf + vi->hdr_len >>> >>> Thanks. >>> >> >> xdp.data points to buf + vi->hdr_len + VIRTIO_XDP_HEADROOM, so we calculate >> xdp.data - xdp.data_hard_start, i.e. buf + vi->hdr_len + VIRTIO_XDP_HEADROOM - (buf + vi->hdr_len) >> >> You can see the headrooms from my tests above, they are correct and they match exactly >> the values from the headroom calculations that you suggested earlier. > > > OK. You are right, xdp.data, xdp.data_hard_start have an offset of hdr_len. I > hope this can be explained in the comments, because the headroom we want to get > is virtio_hdr - buf. Although the value here are equal. > I think it's normal for them to be equal because buf + offset points to vnet_hdr start, that is it doesn't include the vnet_hdr size, so all that is left to subtract to get to buf is offset - headroom after the prepare (given correct headroom, of course). The linearized xdp page buf has the following initial geometry (at prepare): offset = VIRTIO_XDP_HEADROOM headroom = VIRTIO_XDP_HEADROOM data_hard_start = page_address + vnet_hdr data = page_address + vnet_hdr + headroom data_meta = data after xdp prog has run: offset is recalculated as: (page_address + vnet_hdr + adjusted headroom) - page_address - vnet_hdr - metasize = adjusted headroom - metasize I wrote adjusted headroom because it depends on data and data_meta adjustments done by the xdp prog, so based on the above we can get back to page_address (or buf if it's a virtnet buf) by subtracting the following headroom recalculation: headroom = data - data_hard_start - metasize = (page_address + vnet_hdr + adjusted headroom) - page_address - vnet_hdr - metasize = adjusted headroom - metasize That is because in page_to_skb() p points to page_address + recalculated offset, i.e. p = page_address + (adjusted headroom - metasize) for the linearized case. For the other case it's the same just the initial offset is a larger value. I'll add a comment that page_address + offset always points to vnet_hdr start so it will be equal to headroom which is data - data_hard_start because we have data = page_address + vnet_hdr + adjusted headroom and data_hard_start at page_address + vnet_hdr. Note that we have adjusted headroom + vnet_hdr bytes available behind data always, so for offset to point to vnet_hdr it has to be equal to headroom, it just starts at page_address while the actual headroom starts at page_address + vnet_hdr to reserve that many bytes. I'll see how I can make that more concise. :) > In addition, if you are going to post v2, I think you should post a new thread > separately instead of replying in the previous thread. > sure, I don't mind either way > Thanks. > > Cheers, Nik >> >>> >>>> + >>>> /* We can only create skb based on xdp_page. */ >>>> if (unlikely(xdp_page != page)) { >>>> rcu_read_unlock(); >>>> @@ -1012,7 +1018,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev, >>>> head_skb = page_to_skb(vi, rq, xdp_page, offset, >>>> len, PAGE_SIZE, false, >>>> metasize, >>>> - VIRTIO_XDP_HEADROOM); >>>> + headroom); >>>> return head_skb; >>>> } >>>> break; >>>> -- >>>> 2.35.1 >>>> >>