On 24/04/2022 14:18, Xuan Zhuo wrote:
On Sun, 24 Apr 2022 13:56:17 +0300, Nikolay Aleksandrov razor@blackwall.org wrote:
On 24/04/2022 13:42, Xuan Zhuo wrote:
On Sun, 24 Apr 2022 13:21:21 +0300, Nikolay Aleksandrov razor@blackwall.org wrote:
[snip]
CC: stable@vger.kernel.org CC: Jason Wang jasowang@redhat.com CC: Xuan Zhuo xuanzhuo@linux.alibaba.com CC: Daniel Borkmann daniel@iogearbox.net CC: "Michael S. Tsirkin" mst@redhat.com CC: virtualization@lists.linux-foundation.org Fixes: 8fb7da9e9907 ("virtio_net: get build_skb() buf by data ptr") Signed-off-by: Nikolay Aleksandrov razor@blackwall.org
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