Re: [BUG/RFC] vhost: net: big endian viring access despite virtio 1

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 01/26/2017 08:20 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 26, 2017 at 06:39:14PM +0100, Halil Pasic wrote:
>>
>> Hi!
>>
>> Recently I have been investigating some strange migration problems on
>> s390x.
>>
>> It turned out under certain circumstances vhost_net corrupts avail.idx by
>> using wrong endianness.

[..]

>> -------------------------8<--------------
>> >From b26e2bbdc03832a0204ee2b42967a1b49a277dc8 Mon Sep 17 00:00:00 2001
>> From: Halil Pasic <pasic@xxxxxxxxxxxxxxxxxx>
>> Date: Thu, 26 Jan 2017 00:06:15 +0100
>> Subject: [PATCH] vhost: remove useless/dangerous reset of is_le
>>
>> The reset of is_le does no good, but it contributes its fair share to a
>> bug in vhost_net, which occurs if we have some oldubufs when stopping and
>> setting a fd = -1 as a backend. Instead of doing something convoluted in
>> vhost_net, let's just get rid of the reset.
>>
>> Signed-off-by: Halil Pasic <pasic@xxxxxxxxxxxxxxxxxx>
>> Fixes: commit 2751c9882b94 
>> ---
>>  drivers/vhost/vhost.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index d643260..08072a2 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -1714,10 +1714,8 @@ int vhost_vq_init_access(struct vhost_virtqueue *vq)
>>         int r;
>>         bool is_le = vq->is_le;
>>
>> -       if (!vq->private_data) {
>> -               vhost_reset_is_le(vq);
>> +       if (!vq->private_data)
>>                 return 0;
>> -       }
>>
>>         vhost_init_is_le(vq);
> 
> 
> I think you do need to reset it, just maybe within vhost_init_is_le.
> 
>         if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
>                 vq->is_le = true;
> 	else
> 		vhost_reset_is_le(vq);
> 
> 

That is a very good point! I have overlooked that while the 
CONFIG_VHOST_CROSS_ENDIAN_LEGACY variant

static void vhost_init_is_le(struct vhost_virtqueue *vq)
{
        /* Note for legacy virtio: user_be is initialized at reset time
         * according to the host endianness. If userspace does not set an
         * explicit endianness, the default behavior is native endian, as
         * expected by legacy virtio.
         */
        vq->is_le = vhost_has_feature(vq, VIRTIO_F_VERSION_1) || !vq->user_be;
}

is fine the other variant 

static void vhost_init_is_le(struct vhost_virtqueue *vq)
{
        if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
                vq->is_le = true;
}
is a very strange initializer (makes assumptions about the state
to be initialized).

I agree, setting native endianness there sounds very reasonable.

I have a question regarding readability. IMHO the relationship
of reset_is_le and int_is_le is a bit confusing, and I'm afraid
it could become even more confusing with using reset in one of
the init_is_le's.

How about we do the following?

static void vhost_init_is_le(struct vhost_virtqueue *vq)
{
        if (vhost_has_feature(vq, VIRTIO_F_VERSION_1))
                vq->is_le = true;
+       else
+               vq->is_le = virtio_legacy_is_little_endian();

}

static void vhost_reset_is_le(struct vhost_virtqueue *vq)
{
-       vq->is_le = virtio_legacy_is_little_endian();
+       vhost_init_is_le(vq);
}

That way we would have correct endianness both after reset
and after init, I think :).

Thank you very much!

Halil


_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization



[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux