Re: [PATCH net v2] virtio_net: fix wrong buf address calculation when using xdp

[Nikolay Aleksandrov <razor@blackwall.org>]

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