[master/0.14] virtio-serial: Make sure virtqueue is ready before discarding data

Submitted by Amit Shah on Feb. 4, 2011, 8:54 a.m.

Details

Message ID f91470cb09ef21f7caeeb81eda407d498b3c1ad1.1296809653.git.amit.shah@redhat.com
State New
Headers show

Commit Message

Amit Shah Feb. 4, 2011, 8:54 a.m.
This can happen if a port gets unplugged before guest has chance to
initialise vqs.

Reported-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Comments

Juan Quintela Feb. 4, 2011, 12:11 p.m.
Amit Shah <amit.shah@redhat.com> wrote:
> This can happen if a port gets unplugged before guest has chance to
> initialise vqs.
>
> Reported-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/virtio-serial-bus.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 09e22aa..e05ab5e 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -117,6 +117,9 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
>  {
>      VirtQueueElement elem;
>  
> +    if (!virtio_queue_ready(vq)) {
> +        return;
> +    }
>      while (virtqueue_pop(vq, &elem)) {
>          virtqueue_push(vq, &elem, 0);
>      }

Reviewed-by: Juan Quintela <quintela@redhat.com>
Anthony Liguori Feb. 4, 2011, 12:52 p.m.
On 02/04/2011 02:54 AM, Amit Shah wrote:
> This can happen if a port gets unplugged before guest has chance to
> initialise vqs.
>
> Reported-by: Juan Quintela<quintela@redhat.com>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>    

Applied to master, Thanks.

Regards,

Anthony Liguori
> ---
>   hw/virtio-serial-bus.c |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 09e22aa..e05ab5e 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -117,6 +117,9 @@ static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
>   {
>       VirtQueueElement elem;
>
> +    if (!virtio_queue_ready(vq)) {
> +        return;
> +    }
>       while (virtqueue_pop(vq,&elem)) {
>           virtqueue_push(vq,&elem, 0);
>       }
>
Amit Shah Feb. 6, 2011, 6:20 a.m.
On (Fri) 04 Feb 2011 [06:52:04], Anthony Liguori wrote:
> On 02/04/2011 02:54 AM, Amit Shah wrote:
> >This can happen if a port gets unplugged before guest has chance to
> >initialise vqs.
> >
> >Reported-by: Juan Quintela<quintela@redhat.com>
> >Signed-off-by: Amit Shah<amit.shah@redhat.com>
> 
> Applied to master, Thanks.

Hi,

What's the strategy to commit to 0.14?

		Amit
Amit Shah Feb. 8, 2011, 2:14 a.m.
On (Sun) 06 Feb 2011 [11:50:46], Amit Shah wrote:
> On (Fri) 04 Feb 2011 [06:52:04], Anthony Liguori wrote:
> > On 02/04/2011 02:54 AM, Amit Shah wrote:
> > >This can happen if a port gets unplugged before guest has chance to
> > >initialise vqs.
> > >
> > >Reported-by: Juan Quintela<quintela@redhat.com>
> > >Signed-off-by: Amit Shah<amit.shah@redhat.com>
> > 
> > Applied to master, Thanks.
> 
> What's the strategy to commit to 0.14?

Nevermind; saw it in the last pull.

		Amit
Anthony Liguori Feb. 8, 2011, 2:25 a.m.
On 02/07/2011 08:14 PM, Amit Shah wrote:
> On (Sun) 06 Feb 2011 [11:50:46], Amit Shah wrote:
>    
>> On (Fri) 04 Feb 2011 [06:52:04], Anthony Liguori wrote:
>>      
>>> On 02/04/2011 02:54 AM, Amit Shah wrote:
>>>        
>>>> This can happen if a port gets unplugged before guest has chance to
>>>> initialise vqs.
>>>>
>>>> Reported-by: Juan Quintela<quintela@redhat.com>
>>>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>>>          
>>> Applied to master, Thanks.
>>>        
>> What's the strategy to commit to 0.14?
>>      
> Nevermind; saw it in the last pull.
>    

Yes, but if we decide to make this a subsection, we may need to revert 
this before the 0.14.0 release because once 0.14.0 goes out, we can't go 
back and change it.

If that's how you want to approach it, then please send the revert 
patches.  The flow_control=0 patches you sent out before definitely 
aren't the right approach for migration compatibility.

Regards,

Anthony Liguori

> 		Amit
>
>
Amit Shah Feb. 8, 2011, 2:55 a.m.
On (Mon) 07 Feb 2011 [20:25:59], Anthony Liguori wrote:
> On 02/07/2011 08:14 PM, Amit Shah wrote:
> >On (Sun) 06 Feb 2011 [11:50:46], Amit Shah wrote:
> >>On (Fri) 04 Feb 2011 [06:52:04], Anthony Liguori wrote:
> >>>On 02/04/2011 02:54 AM, Amit Shah wrote:
> >>>>This can happen if a port gets unplugged before guest has chance to
> >>>>initialise vqs.
> >>>>
> >>>>Reported-by: Juan Quintela<quintela@redhat.com>
> >>>>Signed-off-by: Amit Shah<amit.shah@redhat.com>
> >>>Applied to master, Thanks.
> >>What's the strategy to commit to 0.14?
> >Nevermind; saw it in the last pull.
> 
> Yes, but if we decide to make this a subsection, we may need to
> revert this before the 0.14.0 release because once 0.14.0 goes out,
> we can't go back and change it.

Right.

However, virtio doesn't have vmstate yet.  Are we willing to put vmstate
in 0.14 to introduce subsections?  I doubt that.

> If that's how you want to approach it, then please send the revert
> patches.  The flow_control=0 patches you sent out before definitely
> aren't the right approach for migration compatibility.

I understand, I've not looked at that thread yet, but if we want to do
that for 0.14 w/o vmstate, that might be the only way.

		Amit
Anthony Liguori Feb. 8, 2011, 7:40 a.m.
On 02/07/2011 08:55 PM, Amit Shah wrote:
> On (Mon) 07 Feb 2011 [20:25:59], Anthony Liguori wrote:
>    
>> On 02/07/2011 08:14 PM, Amit Shah wrote:
>>      
>>> On (Sun) 06 Feb 2011 [11:50:46], Amit Shah wrote:
>>>        
>>>> On (Fri) 04 Feb 2011 [06:52:04], Anthony Liguori wrote:
>>>>          
>>>>> On 02/04/2011 02:54 AM, Amit Shah wrote:
>>>>>            
>>>>>> This can happen if a port gets unplugged before guest has chance to
>>>>>> initialise vqs.
>>>>>>
>>>>>> Reported-by: Juan Quintela<quintela@redhat.com>
>>>>>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>>>>>>              
>>>>> Applied to master, Thanks.
>>>>>            
>>>> What's the strategy to commit to 0.14?
>>>>          
>>> Nevermind; saw it in the last pull.
>>>        
>> Yes, but if we decide to make this a subsection, we may need to
>> revert this before the 0.14.0 release because once 0.14.0 goes out,
>> we can't go back and change it.
>>      
> Right.
>
> However, virtio doesn't have vmstate yet.  Are we willing to put vmstate
> in 0.14 to introduce subsections?  I doubt that.
>    

Not, that's why I suggested reverting this change and pushing it to 
0.15.  We're not going to fix it properly in 0.14.

Regards,

Anthony Liguori

>> If that's how you want to approach it, then please send the revert
>> patches.  The flow_control=0 patches you sent out before definitely
>> aren't the right approach for migration compatibility.
>>      
> I understand, I've not looked at that thread yet, but if we want to do
> that for 0.14 w/o vmstate, that might be the only way.
>
> 		Amit
>
>
Amit Shah Feb. 8, 2011, 7:55 a.m.
On (Tue) 08 Feb 2011 [01:40:09], Anthony Liguori wrote:
> >
> >However, virtio doesn't have vmstate yet.  Are we willing to put vmstate
> >in 0.14 to introduce subsections?  I doubt that.
> 
> Not, that's why I suggested reverting this change and pushing it to
> 0.15.  We're not going to fix it properly in 0.14.

You mean reverting all of the flow control work?  But then it exposes
another bug: a malicious guest doing huge writes will cause an OOM on
the host; it's better to have this fix rather than not have it.

		Amit

Patch hide | download patch | download mbox

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 09e22aa..e05ab5e 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -117,6 +117,9 @@  static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
 {
     VirtQueueElement elem;
 
+    if (!virtio_queue_ready(vq)) {
+        return;
+    }
     while (virtqueue_pop(vq, &elem)) {
         virtqueue_push(vq, &elem, 0);
     }