kernel panic becase pci register more than once

kernel panic becase pci register more than once

[Tonghao Zhang]

Hi all,
When I enable pci msi or msix more than once, the kernel will panic.
Now we just use the WARN_ON, should we add check for that has been
enabled.

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index f2ef896..324840f 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -958,7 +958,9 @@ static int __pci_enable_msix(struct pci_dev *dev,
struct msix_entry *entries,
                        }
                }
        }
-       WARN_ON(!!dev->msix_enabled);
+
+       if (dev->msix_enabled)
+               return -EINVAL;

        /* Check whether driver already requested for MSI irq */
        if (dev->msi_enabled) {
@@ -1028,7 +1030,8 @@ 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);
+       if (dev->msi_enabled)
+               return -EINVAL;

        /* Check whether driver already requested MSI-X irqs */
        if (dev->msix_enabled) {

Re: kernel panic becase pci register more than once

[Bjorn Helgaas]

[+cc Christoph]

On Sun, Sep 09, 2018 at 04:26:37PM +0800, Tonghao Zhang wrote:
> Hi all,
> When I enable pci msi or msix more than once, the kernel will panic.
> Now we just use the WARN_ON, should we add check for that has been
> enabled.
> 

This needs a signed-off-by before I can apply it.  See
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n416

A WARN_ON() shouldn't cause the kernel to panic.  Do you mean that it
causes a backtrace?  I think that's the intended behavior, because
calling this twice would be a driver bug, and we want to find and fix
those bugs.

> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index f2ef896..324840f 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -958,7 +958,9 @@ static int __pci_enable_msix(struct pci_dev *dev,
> struct msix_entry *entries,
>                         }
>                 }
>         }
> -       WARN_ON(!!dev->msix_enabled);
> +
> +       if (dev->msix_enabled)
> +               return -EINVAL;
> 
>         /* Check whether driver already requested for MSI irq */
>         if (dev->msi_enabled) {
> @@ -1028,7 +1030,8 @@ 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);
> +       if (dev->msi_enabled)
> +               return -EINVAL;
> 
>         /* Check whether driver already requested MSI-X irqs */
>         if (dev->msix_enabled) {

Re: kernel panic becase pci register more than once

[Tonghao Zhang]

On Tue, Sep 18, 2018 at 3:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Christoph]
>
> On Sun, Sep 09, 2018 at 04:26:37PM +0800, Tonghao Zhang wrote:
> > Hi all,
> > When I enable pci msi or msix more than once, the kernel will panic.
> > Now we just use the WARN_ON, should we add check for that has been
> > enabled.
> >
>
> This needs a signed-off-by before I can apply it.  See
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n416
OK
> A WARN_ON() shouldn't cause the kernel to panic.  Do you mean that it
> causes a backtrace?  I think that's the intended behavior, because
> calling this twice would be a driver bug, and we want to find and fix
> those bugs.
Yes, you are right, but we can return error code when drivers call this twice.
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index f2ef896..324840f 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -958,7 +958,9 @@ static int __pci_enable_msix(struct pci_dev *dev,
> > struct msix_entry *entries,
> >                         }
> >                 }
> >         }
> > -       WARN_ON(!!dev->msix_enabled);
> > +
> > +       if (dev->msix_enabled)
> > +               return -EINVAL;
> >
> >         /* Check whether driver already requested for MSI irq */
> >         if (dev->msi_enabled) {
> > @@ -1028,7 +1030,8 @@ 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);
> > +       if (dev->msi_enabled)
> > +               return -EINVAL;
> >
> >         /* Check whether driver already requested MSI-X irqs */
> >         if (dev->msix_enabled) {

Re: kernel panic becase pci register more than once

[Bjorn Helgaas]

On Tue, Sep 18, 2018 at 10:00:11AM +0800, Tonghao Zhang wrote:
> On Tue, Sep 18, 2018 at 3:15 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sun, Sep 09, 2018 at 04:26:37PM +0800, Tonghao Zhang wrote:
> > > When I enable pci msi or msix more than once, the kernel will
> > > panic.  Now we just use the WARN_ON, should we add check for
> > > that has been enabled.

> > A WARN_ON() shouldn't cause the kernel to panic.  Do you mean that
> > it causes a backtrace?  I think that's the intended behavior,
> > because calling this twice would be a driver bug, and we want to
> > find and fix those bugs.

> Yes, you are right, but we can return error code when drivers call
> this twice.

That's possible.  We need a clear explanation for why we're making the
change, and that starts with a clear description of the problem, i.e.,
is the problem that the kernel crashes and a reboot is required, or is
the problem some messages in the dmesg log that look alarming but are
otherwise harmless?  I think the latter.

If we decide that an error return is better than the WARN_ON(), we
need to look at where the check is done.  For example, you currently
check in __pci_enable_msix(), which is only called from
__pci_enable_msix_range().  We should check for simple errors like
this as early as possible, which probably means
__pci_enable_msix_range().

> > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > > index f2ef896..324840f 100644
> > > --- a/drivers/pci/msi.c
> > > +++ b/drivers/pci/msi.c
> > > @@ -958,7 +958,9 @@ static int __pci_enable_msix(struct pci_dev *dev,
> > > struct msix_entry *entries,
> > >                         }
> > >                 }
> > >         }
> > > -       WARN_ON(!!dev->msix_enabled);
> > > +
> > > +       if (dev->msix_enabled)
> > > +               return -EINVAL;
> > >
> > >         /* Check whether driver already requested for MSI irq */
> > >         if (dev->msi_enabled) {
> > > @@ -1028,7 +1030,8 @@ 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);
> > > +       if (dev->msi_enabled)
> > > +               return -EINVAL;
> > >
> > >         /* Check whether driver already requested MSI-X irqs */
> > >         if (dev->msix_enabled) {

Re: kernel panic becase pci register more than once

[Christoph Hellwig]

On Mon, Sep 17, 2018 at 02:15:03PM -0500, Bjorn Helgaas wrote:
> A WARN_ON() shouldn't cause the kernel to panic.  Do you mean that it
> causes a backtrace?  I think that's the intended behavior, because
> calling this twice would be a driver bug, and we want to find and fix
> those bugs.
> 
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index f2ef896..324840f 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -958,7 +958,9 @@ static int __pci_enable_msix(struct pci_dev *dev,
> > struct msix_entry *entries,
> >                         }
> >                 }
> >         }
> > -       WARN_ON(!!dev->msix_enabled);
> > +
> > +       if (dev->msix_enabled)
> > +               return -EINVAL;

This is a grave driver bug, so we should keep a trace.  Then again
handling even grave driver bugs more gracefull is a good idea, so I'd
vote for:

	if (WARN_ON_ONCE(dev->msix_enabled))
		return -EINVAL;