[PATCH] uio/gen-pci: don't enable interrupts in ISR

[PATCH] uio/gen-pci: don't enable interrupts in ISR

[Sebastian Andrzej Siewior]

As reported by Anthony in a short way:

|irq 17 handler uio_interrupt+0x0/0x68 enabled interrupts
|NIP [c0069d84] handle_irq_event_percpu+0x260/0x26c

The problem here is that spin_unlock_irq() enables the interrupts which
is a no-no in interrupt context because they always run with interrupts
disabled. This is the case even if IRQF_DISABLED has not been specified
since v2.6.35. Therefore this patch uses simple spin_locks().

Looking at it further here is only one spot where the lock is hold. So
giving the fact that an ISR is not reentrant and is not executed on two
cpus at the same time why do we need a lock here?
The driver lacks of ->irqcontrol function so I guess the interrupt is
enabled via direct PCI-access in userland. So there is _no_ protection
against read-modify-write of user vs kernel so even that
pci_block_user_cfg_access() is kinda pointless.
pci_block_user_cfg_access() in open() + ->irqcontrol() should fix this.
Since changes the API of this driver I leave it up to the relevant users
what to do.

Cc: <stable@kernel.org> # .35 and later
Reported-and-Tested-by: Anthony Foiani <anthony.foiani@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/uio/uio_pci_generic.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index fc22e1e..5c82681 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -57,7 +57,7 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 	BUILD_BUG_ON(PCI_COMMAND % 4);
 	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
 
-	spin_lock_irq(&gdev->lock);
+	spin_lock(&gdev->lock);
 	pci_block_user_cfg_access(pdev);
 
 	/* Read both command and status registers in a single 32-bit operation.
@@ -83,7 +83,7 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 done:
 
 	pci_unblock_user_cfg_access(pdev);
-	spin_unlock_irq(&gdev->lock);
+	spin_unlock(&gdev->lock);
 	return ret;
 }
 
-- 
1.7.4.4

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Michael S. Tsirkin]

On Thu, Aug 04, 2011 at 10:46:06PM +0200, Sebastian Andrzej Siewior wrote:
> As reported by Anthony in a short way:
> 
> |irq 17 handler uio_interrupt+0x0/0x68 enabled interrupts
> |NIP [c0069d84] handle_irq_event_percpu+0x260/0x26c
> 
> The problem here is that spin_unlock_irq() enables the interrupts which
> is a no-no in interrupt context because they always run with interrupts
> disabled. This is the case even if IRQF_DISABLED has not been specified
> since v2.6.35. Therefore this patch uses simple spin_locks().
> 
> Looking at it further here is only one spot where the lock is hold. So
> giving the fact that an ISR is not reentrant and is not executed on two
> cpus at the same time why do we need a lock here?

I'm not sure anymore. I think the idea was to use
it for synchronization down the road somehow,
but it never materialized. Let's drop that lock completely.

> The driver lacks of ->irqcontrol function so I guess the interrupt is
> enabled via direct PCI-access in userland.

Through sysfs.

> So there is _no_ protection
> against read-modify-write of user vs kernel so even that
> pci_block_user_cfg_access() is kinda pointless.

I didn't get that. pci_block_user_cfg_access is to prevent
sysfs access while we read modify-write the command register.
Isn't it effective for that?

> pci_block_user_cfg_access() in open() + ->irqcontrol() should fix this.

Why block in open? We don't access the device there, do we?

> Since changes the API of this driver I leave it up to the relevant users
> what to do.

Yes, changing API's not good, we need to keep existing userspace happy.

> Cc: <stable@kernel.org> # .35 and later
> Reported-and-Tested-by: Anthony Foiani <anthony.foiani@gmail.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/uio/uio_pci_generic.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> index fc22e1e..5c82681 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -57,7 +57,7 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
>  	BUILD_BUG_ON(PCI_COMMAND % 4);
>  	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
>  
> -	spin_lock_irq(&gdev->lock);
> +	spin_lock(&gdev->lock);
>  	pci_block_user_cfg_access(pdev);
>  
>  	/* Read both command and status registers in a single 32-bit operation.
> @@ -83,7 +83,7 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
>  done:
>  
>  	pci_unblock_user_cfg_access(pdev);
> -	spin_unlock_irq(&gdev->lock);
> +	spin_unlock(&gdev->lock);
>  	return ret;
>  }
>  
> -- 
> 1.7.4.4

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Hans J. Koch]

On Fri, Aug 05, 2011 at 12:04:13AM +0300, Michael S. Tsirkin wrote:
> On Thu, Aug 04, 2011 at 10:46:06PM +0200, Sebastian Andrzej Siewior wrote:
> > As reported by Anthony in a short way:
> > 
> > |irq 17 handler uio_interrupt+0x0/0x68 enabled interrupts
> > |NIP [c0069d84] handle_irq_event_percpu+0x260/0x26c
> > 
> > The problem here is that spin_unlock_irq() enables the interrupts which
> > is a no-no in interrupt context because they always run with interrupts
> > disabled. This is the case even if IRQF_DISABLED has not been specified
> > since v2.6.35. Therefore this patch uses simple spin_locks().
> > 
> > Looking at it further here is only one spot where the lock is hold. So
> > giving the fact that an ISR is not reentrant and is not executed on two
> > cpus at the same time why do we need a lock here?
> 
> I'm not sure anymore. I think the idea was to use
> it for synchronization down the road somehow,
> but it never materialized. Let's drop that lock completely.

That sounds reasonable.

> 
> > The driver lacks of ->irqcontrol function so I guess the interrupt is
> > enabled via direct PCI-access in userland.
> 
> Through sysfs.

How? With /sys/devices/pci.../enable ?

Thanks,
Hans

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Michael S. Tsirkin]

On Fri, Aug 05, 2011 at 02:15:07AM +0200, Hans J. Koch wrote:
> On Fri, Aug 05, 2011 at 12:04:13AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Aug 04, 2011 at 10:46:06PM +0200, Sebastian Andrzej Siewior wrote:
> > > As reported by Anthony in a short way:
> > > 
> > > |irq 17 handler uio_interrupt+0x0/0x68 enabled interrupts
> > > |NIP [c0069d84] handle_irq_event_percpu+0x260/0x26c
> > > 
> > > The problem here is that spin_unlock_irq() enables the interrupts which
> > > is a no-no in interrupt context because they always run with interrupts
> > > disabled. This is the case even if IRQF_DISABLED has not been specified
> > > since v2.6.35. Therefore this patch uses simple spin_locks().
> > > 
> > > Looking at it further here is only one spot where the lock is hold. So
> > > giving the fact that an ISR is not reentrant and is not executed on two
> > > cpus at the same time why do we need a lock here?
> > 
> > I'm not sure anymore. I think the idea was to use
> > it for synchronization down the road somehow,
> > but it never materialized. Let's drop that lock completely.
> 
> That sounds reasonable.
> 
> > 
> > > The driver lacks of ->irqcontrol function so I guess the interrupt is
> > > enabled via direct PCI-access in userland.
> > 
> > Through sysfs.
> 
> How? With /sys/devices/pci.../enable ?
> 
> Thanks,
> Hans

No. By writing to the command register using
/sys/bus/pci/devices/.../config

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Hans J. Koch]

On Mon, Aug 08, 2011 at 09:24:31AM +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 05, 2011 at 02:15:07AM +0200, Hans J. Koch wrote:
> > On Fri, Aug 05, 2011 at 12:04:13AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Aug 04, 2011 at 10:46:06PM +0200, Sebastian Andrzej Siewior wrote:
> > > > As reported by Anthony in a short way:
> > > > 
> > > > |irq 17 handler uio_interrupt+0x0/0x68 enabled interrupts
> > > > |NIP [c0069d84] handle_irq_event_percpu+0x260/0x26c
> > > > 
> > > > The problem here is that spin_unlock_irq() enables the interrupts which
> > > > is a no-no in interrupt context because they always run with interrupts
> > > > disabled. This is the case even if IRQF_DISABLED has not been specified
> > > > since v2.6.35. Therefore this patch uses simple spin_locks().
> > > > 
> > > > Looking at it further here is only one spot where the lock is hold. So
> > > > giving the fact that an ISR is not reentrant and is not executed on two
> > > > cpus at the same time why do we need a lock here?
> > > 
> > > I'm not sure anymore. I think the idea was to use
> > > it for synchronization down the road somehow,
> > > but it never materialized. Let's drop that lock completely.
> > 
> > That sounds reasonable.

Should I hack up a patch to remove the lock, or do you have anything in your
pipeline?

> > 
> > > 
> > > > The driver lacks of ->irqcontrol function so I guess the interrupt is
> > > > enabled via direct PCI-access in userland.
> > > 
> > > Through sysfs.
> > 
> > How? With /sys/devices/pci.../enable ?
> > 
> > Thanks,
> > Hans
> 
> No. By writing to the command register using
> /sys/bus/pci/devices/.../config

Hope that works at all times...
Anyway, the spin_lock in uio_pci_generic.c definetly doesn't help.

Thanks,
Hans

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Michael S. Tsirkin]

On Mon, Aug 08, 2011 at 07:19:31PM +0200, Hans J. Koch wrote:
> On Mon, Aug 08, 2011 at 09:24:31AM +0300, Michael S. Tsirkin wrote:
> > On Fri, Aug 05, 2011 at 02:15:07AM +0200, Hans J. Koch wrote:
> > > On Fri, Aug 05, 2011 at 12:04:13AM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Aug 04, 2011 at 10:46:06PM +0200, Sebastian Andrzej Siewior wrote:
> > > > > As reported by Anthony in a short way:
> > > > > 
> > > > > |irq 17 handler uio_interrupt+0x0/0x68 enabled interrupts
> > > > > |NIP [c0069d84] handle_irq_event_percpu+0x260/0x26c
> > > > > 
> > > > > The problem here is that spin_unlock_irq() enables the interrupts which
> > > > > is a no-no in interrupt context because they always run with interrupts
> > > > > disabled. This is the case even if IRQF_DISABLED has not been specified
> > > > > since v2.6.35. Therefore this patch uses simple spin_locks().
> > > > > 
> > > > > Looking at it further here is only one spot where the lock is hold. So
> > > > > giving the fact that an ISR is not reentrant and is not executed on two
> > > > > cpus at the same time why do we need a lock here?
> > > > 
> > > > I'm not sure anymore. I think the idea was to use
> > > > it for synchronization down the road somehow,
> > > > but it never materialized. Let's drop that lock completely.
> > > 
> > > That sounds reasonable.
> 
> Should I hack up a patch to remove the lock, or do you have anything in your
> pipeline?


Please do.

> > > 
> > > > 
> > > > > The driver lacks of ->irqcontrol function so I guess the interrupt is
> > > > > enabled via direct PCI-access in userland.
> > > > 
> > > > Through sysfs.
> > > 
> > > How? With /sys/devices/pci.../enable ?
> > > 
> > > Thanks,
> > > Hans
> > 
> > No. By writing to the command register using
> > /sys/bus/pci/devices/.../config
> 
> Hope that works at all times...
> Anyway, the spin_lock in uio_pci_generic.c definetly doesn't help.
> 
> Thanks,
> Hans

True.

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Hans J. Koch]

On Tue, Aug 09, 2011 at 02:37:43PM +0300, Michael S. Tsirkin wrote:
> > 
> > Should I hack up a patch to remove the lock, or do you have anything in your
> > pipeline?
> 
> 
> Please do.
> 

Here it is:

Greg (and anyone else...), you can also pull this from branch "uio-for-greg" from

git://hansjkoch.de/git/linux-hjk


>From ff74627ade002d40fe1f902b7aadab8b1f15f889 Mon Sep 17 00:00:00 2001
From: "Hans J. Koch" <hjk@hansjkoch.de>
Date: Tue, 9 Aug 2011 20:34:31 +0200
Subject: [PATCH] uio: uio_pci_generic: Remove useless spin_lock

The spin_lock in uio_pci_generic.c is only used in the interrupt
handler, which cannot be executed twice at the same time.
That makes the lock rather pointless. This patch removes it.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Chris Wright <chrisw@redhat.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Anthony Foiani <anthony.foiani@gmail.com>
Reported-by: Anthony Foiani <anthony.foiani@gmail.com>
Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
---
 drivers/uio/uio_pci_generic.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index fc22e1e..02bd47b 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -24,7 +24,6 @@
 #include <linux/pci.h>
 #include <linux/slab.h>
 #include <linux/uio_driver.h>
-#include <linux/spinlock.h>
 
 #define DRIVER_VERSION	"0.01.0"
 #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
@@ -33,7 +32,6 @@
 struct uio_pci_generic_dev {
 	struct uio_info info;
 	struct pci_dev *pdev;
-	spinlock_t lock; /* guards command register accesses */
 };
 
 static inline struct uio_pci_generic_dev *
@@ -57,7 +55,6 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 	BUILD_BUG_ON(PCI_COMMAND % 4);
 	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
 
-	spin_lock_irq(&gdev->lock);
 	pci_block_user_cfg_access(pdev);
 
 	/* Read both command and status registers in a single 32-bit operation.
@@ -83,7 +80,6 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 done:
 
 	pci_unblock_user_cfg_access(pdev);
-	spin_unlock_irq(&gdev->lock);
 	return ret;
 }
 
@@ -158,7 +154,6 @@ static int __devinit probe(struct pci_dev *pdev,
 	gdev->info.irq_flags = IRQF_SHARED;
 	gdev->info.handler = irqhandler;
 	gdev->pdev = pdev;
-	spin_lock_init(&gdev->lock);
 
 	if (uio_register_device(&pdev->dev, &gdev->info))
 		goto err_register;
-- 
1.7.5.4

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Michael S. Tsirkin]

On Tue, Aug 09, 2011 at 08:53:41PM +0200, Hans J. Koch wrote:
> On Tue, Aug 09, 2011 at 02:37:43PM +0300, Michael S. Tsirkin wrote:
> > > 
> > > Should I hack up a patch to remove the lock, or do you have anything in your
> > > pipeline?
> > 
> > 
> > Please do.
> > 
> 
> Here it is:
> 
> Greg (and anyone else...), you can also pull this from branch "uio-for-greg" from
> 
> git://hansjkoch.de/git/linux-hjk
> 
> 
> >From ff74627ade002d40fe1f902b7aadab8b1f15f889 Mon Sep 17 00:00:00 2001
> From: "Hans J. Koch" <hjk@hansjkoch.de>
> Date: Tue, 9 Aug 2011 20:34:31 +0200
> Subject: [PATCH] uio: uio_pci_generic: Remove useless spin_lock
> 
> The spin_lock in uio_pci_generic.c is only used in the interrupt
> handler, which cannot be executed twice at the same time.
> That makes the lock rather pointless. This patch removes it.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Chris Wright <chrisw@redhat.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Anthony Foiani <anthony.foiani@gmail.com>
> Reported-by: Anthony Foiani <anthony.foiani@gmail.com>
> Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>


Acked-by: Michael S. Tsirkin <mst@redhat.com>

I also suggest this for the stable trees.

> ---
>  drivers/uio/uio_pci_generic.c |    5 -----
>  1 files changed, 0 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
> index fc22e1e..02bd47b 100644
> --- a/drivers/uio/uio_pci_generic.c
> +++ b/drivers/uio/uio_pci_generic.c
> @@ -24,7 +24,6 @@
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <linux/uio_driver.h>
> -#include <linux/spinlock.h>
>  
>  #define DRIVER_VERSION	"0.01.0"
>  #define DRIVER_AUTHOR	"Michael S. Tsirkin <mst@redhat.com>"
> @@ -33,7 +32,6 @@
>  struct uio_pci_generic_dev {
>  	struct uio_info info;
>  	struct pci_dev *pdev;
> -	spinlock_t lock; /* guards command register accesses */
>  };
>  
>  static inline struct uio_pci_generic_dev *
> @@ -57,7 +55,6 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
>  	BUILD_BUG_ON(PCI_COMMAND % 4);
>  	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
>  
> -	spin_lock_irq(&gdev->lock);
>  	pci_block_user_cfg_access(pdev);
>  
>  	/* Read both command and status registers in a single 32-bit operation.
> @@ -83,7 +80,6 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
>  done:
>  
>  	pci_unblock_user_cfg_access(pdev);
> -	spin_unlock_irq(&gdev->lock);
>  	return ret;
>  }
>  
> @@ -158,7 +154,6 @@ static int __devinit probe(struct pci_dev *pdev,
>  	gdev->info.irq_flags = IRQF_SHARED;
>  	gdev->info.handler = irqhandler;
>  	gdev->pdev = pdev;
> -	spin_lock_init(&gdev->lock);
>  
>  	if (uio_register_device(&pdev->dev, &gdev->info))
>  		goto err_register;
> -- 
> 1.7.5.4

Re: [stable] [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Greg KH]

On Wed, Aug 10, 2011 at 11:40:12AM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 09, 2011 at 08:53:41PM +0200, Hans J. Koch wrote:
> > On Tue, Aug 09, 2011 at 02:37:43PM +0300, Michael S. Tsirkin wrote:
> > > > 
> > > > Should I hack up a patch to remove the lock, or do you have anything in your
> > > > pipeline?
> > > 
> > > 
> > > Please do.
> > > 
> > 
> > Here it is:
> > 
> > Greg (and anyone else...), you can also pull this from branch "uio-for-greg" from
> > 
> > git://hansjkoch.de/git/linux-hjk
> > 
> > 
> > >From ff74627ade002d40fe1f902b7aadab8b1f15f889 Mon Sep 17 00:00:00 2001
> > From: "Hans J. Koch" <hjk@hansjkoch.de>
> > Date: Tue, 9 Aug 2011 20:34:31 +0200
> > Subject: [PATCH] uio: uio_pci_generic: Remove useless spin_lock
> > 
> > The spin_lock in uio_pci_generic.c is only used in the interrupt
> > handler, which cannot be executed twice at the same time.
> > That makes the lock rather pointless. This patch removes it.
> > 
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Chris Wright <chrisw@redhat.com>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Cc: Anthony Foiani <anthony.foiani@gmail.com>
> > Reported-by: Anthony Foiani <anthony.foiani@gmail.com>
> > Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
> 
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> I also suggest this for the stable trees.

Why, it's not fixing a bug that anyone hits, right?

greg k-h

Re: [stable] [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Sebastian Andrzej Siewior]

* Greg KH | 2011-08-22 17:49:43 [-0700]:

>> > Cc: Anthony Foiani <anthony.foiani@gmail.com>
>
>Why, it's not fixing a bug that anyone hits, right?

"earlier" the interrupt handler was executed either with interrupts
enabled or disabled if IRQF_DISABLED was specified. Later the latter
become default even if IRQF_DISABLED was not specified. This lead to the
splat Anthony reported because the irq handler was entered with IRQs
disabled and the ISR enabled them via spin_unlock_irq(). My initial
patch simply used spin_unlock_irqrestore() (and its counterpart) to have
the same state as we had.

So the bug Anthony hit was that the interrupts were enabled where they
should not be.

>greg k-h

Sebastian

Re: [stable] [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Hans J. Koch]

On Tue, Aug 23, 2011 at 10:40:05AM +0200, Sebastian Andrzej Siewior wrote:
> * Greg KH | 2011-08-22 17:49:43 [-0700]:
> 
> >> > Cc: Anthony Foiani <anthony.foiani@gmail.com>
> >
> >Why, it's not fixing a bug that anyone hits, right?
> 
> "earlier" the interrupt handler was executed either with interrupts
> enabled or disabled if IRQF_DISABLED was specified. Later the latter
> become default even if IRQF_DISABLED was not specified. This lead to the
> splat Anthony reported because the irq handler was entered with IRQs
> disabled and the ISR enabled them via spin_unlock_irq(). My initial
> patch simply used spin_unlock_irqrestore() (and its counterpart) to have
> the same state as we had.
> 
> So the bug Anthony hit was that the interrupts were enabled where they
> should not be.
> 
> >greg k-h
> 
> Sebastian
> 

Greg, I'll change the commit message so that it becomes clear, and send
you a pull request.

There is also one more UIO patch pending. Do you want a separate branch
for -stable, or can you cherry-pick that from one UIO branch?

Thanks,
Hans

Re: [stable] [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Hans J. Koch]

On Tue, Aug 23, 2011 at 12:57:42PM +0200, Hans J. Koch wrote:
> On Tue, Aug 23, 2011 at 10:40:05AM +0200, Sebastian Andrzej Siewior wrote:
> > * Greg KH | 2011-08-22 17:49:43 [-0700]:
> > 
> > >> > Cc: Anthony Foiani <anthony.foiani@gmail.com>
> > >
> > >Why, it's not fixing a bug that anyone hits, right?
> > 
> > "earlier" the interrupt handler was executed either with interrupts
> > enabled or disabled if IRQF_DISABLED was specified. Later the latter
> > become default even if IRQF_DISABLED was not specified. This lead to the
> > splat Anthony reported because the irq handler was entered with IRQs
> > disabled and the ISR enabled them via spin_unlock_irq(). My initial
> > patch simply used spin_unlock_irqrestore() (and its counterpart) to have
> > the same state as we had.
> > 
> > So the bug Anthony hit was that the interrupts were enabled where they
> > should not be.
> > 
> > >greg k-h
> > 
> > Sebastian
> > 
> 
> Greg, I'll change the commit message so that it becomes clear, and send
> you a pull request.
> 
> There is also one more UIO patch pending. Do you want a separate branch
> for -stable, or can you cherry-pick that from one UIO branch?

Forget it, I just saw you already added it to your tree.

Thanks,
Hans

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Sebastian Andrzej Siewior]

* Michael S. Tsirkin | 2011-08-05 00:04:13 [+0300]:

>> Looking at it further here is only one spot where the lock is hold. So
>> giving the fact that an ISR is not reentrant and is not executed on two
>> cpus at the same time why do we need a lock here?
>
>I'm not sure anymore. I think the idea was to use
>it for synchronization down the road somehow,
>but it never materialized. Let's drop that lock completely.
Okay. So I post antoher patch with this lock removed and cc stable.

>> So there is _no_ protection
>> against read-modify-write of user vs kernel so even that
>> pci_block_user_cfg_access() is kinda pointless.
>
>I didn't get that. pci_block_user_cfg_access is to prevent
>sysfs access while we read modify-write the command register.
>Isn't it effective for that?
It probably works well enough for you because you only care the one bit
and don't change anything else in the kernel driver.

Lets assume user land changes another bit in this register:

    user                                kernel
 read config()                             |
   add a bit                               |
     |                                  interrupt
     |                                block user land
     |                              read + clear + write
     |                               unblock user land
 write config back                         | 


You did not *re-read* the config field after the interrupt so kernel's
modifications are lost. So you get two interrupts accounted while only
one happend. It seems to me that you could drop this "user block" thing
since you never change anything outside of this command register and it
does not stop the race.

>> pci_block_user_cfg_access() in open() + ->irqcontrol() should fix this.
>
>Why block in open? We don't access the device there, do we?
Yeah. That might not work for you since you need change other values.

Sebastian

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Michael S. Tsirkin]

On Fri, Aug 05, 2011 at 09:18:42PM +0200, Sebastian Andrzej Siewior wrote:
> * Michael S. Tsirkin | 2011-08-05 00:04:13 [+0300]:
> 
> >> Looking at it further here is only one spot where the lock is hold. So
> >> giving the fact that an ISR is not reentrant and is not executed on two
> >> cpus at the same time why do we need a lock here?
> >
> >I'm not sure anymore. I think the idea was to use
> >it for synchronization down the road somehow,
> >but it never materialized. Let's drop that lock completely.
> Okay. So I post antoher patch with this lock removed and cc stable.
> 
> >> So there is _no_ protection
> >> against read-modify-write of user vs kernel so even that
> >> pci_block_user_cfg_access() is kinda pointless.
> >
> >I didn't get that. pci_block_user_cfg_access is to prevent
> >sysfs access while we read modify-write the command register.
> >Isn't it effective for that?
> It probably works well enough for you because you only care the one bit
> and don't change anything else in the kernel driver.
> 
> Lets assume user land changes another bit in this register:
> 
>     user                                kernel
>  read config()                             |
>    add a bit                               |
>      |                                  interrupt
>      |                                block user land
>      |                              read + clear + write
>      |                               unblock user land
>  write config back                         | 
> 
> 
> You did not *re-read* the config field after the interrupt so kernel's
> modifications are lost. So you get two interrupts accounted while only
> one happend.

So we might get an extra interrupt. This is harmless,
and the window seems small enough for this not to affect
performance. This will also never happen if
userspace is careful to make interrupt unmasking its last action.

> It seems to me that you could drop this "user block" thing
> since you never change anything outside of this command register and it
> does not stop the race.

I don't think so: if we did, we would lose userspace modifications to
other bits such as io enable, and there's no way to guess what their
values should be.

> >> pci_block_user_cfg_access() in open() + ->irqcontrol() should fix this.
> >
> >Why block in open? We don't access the device there, do we?
> Yeah. That might not work for you since you need change other values.
> 
> Sebastian

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Sebastian Andrzej Siewior]

Michael S. Tsirkin wrote:
>> It seems to me that you could drop this "user block" thing
>> since you never change anything outside of this command register and it
>> does not stop the race.
> 
> I don't think so: if we did, we would lose userspace modifications to
> other bits such as io enable, and there's no way to guess what their
> values should be.

How so?

Sebastian

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Michael S. Tsirkin]

On Tue, Aug 09, 2011 at 01:43:46PM +0200, Sebastian Andrzej Siewior wrote:
> Michael S. Tsirkin wrote:
> >>It seems to me that you could drop this "user block" thing
> >>since you never change anything outside of this command register and it
> >>does not stop the race.
> >
> >I don't think so: if we did, we would lose userspace modifications to
> >other bits such as io enable, and there's no way to guess what their
> >values should be.
> 
> How so?
> 
> Sebastian

Let's assume we start with e.g. io enable bit cleared.


    user                                kernel
 read config                               |
 set io enable                             |
     |                                  interrupt                                                
     |                              read + set interrupt mask
 write config back                         |
					write



We end up with io enable bit cleared. Locking around rmw
that we have fixes this race.

-- 
MST

Re: [PATCH] uio/gen-pci: don't enable interrupts in ISR

[Sebastian Andrzej Siewior]

Michael S. Tsirkin wrote:
> Let's assume we start with e.g. io enable bit cleared.
> 
> 
>     user                                kernel
>  read config                               |
>  set io enable                             |
>      |                                  interrupt                                                
>      |                              read + set interrupt mask
>  write config back                         |
> 					write
> 
> 
> 
> We end up with io enable bit cleared. Locking around rmw
> that we have fixes this race.

Okay, forgot about the SMP case, sorry.

Sebastian