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

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 01/29/2017 01:35 AM, Michael S. Tsirkin wrote:
> On Fri, Jan 27, 2017 at 02:37:47PM +0100, Greg Kurz wrote:
>> On Fri, 27 Jan 2017 13:24:13 +0100
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> 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@linux.vnet.ibm.com>
>>>>> 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@linux.vnet.ibm.com>
>>>>> 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 :).
>>>
>>
>> Yes, I think this is what we need.
>>
>> Cheers.
> 
> OK, pls test this patch.
> 

Have submitted a slightly modified version of the above patch
with the subject "[PATCH] vhost: fix initialization for vq->is_le".

I wrote to our tester guys to give it some good testing. Will
post an update on the result of the testing there ASAP (or the
someone from testing will).

Regards,
Halil


>> --
>> Greg
>>
>>> Thank you very much!
>>>
>>> Halil
>>>
>>>
>