Linux Virtualization - Jul 2013 - [PATCH 08/10] virtio: console: add locks around buffer removal in port unplug path

Amit Shah <amit.shah at redhat.com> writes:
> The removal functions act on the vqs, and the vq operations need to be
> locked.
>
> Signed-off-by: Amit Shah <amit.shah at redhat.com>
How can userspace access the port now?  By the time we are cleaning up
buffers, there should be no possibility of such accesses.

The number of bugfixes here is deeply disturbing.  I wonder if it's be
easier to rewrite it all with a lock per port, and one global to protect
ports_driver_data.

Cheers,
Rusty.
> ---
>  drivers/char/virtio_console.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index ce71df1..dd0020d 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1516,18 +1516,22 @@ static void remove_port_data(struct port *port)
>  {
>  	struct port_buffer *buf;
>  
> +	spin_lock_irq(&port->inbuf_lock);
>  	/* Remove unused data this port might have received. */
>  	discard_port_data(port);
>  
> -	reclaim_consumed_buffers(port);
> -
>  	/* Remove buffers we queued up for the Host to send us data in. */
>  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
>  		free_buf(buf, true);
> +	spin_unlock_irq(&port->inbuf_lock);
> +
> +	spin_lock_irq(&port->outvq_lock);
> +	reclaim_consumed_buffers(port);
>  
>  	/* Free pending buffers from the out-queue. */
>  	while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
>  		free_buf(buf, true);
> +	spin_unlock_irq(&port->outvq_lock);
>  }
>  
>  /*
> -- 
> 1.8.1.4

On (Mon) 22 Jul 2013 [15:26:22], Rusty Russell wrote:
> Amit Shah <amit.shah at redhat.com> writes:
> > The removal functions act on the vqs, and the vq operations need to be
> > locked.
> >
> > Signed-off-by: Amit Shah <amit.shah at redhat.com>
> 
> How can userspace access the port now?  By the time we are cleaning up
> buffers, there should be no possibility of such accesses.
close(), can happen when the port is being unplugged.  We're just
making sure here that port_fops_release() and unplug_port() don't try
to free up the same data at the same time.
> The number of bugfixes here is deeply disturbing.
Yes, the first three fix a bug - close() after unplug.  However, the
others are inadequate locking fixes which I noticed while fixing that
bug.

Port unplug isn't a frequently-used or tested path, so these were
lying unnoticed so far.
>  I wonder if it's be
> easier to rewrite it all with a lock per port, and one global to protect
> ports_driver_data.
Hm, with this series, I don't see anything that might need extra
locking.  Though I'll take a look at this afresh in a while -- and see
if we could simplify something.

Given that this was necessary only for unplug operations, (and they
aren't a 'regular operation', so we could drop the stable@ for the
series?), are you OK with this series for now?

		Amit

Amit Shah <amit.shah at redhat.com> writes:
> On (Mon) 22 Jul 2013 [15:26:22], Rusty Russell wrote:
>> Amit Shah <amit.shah at redhat.com> writes:
>> > The removal functions act on the vqs, and the vq operations need
to be
>> > locked.
>> >
>> > Signed-off-by: Amit Shah <amit.shah at redhat.com>
>> 
>> How can userspace access the port now?  By the time we are cleaning up
>> buffers, there should be no possibility of such accesses.
>
> close(), can happen when the port is being unplugged.  We're just
> making sure here that port_fops_release() and unplug_port() don't try
> to free up the same data at the same time.
Why doesn't reference counting help us here?  Surely the last one should
clean up?
>> The number of bugfixes here is deeply disturbing.
>
> Yes, the first three fix a bug - close() after unplug.  However, the
> others are inadequate locking fixes which I noticed while fixing that
> bug.
>
> Port unplug isn't a frequently-used or tested path, so these were
> lying unnoticed so far.
>
>>  I wonder if it's be
>> easier to rewrite it all with a lock per port, and one global to
protect
>> ports_driver_data.
>
> Hm, with this series, I don't see anything that might need extra
> locking.  Though I'll take a look at this afresh in a while -- and see
> if we could simplify something.
>
> Given that this was necessary only for unplug operations, (and they
> aren't a 'regular operation', so we could drop the stable@ for
the
> series?), are you OK with this series for now?
Let's skip stable@ for theoretical bugs.  Please repost.

Cheers,
Rusty.