Re: [PATCH 2/2] virtio: console: add locking around c_ovq operations

[Amit Shah <amit.shah@redhat.com>]

On (Fri) 29 Mar 2013 [08:57:44], Wanlong Gao wrote:
> On 03/28/2013 07:28 PM, Amit Shah wrote:
> > When multiple ovq operations are being performed (lots of open/close
> > operations on virtio_console fds), the __send_control_msg() function can
> > get confused without locking.
> > 
> > A simple recipe to cause badness is:
> > * create a QEMU VM with two virtio-serial ports
> > * in the guest, do
> >   while true;do echo abc >/dev/vport0p1;done
> >   while true;do echo edf >/dev/vport0p2;done
> > 
> > In one run, this caused a panic in __send_control_msg().  In another, I
> > got
> > 
> >    virtio_console virtio0: control-o:id 0 is not a head!
> > 
> > This also results repeated messages similar to these on the host:
> > 
> >   qemu-kvm: virtio-serial-bus: Unexpected port id 478762112 for device virtio-serial-bus.0
> >   qemu-kvm: virtio-serial-bus: Unexpected port id 478762368 for device virtio-serial-bus.0
> > 
> > Reported-by: FuXiangChun <xfu@redhat.com>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  drivers/char/virtio_console.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index 7e9bc1d..410866c 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -150,6 +150,7 @@ struct ports_device {
> >  
> >  	/* To protect the vq operations for the control channel */
> >  	spinlock_t c_ivq_lock;
> > +	spinlock_t c_ovq_lock;
> >  
> >  	/* The current config space is stored here */
> >  	struct virtio_console_config config;
> > @@ -569,11 +570,14 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id,
> >  	vq = portdev->c_ovq;
> >  
> >  	sg_init_one(sg, &cpkt, sizeof(cpkt));
> > +
> > +	spin_lock_irq(&portdev->c_ovq_lock);
> >  	if (virtqueue_add_buf(vq, sg, 1, 0, &cpkt, GFP_ATOMIC) == 0) {
> >  		virtqueue_kick(vq);
> >  		while (!virtqueue_get_buf(vq, &len))
> >  			cpu_relax();
> >  	}
> > +	spin_unlock_irq(&portdev->c_ovq_lock);
> 
> While you lock the irq, why don't we need to save and restore the irq flags here?

_irq isn't actually needed; I'll send a v2 with just spin_lock/unlock.

Thanks,

		Amit