netdev - Re: [PATCH net] vhost_net: remove VHOST_NET_F_VIRTIO_NET

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: <23efe110-f61a-5aee-c0b4-bd3dc5426438@redhat.com>
Date:   Fri, 8 Jun 2018 13:07:09 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] vhost_net: remove VHOST_NET_F_VIRTIO_NET_HDR support



On 2018年06月08日 12:46, Michael S. Tsirkin wrote:
> On Fri, Jun 08, 2018 at 11:50:42AM +0800, Jason Wang wrote:
>> This feature bit is duplicated with VIRTIO_F_ANY_LAYOUT, this means if
>> a userpsace want to enable VRITIO_F_ANY_LAYOUT,
>> VHOST_NET_F_VIRTIO_NET_HDR will be implied too. This is wrong and will
>> break networking.
> What breaks networking exactly? VHOST_NET supported ANY_LAYOUT
> from day one. For this reason it does not need to know about
> VRITIO_F_ANY_LAYOUT and we reused the bit for other purposes.

It's the knowledge of vhost_net code it self but not userspace. For 
userspace, it should depends on the value of returned by 
VHOST_GET_FEATURES. So when userspace can set_features with ANY_LAYOUT, 
vhost may think it wants VHOST_NET_F_VIRTIO_NET_HDR.

>
>
>
>> Fixing this by safely removing
>> VHOST_NET_F_VIRTIO_NET_HDR support. There should be very few or even
>> no userspace can use this.
> Quite possibly, but it is hard to be sure. It seems safer to
> maintain it unless there's an actual reason something's broken.

I think not since the feature is negotiated not mandatory?

>
>> Further cleanups could be done for
>> -net-next for safety.
>>
>> In the future, we need a vhost dedicated feature set/get ioctl()
>> instead of reusing virtio ones.
> Not just in the future, we might want to switch iommu
> to a sane structure without the 64 bit padding bug
> right now.

Yes, I hit this bug when introducing V2 of msg IOTLB message.

>
>> Fixes: 4e9fa50c6ccbe ("vhost: move features to core")
> This tag makes no sense here IMHO. Looks like people are using some tool
> that just looks at the earliest version where patch won't apply. The
> commit in question just moved some code around.

Looks not, before this commit, vhost_net won't return ANY_LAYOUT.

Thanks

>
>> Signed-off-by: Jason Wang <jasowang@...hat.com>
>> ---
>>   drivers/vhost/net.c | 15 +++++----------
>>   1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 986058a..83eef52 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -69,7 +69,6 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
>>   
>>   enum {
>>   	VHOST_NET_FEATURES = VHOST_FEATURES |
>> -			 (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
>>   			 (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
>>   			 (1ULL << VIRTIO_F_IOMMU_PLATFORM)
>>   };
>> @@ -1255,15 +1254,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
>>   			       (1ULL << VIRTIO_F_VERSION_1))) ?
>>   			sizeof(struct virtio_net_hdr_mrg_rxbuf) :
>>   			sizeof(struct virtio_net_hdr);
>> -	if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) {
>> -		/* vhost provides vnet_hdr */
>> -		vhost_hlen = hdr_len;
>> -		sock_hlen = 0;
>> -	} else {
>> -		/* socket provides vnet_hdr */
>> -		vhost_hlen = 0;
>> -		sock_hlen = hdr_len;
>> -	}
>> +
>> +        /* socket provides vnet_hdr */
>> +	vhost_hlen = 0;
>> +	sock_hlen = hdr_len;
>> +
>>   	mutex_lock(&n->dev.mutex);
>>   	if ((features & (1 << VHOST_F_LOG_ALL)) &&
>>   	    !vhost_log_access_ok(&n->dev))
>> -- 
>> 2.7.4