| 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: <77c21953-b850-4962-8673-6effb593d819@daynix.com>
Date: Wed, 12 Mar 2025 14:59:36 +0900
From: Akihiko Odaki <akihiko.odaki@...nix.com>
To: Jason Wang <jasowang@...hat.com>
Cc: Jonathan Corbet <corbet@....net>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
"Michael S. Tsirkin" <mst@...hat.com>, Xuan Zhuo
<xuanzhuo@...ux.alibaba.com>, Shuah Khan <shuah@...nel.org>,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, kvm@...r.kernel.org,
virtualization@...ts.linux-foundation.org, linux-kselftest@...r.kernel.org,
Yuri Benditovich <yuri.benditovich@...nix.com>,
Andrew Melnychenko <andrew@...nix.com>,
Stephen Hemminger <stephen@...workplumber.org>, gur.stavi@...wei.com,
Lei Yang <leiyang@...hat.com>, Simon Horman <horms@...nel.org>
Subject: Re: [PATCH net-next v9 6/6] vhost/net: Support
VIRTIO_NET_F_HASH_REPORT
On 2025/03/12 12:36, Jason Wang wrote:
> On Tue, Mar 11, 2025 at 2:24 PM Akihiko Odaki <akihiko.odaki@...nix.com> wrote:
>>
>> On 2025/03/11 9:42, Jason Wang wrote:
>>> On Mon, Mar 10, 2025 at 3:04 PM Akihiko Odaki <akihiko.odaki@...nix.com> wrote:
>>>>
>>>> On 2025/03/10 13:43, Jason Wang wrote:
>>>>> On Fri, Mar 7, 2025 at 7:02 PM Akihiko Odaki <akihiko.odaki@...nix.com> wrote:
>>>>>>
>>>>>> VIRTIO_NET_F_HASH_REPORT allows to report hash values calculated on the
>>>>>> host. When VHOST_NET_F_VIRTIO_NET_HDR is employed, it will report no
>>>>>> hash values (i.e., the hash_report member is always set to
>>>>>> VIRTIO_NET_HASH_REPORT_NONE). Otherwise, the values reported by the
>>>>>> underlying socket will be reported.
>>>>>>
>>>>>> VIRTIO_NET_F_HASH_REPORT requires VIRTIO_F_VERSION_1.
>>>>>>
>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@...nix.com>
>>>>>> Tested-by: Lei Yang <leiyang@...hat.com>
>>>>>> ---
>>>>>> drivers/vhost/net.c | 49 +++++++++++++++++++++++++++++--------------------
>>>>>> 1 file changed, 29 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>>>>> index b9b9e9d40951856d881d77ac74331d914473cd56..16b241b44f89820a42c302f3586ea6bb5e0d4289 100644
>>>>>> --- a/drivers/vhost/net.c
>>>>>> +++ b/drivers/vhost/net.c
>>>>>> @@ -73,6 +73,7 @@ enum {
>>>>>> VHOST_NET_FEATURES = VHOST_FEATURES |
>>>>>> (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
>>>>>> (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
>>>>>> + (1ULL << VIRTIO_NET_F_HASH_REPORT) |
>>>>>> (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
>>>>>> (1ULL << VIRTIO_F_RING_RESET)
>>>>>> };
>>>>>> @@ -1097,9 +1098,11 @@ static void handle_rx(struct vhost_net *net)
>>>>>> .msg_controllen = 0,
>>>>>> .msg_flags = MSG_DONTWAIT,
>>>>>> };
>>>>>> - struct virtio_net_hdr hdr = {
>>>>>> - .flags = 0,
>>>>>> - .gso_type = VIRTIO_NET_HDR_GSO_NONE
>>>>>> + struct virtio_net_hdr_v1_hash hdr = {
>>>>>> + .hdr = {
>>>>>> + .flags = 0,
>>>>>> + .gso_type = VIRTIO_NET_HDR_GSO_NONE
>>>>>> + }
>>>>>> };
>>>>>> size_t total_len = 0;
>>>>>> int err, mergeable;
>>>>>> @@ -1110,7 +1113,6 @@ static void handle_rx(struct vhost_net *net)
>>>>>> bool set_num_buffers;
>>>>>> struct socket *sock;
>>>>>> struct iov_iter fixup;
>>>>>> - __virtio16 num_buffers;
>>>>>> int recv_pkts = 0;
>>>>>>
>>>>>> mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
>>>>>> @@ -1191,30 +1193,30 @@ static void handle_rx(struct vhost_net *net)
>>>>>> vhost_discard_vq_desc(vq, headcount);
>>>>>> continue;
>>>>>> }
>>>>>> + hdr.hdr.num_buffers = cpu_to_vhost16(vq, headcount);
>>>>>> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
>>>>>> if (unlikely(vhost_hlen)) {
>>>>>> - if (copy_to_iter(&hdr, sizeof(hdr),
>>>>>> - &fixup) != sizeof(hdr)) {
>>>>>> + if (copy_to_iter(&hdr, vhost_hlen,
>>>>>> + &fixup) != vhost_hlen) {
>>>>>> vq_err(vq, "Unable to write vnet_hdr "
>>>>>> "at addr %p\n", vq->iov->iov_base);
>>>>>> goto out;
>>>>>
>>>>> Is this an "issue" specific to RSS/HASH? If it's not, we need a separate patch.
>>>>>
>>>>> Honestly, I'm not sure if it's too late to fix this.
>>>>
>>>> There is nothing wrong with the current implementation.
>>>
>>> Note that I meant the vhost_hlen part, and the current code is tricky.
>>>
>>> The comment said:
>>>
>>> """
>>> /* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
>>> """
>>>
>>> So it tries to only offer virtio_net_hdr even if vhost_hlen is the set
>>> to mrg_rxbuf len.
>>>
>>> And this patch changes this behaviour.
>>
>> mrg_rxbuf only adds the num_buffers field, which is always set for
>> mrg_rxbuf.
>>
>> The num_buffers was not set for VIRTIO_F_VERSION_1 in the past, but this
>> was also fixed with commit a3b9c053d82a ("vhost/net: Set num_buffers for
>> virtio 1.0")
>>
>> So there is no behavioral change for existing features with this patch.
>
> I meant this part.
>
>>>>> + if (copy_to_iter(&hdr, vhost_hlen,
>>>>> + &fixup) != vhost_hlen) {
>
> We should copy only sizeof(hdr) instead of vhost_hlen.> > Anything I miss?
sizeof(hdr) will be greater than vhost_hlen when neither
VIRTIO_NET_F_MRG_RXBUF or VIRTIO_F_VERSION_1 is negotiated.
Regards,
Akihiko Odaki
>
> Thanks
>
>>
>> Regards,
>> Akihiko Odaki
>>
>>>
>>> Thanks
>>>
>>>> The current
>>>> implementation fills the header with zero except num_buffers, which it
>>>> fills some real value. This functionality is working fine with
>>>> VIRTIO_NET_F_MRG_RXBUF and VIRTIO_F_VERSION_1, which change the header size.
>>>>
>>>> Now I'm adding VIRTIO_NET_F_HASH_REPORT and it adds the hash_report
>>>> field, which also needs to be initialized with zero, so I'm making sure
>>>> vhost_net will also initialize it.
>>>>
>>>> Regards,
>>>> Akihiko Odaki
>>>>
>>>>>
>>>>> Others look fine.
>>>>>
>>>>> Thanks
>>>>>
>>>>
>>>
>>
>
Powered by blists - more mailing lists