On 6/29/2018 11:30 AM, Jeff Guo wrote:
> When hot unplug device, the kernel will release the device resource in the
> kernel side, such as the fd sys file will disappear, and the irq will be
> released. At this time, if igb uio driver still try to release this
> resource, it will cause kernel crash. On the other hand, something like
> interrupt disabling do not automatically process in kernel side. If not
> handler it, this redundancy and dirty thing will affect the interrupt
> resource be used by other device. So the igb_uio driver have to check the
> hot plug status, and the corresponding process should be taken in igb uio
> driver.
>
> This patch propose to add structure of rte_udev_state into rte_uio_pci_dev
> of igb_uio kernel driver, which will record the state of uio device, such
> as probed/opened/released/removed/unplug. When detect the unexpected
> removal which cause of hot unplug behavior, it will corresponding disable
> interrupt resource, while for the part of releasement which kernel have
> already handle, just skip it to avoid double free or null pointer kernel
> crash issue.
>
> Signed-off-by: Jeff Guo <jia....@intel.com>
> ---
> v4->v3:
> no change
> ---
> kernel/linux/igb_uio/igb_uio.c | 50
> +++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
> index b3233f1..d301302 100644
> --- a/kernel/linux/igb_uio/igb_uio.c
> +++ b/kernel/linux/igb_uio/igb_uio.c
> @@ -19,6 +19,15 @@
>
> #include "compat.h"
>
> +/* uio pci device state */
> +enum rte_udev_state {
> + RTE_UDEV_PROBED,
> + RTE_UDEV_OPENNED,
> + RTE_UDEV_RELEASED,
> + RTE_UDEV_REMOVED,
> + RTE_UDEV_UNPLUG
> +};
> +
> /**
> * A structure describing the private information for a uio device.
> */
> @@ -28,6 +37,7 @@ struct rte_uio_pci_dev {
> enum rte_intr_mode mode;
> struct mutex lock;
> int refcnt;
> + enum rte_udev_state state;
> };
>
> static char *intr_mode;
> @@ -194,12 +204,20 @@ igbuio_pci_irqhandler(int irq, void *dev_id)
> {
> struct rte_uio_pci_dev *udev = (struct rte_uio_pci_dev *)dev_id;
> struct uio_info *info = &udev->info;
> + struct pci_dev *pdev = udev->pdev;
>
> /* Legacy mode need to mask in hardware */
> if (udev->mode == RTE_INTR_MODE_LEGACY &&
> !pci_check_and_mask_intx(udev->pdev))
> return IRQ_NONE;
>
> + /* check the uevent of the kobj */
> + if ((&pdev->dev.kobj)->state_remove_uevent_sent == 1) {
> + dev_notice(&pdev->dev, "device:%s, sent remove uevent!\n",
> + (&pdev->dev.kobj)->name);
> + udev->state = RTE_UDEV_UNPLUG;
> + }
I guess commit log says kernel can remove device, if so do we need any locking
before accessing dev?
> +
> uio_event_notify(info);
>
> /* Message signal mode, no share IRQ and automasked */
> @@ -308,7 +326,6 @@ igbuio_pci_disable_interrupts(struct rte_uio_pci_dev
> *udev)
> #endif
> }
>
> -
> /**
> * This gets called while opening uio device file.
> */
> @@ -330,24 +347,33 @@ igbuio_pci_open(struct uio_info *info, struct inode
> *inode)
>
> /* enable interrupts */
> err = igbuio_pci_enable_interrupts(udev);
> - mutex_unlock(&udev->lock);
> if (err) {
> dev_err(&dev->dev, "Enable interrupt fails\n");
> + pci_clear_master(dev);
> return err;
> }
> + udev->state = RTE_UDEV_OPENNED;
> + mutex_unlock(&udev->lock);
> return 0;
> }
>
> +/**
> + * This gets called while closing uio device file.
> + */
> static int
> igbuio_pci_release(struct uio_info *info, struct inode *inode)
> {
> +
> struct rte_uio_pci_dev *udev = info->priv;
> struct pci_dev *dev = udev->pdev;
>
> + if (udev->state == RTE_UDEV_REMOVED)
> + return 0;
> +
> mutex_lock(&udev->lock);
> if (--udev->refcnt > 0) {
> mutex_unlock(&udev->lock);
> - return 0;
> + return -1;
> }
>
> /* disable interrupts */
> @@ -355,7 +381,7 @@ igbuio_pci_release(struct uio_info *info, struct inode
> *inode)
>
> /* stop the device from further DMA */
> pci_clear_master(dev);
> -
> + udev->state = RTE_UDEV_RELEASED;
> mutex_unlock(&udev->lock);
> return 0;
> }
> @@ -557,6 +583,7 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> (unsigned long long)map_dma_addr, map_addr);
> }
>
> + udev->state = RTE_UDEV_PROBED;
> return 0;
>
> fail_remove_group:
> @@ -573,11 +600,24 @@ igbuio_pci_probe(struct pci_dev *dev, const struct
> pci_device_id *id)
> static void
> igbuio_pci_remove(struct pci_dev *dev)
> {
> +
> struct rte_uio_pci_dev *udev = pci_get_drvdata(dev);
> + int ret;
> +
> + /* handler hot unplug */
> + if (udev->state == RTE_UDEV_OPENNED ||
> + udev->state == RTE_UDEV_UNPLUG) {
> + dev_notice(&dev->dev, "Unexpected removal!\n");
> + ret = igbuio_pci_release(&udev->info, NULL);
> + if (ret)
> + return;
> + udev->state = RTE_UDEV_REMOVED;
> + return;
> + }
>
> mutex_destroy(&udev->lock);
> - sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
> uio_unregister_device(&udev->info);
> + sysfs_remove_group(&dev->dev.kobj, &dev_attr_grp);
> igbuio_pci_release_iomem(&udev->info);
> pci_disable_device(dev);
> pci_set_drvdata(dev, NULL);
>
