netdev - Re: [RFC PATCH v2 3/8] vhost-net: allow configuring extended features

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f0ee95b7-6830-4a53-8d6b-0edf1a7ab142@redhat.com>
Date: Tue, 3 Jun 2025 15:32:13 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Akihiko Odaki <akihiko.odaki@...nix.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 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.

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

/P