Linux Virtualization - Jul 2013 - [PATCH 04/10] virtio: console: return -ENODEV on all read operations after unplug

On 07/19/2013 04:16 AM, Amit Shah wrote:
> If a port gets unplugged while a user is blocked on read(), -ENODEV is
> returned.  However, subsequent read()s returned 0, indicating there's
no
> host-side connection (but not indicating the device went away).
>
> This also happened when a port was unplugged and the user didn't have
> any blocking operation pending.  If the user didn't monitor the SIGIO
> signal, they won't have a chance to find out if the port went away.
>
> Fix by returning -ENODEV on all read()s after the port gets unplugged.
> write() already behaves this way.
>
> CC: <stable at vger.kernel.org>
> Signed-off-by: Amit Shah <amit.shah at redhat.com>
> ---
>  drivers/char/virtio_console.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 6bf0df3..a39702a 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -749,6 +749,10 @@ static ssize_t port_fops_read(struct file *filp, char
__user *ubuf,
>  
>  	port = filp->private_data;
>  
> +	/* Port is hot-unplugged. */
> +	if (!port->guest_connected)
> +		return -ENODEV;
> +
What if the port is hot-unplugged after this check? Should we serialize
the hot plug with inbuf_lock here?
>  	if (!port_has_data(port)) {
>  		/*
>  		 * If nothing's connected on the host just return 0 in
> @@ -765,7 +769,7 @@ static ssize_t port_fops_read(struct file *filp, char
__user *ubuf,
>  		if (ret < 0)
>  			return ret;
>  	}
> -	/* Port got hot-unplugged. */
> +	/* Port got hot-unplugged while we were waiting above. */
>  	if (!port->guest_connected)
>  		return -ENODEV;
>  	/*

On (Fri) 19 Jul 2013 [13:07:49], Jason Wang wrote:
> On 07/19/2013 04:16 AM, Amit Shah wrote:
> > If a port gets unplugged while a user is blocked on read(), -ENODEV is
> > returned.  However, subsequent read()s returned 0, indicating
there's no
> > host-side connection (but not indicating the device went away).
> >
> > This also happened when a port was unplugged and the user didn't
have
> > any blocking operation pending.  If the user didn't monitor the
SIGIO
> > signal, they won't have a chance to find out if the port went
away.
> >
> > Fix by returning -ENODEV on all read()s after the port gets unplugged.
> > write() already behaves this way.
> >
> > CC: <stable at vger.kernel.org>
> > Signed-off-by: Amit Shah <amit.shah at redhat.com>
> > ---
> >  drivers/char/virtio_console.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/virtio_console.c
b/drivers/char/virtio_console.c
> > index 6bf0df3..a39702a 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -749,6 +749,10 @@ static ssize_t port_fops_read(struct file *filp,
char __user *ubuf,
> >  
> >  	port = filp->private_data;
> >  
> > +	/* Port is hot-unplugged. */
> > +	if (!port->guest_connected)
> > +		return -ENODEV;
> > +
> 
> What if the port is hot-unplugged after this check? Should we serialize
> the hot plug with inbuf_lock here?
If that happens, port_has_data() returns false, and the later
functions return appropriately, as unplug_port() clears out all the
data.

However, I spotted a couple of things that need fixing:

1. In the condition you describe above, port_has_data will return
false, and host_connected will be false as well, and fops_read() will
return 0 instead of -ENODEV once.  Subsequent reads will return
-ENODEV.

2. get_inbuf(), which is called by port_has_data() accesses the vqs
even if the port is unplugged.  The vqs are still available, and won't
have data in them (since the port is unplugged), but it's best to not
rely on such behaviour.  For the next merge window, I'll add extra
state, port_(un)plugged to track this instead of abusing
guest_connected.

That also simplifies the path for later if we get vq hot-plug as well.

I think the current code will behave satisfactorily for now; what do
you think?

		Amit

On 07/19/2013 01:45 PM, Amit Shah wrote:
> On (Fri) 19 Jul 2013 [13:07:49], Jason Wang wrote:
>> On 07/19/2013 04:16 AM, Amit Shah wrote:
>>> If a port gets unplugged while a user is blocked on read(), -ENODEV
is
>>> returned.  However, subsequent read()s returned 0, indicating
there's no
>>> host-side connection (but not indicating the device went away).
>>>
>>> This also happened when a port was unplugged and the user
didn't have
>>> any blocking operation pending.  If the user didn't monitor the
SIGIO
>>> signal, they won't have a chance to find out if the port went
away.
>>>
>>> Fix by returning -ENODEV on all read()s after the port gets
unplugged.
>>> write() already behaves this way.
>>>
>>> CC: <stable at vger.kernel.org>
>>> Signed-off-by: Amit Shah <amit.shah at redhat.com>
>>> ---
>>>  drivers/char/virtio_console.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/char/virtio_console.c
b/drivers/char/virtio_console.c
>>> index 6bf0df3..a39702a 100644
>>> --- a/drivers/char/virtio_console.c
>>> +++ b/drivers/char/virtio_console.c
>>> @@ -749,6 +749,10 @@ static ssize_t port_fops_read(struct file
*filp, char __user *ubuf,
>>>  
>>>  	port = filp->private_data;
>>>  
>>> +	/* Port is hot-unplugged. */
>>> +	if (!port->guest_connected)
>>> +		return -ENODEV;
>>> +
>> What if the port is hot-unplugged after this check? Should we serialize
>> the hot plug with inbuf_lock here?
> If that happens, port_has_data() returns false, and the later
> functions return appropriately, as unplug_port() clears out all the
> data.
>
> However, I spotted a couple of things that need fixing:
>
> 1. In the condition you describe above, port_has_data will return
> false, and host_connected will be false as well, and fops_read() will
> return 0 instead of -ENODEV once.  Subsequent reads will return
> -ENODEV.
Then the check is not needed here.
>
> 2. get_inbuf(), which is called by port_has_data() accesses the vqs
> even if the port is unplugged.  The vqs are still available, and won't
> have data in them (since the port is unplugged), but it's best to not
> rely on such behaviour.  For the next merge window, I'll add extra
> state, port_(un)plugged to track this instead of abusing
> guest_connected.
>
> That also simplifies the path for later if we get vq hot-plug as well.
>
> I think the current code will behave satisfactorily for now; what do
> you think?
Yes, sounds good.
>
> 		Amit