netdev - Re: [PATCH v2 3/3] vduse: Temporarily disable control queue 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] [day] [month] [year] [list]

Date: Tue, 4 Jul 2023 21:22:34 +0200
From: Maxime Coquelin <maxime.coquelin@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: xieyongji@...edance.com, jasowang@...hat.com, david.marchand@...hat.com,
 lulu@...hat.com, linux-kernel@...r.kernel.org,
 virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
 xuanzhuo@...ux.alibaba.com, eperezma@...hat.com
Subject: Re: [PATCH v2 3/3] vduse: Temporarily disable control queue features



On 7/4/23 18:43, Michael S. Tsirkin wrote:
> On Tue, Jul 04, 2023 at 06:40:45PM +0200, Maxime Coquelin wrote:
>> Virtio-net driver control queue implementation is not safe
>> when used with VDUSE. If the VDUSE application does not
>> reply to control queue messages, it currently ends up
>> hanging the kernel thread sending this command.
>>
>> Some work is on-going to make the control queue
>> implementation robust with VDUSE. Until it is completed,
>> let's disable control virtqueue and features that depend on
>> it.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@...hat.com>
>> ---
>>   drivers/vdpa/vdpa_user/vduse_dev.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>> index 1271c9796517..04367a53802b 100644
>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>> @@ -1778,6 +1778,25 @@ static struct attribute *vduse_dev_attrs[] = {
>>   
>>   ATTRIBUTE_GROUPS(vduse_dev);
>>   
>> +static void vduse_dev_features_fixup(struct vduse_dev_config *config)
>> +{
>> +	if (config->device_id == VIRTIO_ID_NET) {
>> +		/*
>> +		 * Temporarily disable control virtqueue and features that
>> +		 * depend on it while CVQ is being made more robust for VDUSE.
>> +		 */
>> +		config->features &= ~((1ULL << VIRTIO_NET_F_CTRL_VQ) |
>> +				(1ULL << VIRTIO_NET_F_CTRL_RX) |
>> +				(1ULL << VIRTIO_NET_F_CTRL_VLAN) |
>> +				(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE) |
>> +				(1ULL << VIRTIO_NET_F_MQ) |
>> +				(1ULL << VIRTIO_NET_F_CTRL_MAC_ADDR) |
>> +				(1ULL << VIRTIO_NET_F_RSS) |
>> +				(1ULL << VIRTIO_NET_F_HASH_REPORT) |
>> +				(1ULL << VIRTIO_NET_F_NOTF_COAL));
>> +	}
>> +}
>> +
> 
> 
> This will never be exhaustive, we are adding new features.
> Please add an allowlist with just legal ones instead.

Ok, got it!
I'll post a new revision.

Thanks,
Maxime

> 
> 
>>   static int vduse_create_dev(struct vduse_dev_config *config,
>>   			    void *config_buf, u64 api_version)
>>   {
>> @@ -1793,6 +1812,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>>   	if (!dev)
>>   		goto err;
>>   
>> +	vduse_dev_features_fixup(config);
>> +
>>   	dev->api_version = api_version;
>>   	dev->device_features = config->features;
>>   	dev->device_id = config->device_id;
>> -- 
>> 2.41.0
>