virtio_console: Add support for DMA memory allocation

From: Sjur Brændeland <[email protected]>

Add feature VIRTIO_CONSOLE_F_DMA_MEM. If the architecture has DMA support and this feature bit is set, the virtio data buffers will be allocated from DMA memory.

This is needed for using virtio_console from the remoteproc framework.

Signed-off-by Sjur Brændeland <[email protected]>

cc Rusty Russell <[email protected]>

cc Michael S. Tsirkin <[email protected]>

cc Ohad Ben-Cohen <[email protected]>

cc Linus Walleij <[email protected]>

cc [email protected]

---
 drivers/char/virtio_console.c  |   76 ++++++++++++++++++++++++++++++++--------
 include/linux/virtio_console.h |    1 +
 2 files changed, 62 insertions(+), 15 deletions(-)

drivers/char/virtio_console.c +1 -0

index cdf2f54..6bfbd09 100644
@@ -35,6 +35,7 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/module.h>
+#include <linux/dma-mapping.h>
 #include "../tty/hvc/hvc_console.h"
 
 /*

drivers/char/virtio_console.c +6 -1

+static inline void *
+alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t *dma_handle,
+		   gfp_t flag)
 {
-	kfree(buf->buf);
+#ifdef CONFIG_HAS_DMA
+	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+		struct device *dev = &vdev->dev;

drivers/char/virtio_console.c +43 -13

+	return kzalloc(size, flag);
+}
+
+static inline void
+free_databuf(struct virtio_device *vdev, size_t size, void *cpu_addr)
+{
+#ifdef CONFIG_HAS_DMA
+	dma_addr_t dma_handle = virt_to_bus(cpu_addr);
+	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+		struct device *dev = &vdev->dev;
+
+		dev = dev->parent ? dev->parent : dev;
+		dev = dev->parent ? dev->parent : dev;
+		dma_free_coherent(dev, size, cpu_addr, dma_handle);
+		return;
+	}
+#endif
+	kfree(cpu_addr);
+}
+
+static void
+free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
+{
+	free_databuf(vq->vdev, buf_size, buf);
 	kfree(buf);
 }
 
-static struct port_buffer *alloc_buf(size_t buf_size)
+static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size)
 {
 	struct port_buffer *buf;
+	dma_addr_t dma;
 
 	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		goto fail;
-	buf->buf = kzalloc(buf_size, GFP_KERNEL);
+	buf->buf = alloc_databuf(vq->vdev, buf_size, &dma, GFP_KERNEL);
 	if (!buf->buf)
 		goto free_buf;
 	buf->len = 0;

@@ -414,7 +455,7 @@ static void discard_port_data(struct port *port)
 		port->stats.bytes_discarded += buf->len - buf->offset;
 		if (add_inbuf(port->in_vq, buf) < 0) {
 			err++;
-			free_buf(buf);
+			free_buf(port->in_vq, buf, PAGE_SIZE);
 		}
 		port->inbuf = NULL;
 		buf = get_inbuf(port);

@@ -485,7 +526,7 @@ static void reclaim_consumed_buffers(struct port *port)
 		return;
 	}
 	while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
-		kfree(buf);
+		free_databuf(port->portdev->vdev, len, buf);
 		port->outvq_full = false;
 	}
 }

@@ -672,6 +713,8 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 	char *buf;
 	ssize_t ret;
 	bool nonblock;
+	struct virtio_device *vdev;
+	dma_addr_t dma;
 
 	/* Userspace could be out to fool us */
 	if (!count)

@@ -694,9 +737,10 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 	if (!port->guest_connected)
 		return -ENODEV;
 
+	vdev = port->portdev->vdev;
 	count = min((size_t)(32 * 1024), count);
 
-	buf = kmalloc(count, GFP_KERNEL);
+	buf = alloc_databuf(vdev, count, &dma, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 

@@ -720,7 +764,8 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 		goto out;
 
 free_buf:
-	kfree(buf);
+	free_databuf(vdev, count, buf);
+
 out:
 	return ret;
 }

@@ -1102,7 +1147,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
 
 	nr_added_bufs = 0;
 	do {
-		buf = alloc_buf(PAGE_SIZE);
+		buf = alloc_buf(vq, PAGE_SIZE);
 		if (!buf)
 			break;
 

@@ -1110,7 +1155,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
 		ret = add_inbuf(vq, buf);
 		if (ret < 0) {
 			spin_unlock_irq(lock);
-			free_buf(buf);
+			free_buf(vq, buf, PAGE_SIZE);
 			break;
 		}
 		nr_added_bufs++;

@@ -1234,7 +1279,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 
 free_inbufs:
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-		free_buf(buf);
+		free_buf(port->in_vq, buf, PAGE_SIZE);
 free_device:
 	device_destroy(pdrvdata.class, port->dev->devt);
 free_cdev:

@@ -1276,7 +1321,7 @@ static void remove_port_data(struct port *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);
+		free_buf(port->in_vq, buf, PAGE_SIZE);
 }
 
 /*

@@ -1478,7 +1523,7 @@ static void control_work_handler(struct work_struct *work)
 		if (add_inbuf(portdev->c_ivq, buf) < 0) {
 			dev_warn(&portdev->vdev->dev,
 				 "Error adding buffer to queue\n");
-			free_buf(buf);
+			free_buf(portdev->c_ivq, buf, PAGE_SIZE);
 		}
 	}
 	spin_unlock(&portdev->cvq_lock);

@@ -1674,10 +1719,10 @@ static void remove_controlq_data(struct ports_device *portdev)
 		return;
 
 	while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
-		free_buf(buf);
+		free_buf(portdev->c_ivq, buf, PAGE_SIZE);
 
 	while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
-		free_buf(buf);
+		free_buf(portdev->c_ivq, buf, PAGE_SIZE);
 }
 
 /*

@@ -1836,6 +1881,7 @@ static struct virtio_device_id id_table[] = {
 static unsigned int features[] = {
 	VIRTIO_CONSOLE_F_SIZE,
 	VIRTIO_CONSOLE_F_MULTIPORT,
+	VIRTIO_CONSOLE_F_DMA_MEM,
 };
 
 #ifdef CONFIG_PM

include/linux/virtio_console.h +1 -0

index bdf4b00..b27f7fa 100644
@@ -38,6 +38,7 @@
 /* Feature bits */
 #define VIRTIO_CONSOLE_F_SIZE	0	/* Does host provide console size? */
 #define VIRTIO_CONSOLE_F_MULTIPORT 1	/* Does host provide multiple ports? */
+#define VIRTIO_CONSOLE_F_DMA_MEM 2	/* Use DMA memory in vrings */
 
 #define VIRTIO_CONSOLE_BAD_ID		(~(u32)0)
 

Signature · 1 line

1.7.5.4

How does access to descriptors work in this setup?

drivers/char/virtio_console.c static inline bool use_multiport(struct ports_device *portdev) · line 338

Quoted 2 lines

drivers/char/virtio_console.c +1 -1

-static void free_buf(struct port_buffer *buf)
+/* Allcoate data buffer from DMA memory if requested */

typo

Quoted 14 lines

drivers/char/virtio_console.c +12 -1

+static inline void *
+alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t *dma_handle,
+                gfp_t flag)
 {
-     kfree(buf->buf);
+#ifdef CONFIG_HAS_DMA
+     if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+             struct device *dev = &vdev->dev;
+             /*
+              * Allocate DMA memory from ancestors. Finding the ancestor
+              * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
+              * implemented.
+              */
+             dev = dev->parent ? dev->parent : dev;

drivers/char/virtio_console.c static inline bool use_multiport(struct ports_device *portdev) · line 355

Quoted 4 lines

drivers/char/virtio_console.c +4 -0

+             dev = dev->parent ? dev->parent : dev;
+             return dma_alloc_coherent(dev, size, dma_handle, flag);
+     }
+#endif

Are these ifdefs really needed? If DMA_MEM is set, can't we use dma_alloc_coherent unconditionally?

Hi Michael,

Quoted 1 line

How does access to descriptors work in this setup?

When the ring is setup by remoteproc the descriptors are also allocated using dma_alloc_coherent().

Quoted 4 lines

drivers/char/virtio_console.c static inline bool use_multiport(struct ports_device *portdev) · line 338

Quoted 2 lines

drivers/char/virtio_console.c +1 -1

-static void free_buf(struct port_buffer *buf)
+/* Allcoate data buffer from DMA memory if requested */

typo

Thanks.

Quoted 14 lines

Quoted 14 lines

drivers/char/virtio_console.c +12 -1

+static inline void *
+alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t *dma_handle,
+                gfp_t flag)
 {
-     kfree(buf->buf);
+#ifdef CONFIG_HAS_DMA
+     if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
+             struct device *dev = &vdev->dev;
+             /*
+              * Allocate DMA memory from ancestors. Finding the ancestor
+              * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
+              * implemented.
+              */
+             dev = dev->parent ? dev->parent : dev;

Quoted 8 lines

drivers/char/virtio_console.c static inline bool use_multiport(struct ports_device *portdev) · line 355

Quoted 4 lines

drivers/char/virtio_console.c +4 -0

+             dev = dev->parent ? dev->parent : dev;
+             return dma_alloc_coherent(dev, size, dma_handle, flag);
+     }
+#endif

Are these ifdefs really needed? If DMA_MEM is set, can't we use dma_alloc_coherent unconditionally?

If an architecture do not support DMA you will get a link error: "unknown symbol" for dma_alloc_coherent.

Regards,
Sjur

Also let's add a wrapper at top of file so ifdefs
do not litter the code like this. For example:

#ifdef CONFIG_HAS_DMA
#define VIRTIO_CONSOLE_HAS_DMA (1)
#else
#define VIRTIO_CONSOLE_HAS_DMA (0)
#endif

Now use if instead of ifdef.

The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you don't have DMA!

Quoted 2 lines

The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you don't have DMA!

OK, so the feature table could be done like this:

static unsigned int features[] = {
VIRTIO_CONSOLE_F_SIZE,
VIRTIO_CONSOLE_F_MULTIPORT,
#if VIRTIO_CONSOLE_HAS_DMA
VIRTIO_CONSOLE_F_DMA_MEM,
#endif
}

If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM when DMA is not supported, virtio will do BUG_ON() from virtio_check_driver_offered_feature().

Is this acceptable or should we add a check in virtcons_probe() and let the probing fail instead?

E.g:

	/* Refuse to bind if F_DMA_MEM request cannot be met */
	if (!VIRTIO_CONSOLE_HAS_DMA &&
	    (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
		dev_err(&vdev->dev,
			"DMA_MEM requested, but arch does not support DMA\n");
		err = -EINVAL;
		goto fail;
	}

Regards,
Sjur

Quoted 17 lines

If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM when DMA is not supported, virtio will do BUG_ON() from virtio_check_driver_offered_feature().

Is this acceptable or should we add a check in virtcons_probe() and let the probing fail instead?

E.g:

      /* Refuse to bind if F_DMA_MEM request cannot be met */
      if (!VIRTIO_CONSOLE_HAS_DMA &&
          (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
              dev_err(&vdev->dev,
                      "DMA_MEM requested, but arch does not support DMA\n");
              err = -EINVAL;
              goto fail;
      }

Quoted 2 lines

Regards,
Sjur

Failing probe would be cleaner. But there is still a problem:
old driver will happily bind to that device and then fail to work, right?

virtio pci has revision id for this, but remoteproc doesn't seem to have anything similar. Or did I miss it?

If not -
we probably need to use a different
device id, and not a feature bit.

Hi Michael.

Quoted 20 lines

Quoted 19 lines

If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM when DMA is not supported, virtio will do BUG_ON() from virtio_check_driver_offered_feature().

Is this acceptable or should we add a check in virtcons_probe() and let the probing fail instead?

E.g:

      /* Refuse to bind if F_DMA_MEM request cannot be met */
      if (!VIRTIO_CONSOLE_HAS_DMA &&
          (vdev->config->get_features(vdev) & (1 << VIRTIO_CONSOLE_F_DMA_MEM))){
              dev_err(&vdev->dev,
                      "DMA_MEM requested, but arch does not support DMA\n");
              err = -EINVAL;
              goto fail;
      }

Regards,
Sjur

Quoted 3 lines

Failing probe would be cleaner. But there is still a problem:
old driver will happily bind to that device and then fail to work, right?

Not just fail to work, the kernel will panic on the BUG_ON(). Remoteproc gets the virtio configuration from firmware loaded from user space. So this type of problem might be triggered for other virtio drivers as well.

Quoted 3 lines

virtio pci has revision id for this, but remoteproc doesn't seem to have anything similar. Or did I miss it?

No there are currently no sanity check of
virtio type and feature bits in remoteproc.
One option may be to add this...

Quoted 3 lines

If not -
we probably need to use a different
device id, and not a feature bit.

But if I create a new virtio console type, remoteproc could still call the existing virtio_console with random bad feature bits, causing kernel panic.

Even if we fix this particular problem, the general problem still exists: bogus virtio declarations in remoteproc's firmware may cause BUG_ON().

(Note the fundamental difference
between visualizations and remoteproc. For remoteproc the virtio configuration comes from binaries loaded from user space).

So maybe we should look for a more generic solution, e.g. changing virtio probe functionality so that devices with bad feature bits will not trigger BUG_ON(), but rather refuse to bind the driver.

Regards,
Sjur

Quoted 4 lines

Not just fail to work, the kernel will panic on the BUG_ON(). Remoteproc gets the virtio configuration from firmware loaded from user space. So this type of problem might be triggered for other virtio drivers as well.

how?

Quoted 3 lines

Even if we fix this particular problem, the general problem still exists: bogus virtio declarations in remoteproc's firmware may cause BUG_ON().

which BUG_ON exactly?

Quoted 2 lines

If an architecture do not support DMA you will get a link error: "unknown symbol" for dma_alloc_coherent.

Yes, it even seems intentional.
But I have a question: can the device work without DMA?

Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?

If yes you should just fail load.

Also let's add a wrapper at top of file so ifdefs
do not litter the code like this. For example:

#ifdef CONFIG_HAS_DMA
#define VIRTIO_CONSOLE_HAS_DMA (1)
#else
#define VIRTIO_CONSOLE_HAS_DMA (0)
#endif

Hi Michael,

Quoted 5 lines

Quoted 2 lines

If an architecture do not support DMA you will get a link error: "unknown symbol" for dma_alloc_coherent.

Yes, it even seems intentional.
But I have a question: can the device work without DMA?

The main dependency is actually on the dma-allocation. In my case I do dma_declare_coherent_memory() with the memory area shared with the remote device as argument. Subsequent calls to dma_alloc_coherent() will then allocate from this memory area.

Quoted 1 line

Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?

Yes.dma_alloc_coherent to work.

Quoted 1 line

If yes you should just fail load.

Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding VIRTIO_CONSOLE_F_DMA_MEM to the feature array.

Quoted 8 lines

Also let's add a wrapper at top of file so ifdefs
do not litter the code like this. For example:

#ifdef CONFIG_HAS_DMA
#define VIRTIO_CONSOLE_HAS_DMA (1)
#else
#define VIRTIO_CONSOLE_HAS_DMA (0)
#endif

OK, good point. Perhaps we could even exploit gcc's ability to remove dead code and do something like this:

       if (VIRTIO_CONSOLE_HAS_DMA &&
           virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {

...

This does not give any linker warnings when compiling for UML (no DMA).

Regards,
Sjur

Exactly. Though if we just fail load it will be much less code.

Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.

Hi Michael,

Quoted 7 lines

Exactly. Though if we just fail load it will be much less code.

Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.

I see - so in order to avoid the binding between driver and device there are two options I guess. Either make virtio_dev_match() or virtcons_probe() fail. Neither of them seems like the obvious choice.

Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match between device and driver in virtcons_probe() is the lesser evil?

Regards,
Sjur

Quoted 14 lines · Sjur Brændeland

Hi Michael,

Quoted 7 lines

Exactly. Though if we just fail load it will be much less code.

Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.

I see - so in order to avoid the binding between driver and device there are two options I guess. Either make virtio_dev_match() or virtcons_probe() fail. Neither of them seems like the obvious choice.

Quoted 5 lines

Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match between device and driver in virtcons_probe() is the lesser evil?

Regards,
Sjur

A simplest thing to do is change dev id. rusty?

Quoted 15 lines · Michael S. Tsirkin

Quoted 14 lines · Sjur Brændeland

Hi Michael,

Quoted 7 lines

Exactly. Though if we just fail load it will be much less code.

Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.

I see - so in order to avoid the binding between driver and device there are two options I guess. Either make virtio_dev_match() or virtcons_probe() fail. Neither of them seems like the obvious choice.

Quoted 7 lines

Quoted 5 lines

Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match between device and driver in virtcons_probe() is the lesser evil?

Regards,
Sjur

A simplest thing to do is change dev id. rusty?

For generic usage, this is correct. But my opinion is that fallback on feature non-ack is quality-of-implementation issue: great to have, but there are cases where you just want to fail with "you're too old".

And in this case, an old system simply will never work. So it's a question of how graceful the failure is.

Can your userspace loader can refuse to proceed if the driver doesn't ack the bits? If so, it's simpler than a whole new ID.

Cheers,
Rusty.

Quoted 28 lines · Rusty Russell

Quoted 15 lines · Michael S. Tsirkin

Quoted 14 lines · Sjur Brændeland

Hi Michael,

Quoted 7 lines

Exactly. Though if we just fail load it will be much less code.

Generally, using a feature bit for this is a bit of a problem though:
normally driver is expected to be able to simply ignore
a feature bit. In this case driver is required to
do something so a feature bit is not a good fit.
I am not sure what the right thing to do is.

I see - so in order to avoid the binding between driver and device there are two options I guess. Either make virtio_dev_match() or virtcons_probe() fail. Neither of them seems like the obvious choice.

Quoted 7 lines

Quoted 5 lines

Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match between device and driver in virtcons_probe() is the lesser evil?

Regards,
Sjur

A simplest thing to do is change dev id. rusty?

For generic usage, this is correct. But my opinion is that fallback on feature non-ack is quality-of-implementation issue: great to have, but there are cases where you just want to fail with "you're too old".

Quoted 8 lines

And in this case, an old system simply will never work. So it's a question of how graceful the failure is.

Can your userspace loader can refuse to proceed if the driver doesn't ack the bits? If so, it's simpler than a whole new ID.

Cheers,
Rusty.

Yes but how can it signal guest that it will never proceed?

Also grep for BUG_ON in core found this:

       drv->remove(dev);

       /* Driver should have reset device. */
       BUG_ON(dev->config->get_status(dev));

I think below is what Sjur refers to.
I think below is a good idea for 3.6. Thoughts?

--->

virtio: don't crash when device is buggy

Because of a sanity check in virtio_dev_remove, a buggy device can crash kernel. And in case of rproc it's userspace so it's not a good idea. We are unloading a driver so how bad can it be?
Be less aggressive in handling this error: if it's a driver bug, warning once should be enough.

Signed-off-by Michael S. Tsirkin <[email protected]>

--

drivers/virtio/virtio.c +1 -1

index c3b3f7f..1e8659c 100644
@@ -159,7 +159,7 @@ static int virtio_dev_remove(struct device *_d)
 	drv->remove(dev);
 
 	/* Driver should have reset device. */
-	BUG_ON(dev->config->get_status(dev));
+	WARN_ON_ONCE(dev->config->get_status(dev));
 
 	/* Acknowledge the device's existence again. */
 	add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);

Signature · 2 lines

-- 
MST

Quoted 9 lines

virtio: don't crash when device is buggy

Because of a sanity check in virtio_dev_remove, a buggy device can crash kernel. And in case of rproc it's userspace so it's not a good idea. We are unloading a driver so how bad can it be?
Be less aggressive in handling this error: if it's a driver bug, warning once should be enough.

Signed-off-by Michael S. Tsirkin <[email protected]>

Rusty?

Quoted 1 line

A simplest thing to do is change dev id. rusty?

For generic usage, this is correct. But my opinion is that fallback on feature non-ack is quality-of-implementation issue: great to have, but there are cases where you just want to fail with "you're too old".

And in this case, an old system simply will never work. So it's a question of how graceful the failure is.

Can your userspace loader can refuse to proceed if the driver doesn't ack the bits? If so, it's simpler than a whole new ID.

Because of a sanity check in virtio_dev_remove, a buggy device can crash kernel. And in case of rproc it's userspace so it's not a good idea. We are unloading a driver so how bad can it be?
Be less aggressive in handling this error: if it's a driver bug, warning once should be enough.

Signed-off-by Michael S. Tsirkin <[email protected]>

--

3.6 material?

Quoted 10 lines · Michael S. Tsirkin

Because of a sanity check in virtio_dev_remove, a buggy device can crash kernel. And in case of rproc it's userspace so it's not a good idea. We are unloading a driver so how bad can it be?
Be less aggressive in handling this error: if it's a driver bug, warning once should be enough.

Signed-off-by Michael S. Tsirkin <[email protected]>

--

Quoted 1 line

3.6 material?

I have already applied, this, but it's not for stable, since it's a "theoretical bugfix". That check has been in there forever and noone AFAIK has actually struck it.

Cheers,
Rusty.

Quoted 18 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

Because of a sanity check in virtio_dev_remove, a buggy device can crash kernel. And in case of rproc it's userspace so it's not a good idea. We are unloading a driver so how bad can it be?
Be less aggressive in handling this error: if it's a driver bug, warning once should be enough.

Signed-off-by Michael S. Tsirkin <[email protected]>

--

3.6 material?

I have already applied, this, but it's not for stable, since it's a "theoretical bugfix". That check has been in there forever and noone AFAIK has actually struck it.

Quoted 2 lines

Cheers,
Rusty.

Yes but can't malicious userspace trigger this with remoteproc? If yes it's not a question of whether anyone has struck it since people don't normally run malicious userspace :)

Quoted 18 lines · Rusty Russell

Quoted 11 lines · Michael S. Tsirkin

Because of a sanity check in virtio_dev_remove, a buggy device can crash kernel. And in case of rproc it's userspace so it's not a good idea. We are unloading a driver so how bad can it be?
Be less aggressive in handling this error: if it's a driver bug, warning once should be enough.

Signed-off-by Michael S. Tsirkin <[email protected]>

--

3.6 material?

I have already applied, this, but it's not for stable, since it's a "theoretical bugfix". That check has been in there forever and noone AFAIK has actually struck it.

Quoted 2 lines

Cheers,
Rusty.

Yes but can't malicious userspace trigger this with remoteproc? If yes it's not a question of whether anyone has struck it since people don't normally run malicious userspace :)