| 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: <ece8f139-fcd4-4b55-b4bb-3a62da66ad01@daynix.com>
Date: Sun, 8 Jun 2025 14:28:25 +0900
From: Akihiko Odaki <akihiko.odaki@...nix.com>
To: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>,
Jason Wang <jasowang@...hat.com>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, "Michael S. Tsirkin" <mst@...hat.com>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez
<eperezma@...hat.com>, Yuri Benditovich <yuri.benditovich@...nix.com>
Subject: Re: [RFC PATCH v2 3/8] vhost-net: allow configuring extended features
On 2025/06/06 20:52, Paolo Abeni wrote:
> On 6/6/25 11:57 AM, Akihiko Odaki wrote:
>> On 2025/06/03 22:32, Paolo Abeni wrote:
>>> On 5/31/25 8:15 AM, Akihiko Odaki wrote:
>>>> On 2025/05/30 23:49, Paolo Abeni wrote:
>>>>> Use the extended feature type for 'acked_features' and implement
>>>>> two new ioctls operation allowing the user-space to set/query an
>>>>> unbounded amount of features.
>>>>>
>>>>> The actual number of processed features is limited by virtio_features_t
>>>>> size, and attempts to set features above such limit fail with
>>>>> EOPNOTSUPP.
>>>>>
>>>>> Note that the legacy ioctls implicitly truncate the negotiated
>>>>> features to the lower 64 bits range.
>>>>>
>>>>> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
>>>>> ---
>>>>> v1 -> v2:
>>>>> - change the ioctl to use an extensible API
>>>>> ---
>>>>> drivers/vhost/net.c | 61 ++++++++++++++++++++++++++++++--
>>>>> drivers/vhost/vhost.h | 2 +-
>>>>> include/uapi/linux/vhost.h | 7 ++++
>>>>> include/uapi/linux/vhost_types.h | 5 +++
>>>>> 4 files changed, 71 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>>>> index 7cbfc7d718b3..f53294440695 100644
>>>>> --- a/drivers/vhost/net.c
>>>>> +++ b/drivers/vhost/net.c
>>>>> @@ -77,6 +77,8 @@ enum {
>>>>> (1ULL << VIRTIO_F_RING_RESET)
>>>>> };
>>>>>
>>>>> +#define VHOST_NET_ALL_FEATURES VHOST_NET_FEATURES
>>>>> +
>>>>> enum {
>>>>> VHOST_NET_BACKEND_FEATURES = (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2)
>>>>> };
>>>>> @@ -1614,7 +1616,7 @@ static long vhost_net_reset_owner(struct vhost_net *n)
>>>>> return err;
>>>>> }
>>>>>
>>>>> -static int vhost_net_set_features(struct vhost_net *n, u64 features)
>>>>> +static int vhost_net_set_features(struct vhost_net *n, virtio_features_t features)
>>>>> {
>>>>> size_t vhost_hlen, sock_hlen, hdr_len;
>>>>> int i;
>>>>> @@ -1685,8 +1687,9 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
>>>>> void __user *argp = (void __user *)arg;
>>>>> u64 __user *featurep = argp;
>>>>> struct vhost_vring_file backend;
>>>>> - u64 features;
>>>>> - int r;
>>>>> + virtio_features_t all_features;
>>>>> + u64 features, count;
>>>>> + int r, i;
>>>>>
>>>>> switch (ioctl) {
>>>>> case VHOST_NET_SET_BACKEND:
>>>>> @@ -1704,6 +1707,58 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
>>>>> if (features & ~VHOST_NET_FEATURES)
>>>>> return -EOPNOTSUPP;
>>>>> return vhost_net_set_features(n, features);
>>>>> + case VHOST_GET_FEATURES_ARRAY:
>>>>> + {
>>>>> + if (copy_from_user(&count, argp, sizeof(u64)))
>>>>> + return -EFAULT;
>>>>> +
>>>>> + /* Copy the net features, up to the user-provided buffer size */
>>>>> + all_features = VHOST_NET_ALL_FEATURES;
>>>>> + for (i = 0; i < min(VIRTIO_FEATURES_WORDS / 2, count); ++i) {
>>>>
>>>> I think you need to use: array_index_nospec()
>>>
>>> Do you mean like:
>>> i = array_index_nospec(i, min(VIRTIO_FEATURES_WORDS / 2, count));
>>>
>>> ?
>>>
>>> Note that even if the cpu would speculative execute the loop for too
>>> high 'i' values, it will could only read `all_features`, which
>>> user-space can access freely.
>>
>> I was wrong; I forgot you used a 128-bit integer instead of an array.
>>
>>>
>>>>> diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
>>>>> index d7656908f730..3f227114c557 100644
>>>>> --- a/include/uapi/linux/vhost_types.h
>>>>> +++ b/include/uapi/linux/vhost_types.h
>>>>> @@ -110,6 +110,11 @@ struct vhost_msg_v2 {
>>>>> };
>>>>> };
>>>>>
>>>>> +struct vhost_features_array {
>>>>> + __u64 count; /* number of entries present in features array */
>>>>> + __u64 features[];
>>>>
>>>>
>>>> An alternative idea:
>>>>
>>>> #define VHOST_GET_FEATURES_ARRAY(len) _IOC(_IOC_READ, VHOST_VIRTIO,
>>>> 0x00, (len))
>>>>
>>>> By doing so, the kernel can have share the code for
>>>> VHOST_GET_FEATURES_ARRAY() with VHOST_GET_FEATURES() since
>>>> VHOST_GET_FEATURES() will be just a specialized definition.
>>>>
>>>> It also makes the life of the userspace a bit easier by not making it
>>>> construct struct vhost_features_array.
>>>>
>>>> Looking at include/uapi, it seems there are examples of both your
>>>> pattern and my alternative, so please pick what you prefer.
>>>
>>> I'm ok either way, but I don't see big win code-wise. The user-space
>>> side saving will be literally a one liner. In the kernel the get/set
>>> sockopt could be consolidated, but there will be a slightly increase in
>>> complexity, to extract the ioctl len from the ioctl op value itself.
>>
>> The current patch also requires copy_from_user() to get the count, so I
>> don't think they are different in that sense.
>>
>> The difference will be marginal anyway, and it may turn out encoding the
>> length in the ioctl number requires a bit more code.
>
> I'm sorry, almost mid-air collision. I just send out the rfc v3, and I
> read your reply here only afterwards.
>
> I stuck to separate ioctls operations; as an additional reason for that,
> I understand there is interest in extending the features space even
> more, and let user-space/kernel with different features space limits
> easily interact.
>
> I think that with a single ioctl either the kernel or the user-space
> should be update to handle explicitly every additional features space
> expansion, while the API proposed here no additional changes should be
> required.
It is not a problem with the VHOST_GET_FEATURES_ARRAY() macro I
suggested. It takes the size of array as a parameter, enabling it to
grow without updating the ioctl definition.
Regards,
Akihiko Odaki
Powered by blists - more mailing lists