| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <540a850d-11f3-5d14-a9e7-0cf78e878b75@blackwall.org>
Date: Sun, 24 Apr 2022 17:53:14 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Cc: kuba@...nel.org, davem@...emloft.net, stable@...r.kernel.org,
Jason Wang <jasowang@...hat.com>,
Daniel Borkmann <daniel@...earbox.net>,
"Michael S. Tsirkin" <mst@...hat.com>,
virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org
Subject: Re: [PATCH net v2] virtio_net: fix wrong buf address calculation when
using xdp
On 24/04/2022 14:18, Xuan Zhuo wrote:
> On Sun, 24 Apr 2022 13:56:17 +0300, Nikolay Aleksandrov <razor@...ckwall.org> wrote:
>> On 24/04/2022 13:42, Xuan Zhuo wrote:
>>> On Sun, 24 Apr 2022 13:21:21 +0300, Nikolay Aleksandrov <razor@...ckwall.org> wrote:
[snip]
>>>>
>>>> CC: stable@...r.kernel.org
>>>> CC: Jason Wang <jasowang@...hat.com>
>>>> CC: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
>>>> CC: Daniel Borkmann <daniel@...earbox.net>
>>>> CC: "Michael S. Tsirkin" <mst@...hat.com>
>>>> CC: virtualization@...ts.linux-foundation.org
>>>> Fixes: 8fb7da9e9907 ("virtio_net: get build_skb() buf by data ptr")
>>>> Signed-off-by: Nikolay Aleksandrov <razor@...ckwall.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
>>>>
>>
Powered by blists - more mailing lists