[PATCH] pci: msi: return error code instead of only warn

[PATCH] pci: msi: return error code instead of only warn

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

The drivers may enable the msi/msix more than once.
Actually, this is the driver bug. But the bug may panic
the kernel and should be fixed. We can return the error
code instead of WARN_ON, to tell the drivers that the
msi/msix is already enabled.

When enable them again, the call tree is shown on kernel
3.10.0, but linux upstream in the same way.

Call Trace:
	dump_stack+0x19/0x1b
	warn_slowpath_common+0x70/0xb0
	warn_slowpath_null+0x1a/0x20
	pci_enable_msix+0x3c9/0x3e0
	igbuio_pci_open+0x72/0x300 [igb_uio]
	uio_open+0xcc/0x120 [uio]
	chrdev_open+0xa1/0x1e0
	[...]
	do_sys_open+0xf3/0x1f0
	SyS_open+0x1e/0x20
	system_call_fastpath+0x16/0x1b
	------------[ cut here ]------------
	WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0xa5/0xd0()
	sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:03.0/0000:01:00.1/msi_irqs'

Call Trace:
	dump_stack+0x19/0x1b
	warn_slowpath_common+0x70/0xb0
	warn_slowpath_fmt+0x5c/0x80
	sysfs_add_one+0xa5/0xd0
	create_dir+0x7c/0xe0
	sysfs_create_subdir+0x1c/0x20
	internal_create_group+0x6d/0x290
	sysfs_create_groups+0x4a/0xa0
	populate_msi_sysfs+0x1cd/0x210
	pci_enable_msix+0x31c/0x3e0
	igbuio_pci_open+0x72/0x300 [igb_uio]
	uio_open+0xcc/0x120 [uio]
	chrdev_open+0xa1/0x1e0
	[...]
	do_sys_open+0xf3/0x1f0
	SyS_open+0x1e/0x20
	system_call_fastpath+0x16/0x1b
	---[ end trace 11042e2848880209 ]---
	Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffffa056b4fa

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 drivers/pci/msi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 4d88afd..4326e27 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -958,7 +958,6 @@ static int __pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
 			}
 		}
 	}
-	WARN_ON(!!dev->msix_enabled);
 
 	/* Check whether driver already requested for MSI irq */
 	if (dev->msi_enabled) {
@@ -1028,8 +1027,6 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
 	if (!pci_msi_supported(dev, minvec))
 		return -EINVAL;
 
-	WARN_ON(!!dev->msi_enabled);
-
 	/* Check whether driver already requested MSI-X irqs */
 	if (dev->msix_enabled) {
 		pci_info(dev, "can't enable MSI (MSI-X already enabled)\n");
@@ -1039,6 +1036,9 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
 	if (maxvec < minvec)
 		return -ERANGE;
 
+	if (WARN_ON_ONCE(!!dev->msi_enabled))
+		return  -EINVAL;
+
 	nvec = pci_msi_vec_count(dev);
 	if (nvec < 0)
 		return nvec;
@@ -1087,6 +1087,9 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
 	if (maxvec < minvec)
 		return -ERANGE;
 
+	if (WARN_ON_ONCE(!!dev->msix_enabled))
+		return  -EINVAL;
+
 	for (;;) {
 		if (affd) {
 			nvec = irq_calc_affinity_vectors(minvec, nvec, affd);
-- 
1.8.3.1

Re: [PATCH] pci: msi: return error code instead of only warn

[Christoph Hellwig]

On Mon, Sep 24, 2018 at 07:00:41AM -0700, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> The drivers may enable the msi/msix more than once.
> Actually, this is the driver bug. But the bug may panic
> the kernel and should be fixed. We can return the error
> code instead of WARN_ON, to tell the drivers that the
> msi/msix is already enabled.
> 
> When enable them again, the call tree is shown on kernel
> 3.10.0, but linux upstream in the same way.

You can use up to 73 characters per line for your changelog, and doing so
makes it a little more readable.

> +	if (WARN_ON_ONCE(!!dev->msi_enabled))
> +		return  -EINVAL;
> +
>  	nvec = pci_msi_vec_count(dev);
>  	if (nvec < 0)
>  		return nvec;
> @@ -1087,6 +1087,9 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
>  	if (maxvec < minvec)
>  		return -ERANGE;
>  
> +	if (WARN_ON_ONCE(!!dev->msix_enabled))
> +		return  -EINVAL;
> +

You can drop the "!!", WARN_ON_ONCE will do the implicit bool conversion
for you.

Except for that the patch looks good to me.

Re: [PATCH] pci: msi: return error code instead of only warn

[Bjorn Helgaas]

On Mon, Sep 24, 2018 at 07:00:41AM -0700, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> The drivers may enable the msi/msix more than once.
> Actually, this is the driver bug. But the bug may panic
> the kernel and should be fixed. We can return the error
> code instead of WARN_ON, to tell the drivers that the
> msi/msix is already enabled.
> 
> When enable them again, the call tree is shown on kernel
> 3.10.0, but linux upstream in the same way.
> 
> Call Trace:
> 	dump_stack+0x19/0x1b
> 	warn_slowpath_common+0x70/0xb0
> 	warn_slowpath_null+0x1a/0x20
> 	pci_enable_msix+0x3c9/0x3e0
> 	igbuio_pci_open+0x72/0x300 [igb_uio]
> 	uio_open+0xcc/0x120 [uio]
> 	chrdev_open+0xa1/0x1e0
> 	[...]
> 	do_sys_open+0xf3/0x1f0
> 	SyS_open+0x1e/0x20
> 	system_call_fastpath+0x16/0x1b
> 	------------[ cut here ]------------
> 	WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0xa5/0xd0()
> 	sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:03.0/0000:01:00.1/msi_irqs'
> 
> Call Trace:
> 	dump_stack+0x19/0x1b
> 	warn_slowpath_common+0x70/0xb0
> 	warn_slowpath_fmt+0x5c/0x80
> 	sysfs_add_one+0xa5/0xd0
> 	create_dir+0x7c/0xe0
> 	sysfs_create_subdir+0x1c/0x20
> 	internal_create_group+0x6d/0x290
> 	sysfs_create_groups+0x4a/0xa0
> 	populate_msi_sysfs+0x1cd/0x210
> 	pci_enable_msix+0x31c/0x3e0
> 	igbuio_pci_open+0x72/0x300 [igb_uio]
> 	uio_open+0xcc/0x120 [uio]
> 	chrdev_open+0xa1/0x1e0
> 	[...]
> 	do_sys_open+0xf3/0x1f0
> 	SyS_open+0x1e/0x20
> 	system_call_fastpath+0x16/0x1b
> 	---[ end trace 11042e2848880209 ]---
> 	Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffffa056b4fa
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

I cleaned up whitespace (extra spaces after "return") and !! (as Christoph
mentioned) and applied the patch below to pci/msi for v4.20, thanks!


commit 4c1ef72e9b71a19fb405ebfcd37c0a5e16fa44ca
Author: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Date:   Mon Sep 24 07:00:41 2018 -0700

    PCI/MSI: Warn and return error if driver enables MSI/MSI-X twice
    
    It is a serious driver defect to enable MSI or MSI-X more than once.  Doing
    so may panic the kernel as in the stack trace below:
    
      Call Trace:
        sysfs_add_one+0xa5/0xd0
        create_dir+0x7c/0xe0
        sysfs_create_subdir+0x1c/0x20
        internal_create_group+0x6d/0x290
        sysfs_create_groups+0x4a/0xa0
        populate_msi_sysfs+0x1cd/0x210
        pci_enable_msix+0x31c/0x3e0
        igbuio_pci_open+0x72/0x300 [igb_uio]
        uio_open+0xcc/0x120 [uio]
        chrdev_open+0xa1/0x1e0
        [...]
        do_sys_open+0xf3/0x1f0
        SyS_open+0x1e/0x20
        system_call_fastpath+0x16/0x1b
        ---[ end trace 11042e2848880209 ]---
        Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffffa056b4fa
    
    We want to keep the WARN_ON() and stack trace so the driver can be fixed,
    but we can avoid the kernel panic by returning an error.  We may still get
    warnings like this:
    
      Call Trace:
        pci_enable_msix+0x3c9/0x3e0
        igbuio_pci_open+0x72/0x300 [igb_uio]
        uio_open+0xcc/0x120 [uio]
        chrdev_open+0xa1/0x1e0
        [...]
        do_sys_open+0xf3/0x1f0
        SyS_open+0x1e/0x20
        system_call_fastpath+0x16/0x1b
        ------------[ cut here ]------------
        WARNING: at fs/sysfs/dir.c:526 sysfs_add_one+0xa5/0xd0()
        sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:03.0/0000:01:00.1/msi_irqs'
    
    Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
    [bhelgaas: changelog, fix patch whitespace, remove !!]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index f2ef896464b3..af24ed50a245 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -958,7 +958,6 @@ static int __pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
 			}
 		}
 	}
-	WARN_ON(!!dev->msix_enabled);
 
 	/* Check whether driver already requested for MSI irq */
 	if (dev->msi_enabled) {
@@ -1028,8 +1027,6 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
 	if (!pci_msi_supported(dev, minvec))
 		return -EINVAL;
 
-	WARN_ON(!!dev->msi_enabled);
-
 	/* Check whether driver already requested MSI-X irqs */
 	if (dev->msix_enabled) {
 		pci_info(dev, "can't enable MSI (MSI-X already enabled)\n");
@@ -1039,6 +1036,9 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
 	if (maxvec < minvec)
 		return -ERANGE;
 
+	if (WARN_ON_ONCE(dev->msi_enabled))
+		return -EINVAL;
+
 	nvec = pci_msi_vec_count(dev);
 	if (nvec < 0)
 		return nvec;
@@ -1087,6 +1087,9 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
 	if (maxvec < minvec)
 		return -ERANGE;
 
+	if (WARN_ON_ONCE(dev->msix_enabled))
+		return -EINVAL;
+
 	for (;;) {
 		if (affd) {
 			nvec = irq_calc_affinity_vectors(minvec, nvec, affd);