Re: [PATCH 3/6] virtio_console: free buffers after reset

On 2018年04月21日 02:18, Michael S. Tsirkin wrote:

Console driver is out of spec. The spec says: A driver MUST NOT decrement the available idx on a live virtqueue (ie. there is no way to “unexpose” buffers). and it does exactly that by trying to detach unused buffers without doing a device reset first.

Defer detaching the buffers until device unplug.

Of course this means we might get an interrupt for a vq without an attached port now. Handle that by discarding the consumed buffer.

Reported-by: Tiwei Bie tiwei.bie@intel.com Fixes: b3258ff1d6 ("virtio: Decrement avail idx on buffer detach") CC: stable@kernel.org Signed-off-by: Michael S. Tsirkin mst@redhat.com

I wonder whether or not we can have some BUG_ON() in virtqueue_detach_unused_buf() to detect such bugs (e.g by checking status?).

Thanks

---

drivers/char/virtio_console.c | 49 +++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 3e56f32..26a66ff 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -1402,7 +1402,6 @@ static int add_port(struct ports_device *portdev, u32 id) { char debugfs_name[16]; struct port *port;

• struct port_buffer *buf; dev_t devt; unsigned int nr_added_bufs; int err;

@@ -1513,8 +1512,6 @@ static int add_port(struct ports_device *portdev, u32 id) return 0; free_inbufs:

• while ((buf = virtqueue_detach_unused_buf(port->in_vq)))

• free_buf(buf, true);
  

  free_device: device_destroy(pdrvdata.class, port->dev->devt); free_cdev:

@@ -1539,34 +1536,14 @@ static void remove_port(struct kref *kref) 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); spin_unlock_irq(&port->inbuf_lock);

• /* Remove buffers we queued up for the Host to send us data in. */
• do {

• spin_lock_irq(&port->inbuf_lock);
  

• buf = virtqueue_detach_unused_buf(port->in_vq);
  

• spin_unlock_irq(&port->inbuf_lock);
  

• if (buf)
  

• 	free_buf(buf, true);
  

• } while (buf);
• spin_lock_irq(&port->outvq_lock); reclaim_consumed_buffers(port); spin_unlock_irq(&port->outvq_lock);
• /* Free pending buffers from the out-queue. */
• do {

• spin_lock_irq(&port->outvq_lock);
  

• buf = virtqueue_detach_unused_buf(port->out_vq);
  

• spin_unlock_irq(&port->outvq_lock);
  

• if (buf)
  

• 	free_buf(buf, true);
  

• } while (buf); }

/* @@ -1791,13 +1768,24 @@ static void control_work_handler(struct work_struct *work) spin_unlock(&portdev->c_ivq_lock); } +static void flush_bufs(struct virtqueue *vq, bool can_sleep) +{

• struct port_buffer *buf;
• unsigned int len;
• while ((buf = virtqueue_get_buf(vq, &len)))

• free_buf(buf, can_sleep);
  

+}

• static void out_intr(struct virtqueue *vq) { struct port *port;

port = find_port_by_vq(vq->vdev->priv, vq);

• if (!port)

• if (!port) {

• flush_bufs(vq, false);
  

  return;
• }

wake_up_interruptible(&port->waitqueue); } @@ -1808,8 +1796,10 @@ static void in_intr(struct virtqueue *vq) unsigned long flags; port = find_port_by_vq(vq->vdev->priv, vq);

• if (!port)

• if (!port) {

• flush_bufs(vq, false);
  

  return;
• }

spin_lock_irqsave(&port->inbuf_lock, flags); port->inbuf = get_inbuf(port); @@ -1984,6 +1974,15 @@ static const struct file_operations portdev_fops = { static void remove_vqs(struct ports_device *portdev) {

• struct virtqueue *vq;
• virtio_device_for_each_vq(portdev->vdev, vq) {

• struct port_buffer *buf;
  

• flush_bufs(vq, true);
  

• while ((buf = virtqueue_detach_unused_buf(vq)))
  

• 	free_buf(buf, true);
  

• } portdev->vdev->config->del_vqs(portdev->vdev); kfree(portdev->in_vqs); kfree(portdev->out_vqs);