Re: [PATCH] virtio: Don't access device data after unregistration.

On Tue, Sep 04, 2012 at 02:12:33PM +0200, Sjur Brændeland wrote:

Hi Michael,

quoted

quoted

quoted

quoted

Fix panic in virtio.c when CONFIG_DEBUG_SLAB is set.

What's the root cause of the panic?

I believe the cause of the panic is calling
ida_simple_remove(&virtio_index_ida, dev->index);
when the dev structure is "poisoned" after kfree.
It might be the "BUG_ON((int)id < 0)" that bites...

quoted

quoted

Use device_del() and put_device() instead of
device_unregister(), and access device data before
calling put_device().

quoted

Why does this help? Does device_unregister free the
device so dev->index access crashes?

Yes, if device ref-count is one when calling unregister
the device is freed.

Interesting. Where exactly?...

I was wrong here, the reason is not related to ref-count being
above one. The reason this issue do not show up in virtio_pci
is that the release function is a dummy:

[snip]
static void virtio_pci_release_dev(struct device *_d)
{
	/*
	 * No need for a release method as we allocate/free
	 * all devices together with the pci devices.
	 * Provide an empty one to avoid getting a warning from core.
	 */
}

The device structure uses a kref for reference counting the device.
In virtio_pci() the release function virtio_pci_release_dev()
will be called when the device is unregistered, but because the
release function is dummy, data isn't freed or reset at this point.
So for virtio devices created from virtio_pci my patch is not
currently needed.

However, empty release functions are not the preferred way, e.g look at
https://lkml.org/lkml/2012/4/3/301

[Greg K.H:]

quoted

quoted

quoted

quoted

+static void hsi_port_release(struct device *dev __maybe_unused)
+{
+}

As per the documentation in the kernel tree, I get to mock you
mercilessly for doing something as foolish as this.  You are not smarter
than the kernel and don't think that you got rid of the kernel warning
properly by doing this.  Do you think that I wrote that code for no good
reason?  The kernel was being nice and telling you what you did wrong,
don't try to fake it out, it's smarter than you are here.

But remoteproc frees the device memory in the release function
rproc_vdev_release() and needs this patch.

Regards,
Sjur

Okay, so let's add a comment in virtio in unregister
function. Also slightly preferable to just use device_unregister
IMHO. Something like the below?

	/*
	   device_unregister drops reference to device so put_device could
	   invoke release callback. In case that callback will free the device,
	   make sure we don't access device after this call.
	 */

	int index = dev->index;
        device_unregister(&dev->dev);
        ida_simple_remove(&virtio_index_ida, index);

-- 
MST