[PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

Add a spin_unlock missing on the error path.  The locks and unlocks are
balanced in other functions, so it seems that the same should be the case
here.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression E1;
@@

* spin_lock(E1,...);
  <+... when != E1
  if (...) {
    ... when != E1
*   return ...;
  }
  ...+>
* spin_unlock(E1,...);
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/x86/kernel/amd_iommu.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index fa5a147..b98e1cd 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
 
 	/* Some sanity checks */
 	if (alias_data->domain != NULL &&
-	    alias_data->domain != domain)
+	    alias_data->domain != domain) {
+		spin_unlock(&domain->lock);
 		return -EBUSY;
+	}
 
 	if (dev_data->domain != NULL &&
-	    dev_data->domain != domain)
+	    dev_data->domain != domain) {
+		spin_unlock(&domain->lock);
 		return -EBUSY;
+	}
 
 	/* Do real assignment */
 	if (dev_data->alias != dev) {

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Roedel, Joerg]

On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
> 
> Add a spin_unlock missing on the error path.  The locks and unlocks are
> balanced in other functions, so it seems that the same should be the case
> here.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)

Applied the attached version of the patch. Thanks for catching this.

	Joerg

>From 84fe6c19e4a598e8071e3bd1b2c923454eae1268 Mon Sep 17 00:00:00 2001
From: Julia Lawall <julia@diku.dk>
Date: Thu, 27 May 2010 12:31:51 +0200
Subject: [PATCH] arch/x86/kernel: Add missing spin_unlock

Add a spin_unlock missing on the error path.  The locks and unlocks are
balanced in other functions, so it seems that the same should be the case
here.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression E1;
@@

* spin_lock(E1,...);
  <+... when != E1
  if (...) {
    ... when != E1
*   return ...;
  }
  ...+>
* spin_unlock(E1,...);
// </smpl>

Cc: stable@kernel.org
Signed-off-by: Julia Lawall <julia@diku.dk>
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/amd_iommu.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
index fa5a147..8a9aaa8 100644
--- a/arch/x86/kernel/amd_iommu.c
+++ b/arch/x86/kernel/amd_iommu.c
@@ -1487,6 +1487,7 @@ static int __attach_device(struct device *dev,
 			   struct protection_domain *domain)
 {
 	struct iommu_dev_data *dev_data, *alias_data;
+	int ret;
 
 	dev_data   = get_dev_data(dev);
 	alias_data = get_dev_data(dev_data->alias);
@@ -1498,13 +1499,14 @@ static int __attach_device(struct device *dev,
 	spin_lock(&domain->lock);
 
 	/* Some sanity checks */
+	ret = -EBUSY;
 	if (alias_data->domain != NULL &&
 	    alias_data->domain != domain)
-		return -EBUSY;
+		goto out_unlock;
 
 	if (dev_data->domain != NULL &&
 	    dev_data->domain != domain)
-		return -EBUSY;
+		goto out_unlock;
 
 	/* Do real assignment */
 	if (dev_data->alias != dev) {
@@ -1520,10 +1522,14 @@ static int __attach_device(struct device *dev,
 
 	atomic_inc(&dev_data->bind);
 
+	ret = 0;
+
+out_unlock:
+
 	/* ready */
 	spin_unlock(&domain->lock);
 
-	return 0;
+	return ret;
 }
 
 /*
-- 
1.7.1

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Roedel, Joerg]

On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression E1;
> @@
> 
> * spin_lock(E1,...);
>   <+... when != E1
>   if (...) {
>     ... when != E1
> *   return ...;
>   }
>   ...+>
> * spin_unlock(E1,...);
> // </smpl>

Btw, I think it would be great to have a collection of these semantic
match scripts in the kernel repository together with a build target to
run these scripts over the kernel sources (like the cscope target).
Opinions?

	Joerg

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Julia Lawall]

On Thu, 27 May 2010, Roedel, Joerg wrote:

> On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression E1;
> > @@
> > 
> > * spin_lock(E1,...);
> >   <+... when != E1
> >   if (...) {
> >     ... when != E1
> > *   return ...;
> >   }
> >   ...+>
> > * spin_unlock(E1,...);
> > // </smpl>
> 
> Btw, I think it would be great to have a collection of these semantic
> match scripts in the kernel repository together with a build target to
> run these scripts over the kernel sources (like the cscope target).
> Opinions?

We have submitted and received some feedback on an initial version of 
this, but I'm not completely sure of the current status.

julia

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Nicolas Palix]

On Thursday 27 May 2010 13:17:58 Julia Lawall wrote:
> On Thu, 27 May 2010, Roedel, Joerg wrote:
> 
> > On Wed, May 26, 2010 at 11:55:59AM -0400, Julia Lawall wrote:
> > > The semantic match that finds this problem is as follows:
> > > (http://coccinelle.lip6.fr/)
> > > 
> > > // <smpl>
> > > @@
> > > expression E1;
> > > @@
> > > 
> > > * spin_lock(E1,...);
> > >   <+... when != E1
> > >   if (...) {
> > >     ... when != E1
> > > *   return ...;
> > >   }
> > >   ...+>
> > > * spin_unlock(E1,...);
> > > // </smpl>
> > 
> > Btw, I think it would be great to have a collection of these semantic
> > match scripts in the kernel repository together with a build target to
> > run these scripts over the kernel sources (like the cscope target).
> > Opinions?
> 
> We have submitted and received some feedback on an initial version of 
> this, but I'm not completely sure of the current status.

You can see the latest feedback we get at
http://lkml.org/lkml/2010/5/10/257

The initial submission and its comments are at
http://lkml.org/lkml/2010/4/26/269

> 
> julia
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Nicolas Palix
Tel: (+33) 1 44 27 87 25
Tel: (+33) 6 81 07 91 72
Web: http://www.diku.dk/~npalix/

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Roedel, Joerg]

On Thu, May 27, 2010 at 07:42:25AM -0400, Nicolas Palix wrote:
> > We have submitted and received some feedback on an initial version of 
> > this, but I'm not completely sure of the current status.
> 
> You can see the latest feedback we get at
> http://lkml.org/lkml/2010/5/10/257
> 
> The initial submission and its comments are at
> http://lkml.org/lkml/2010/4/26/269

I've also sent some feedback. Would be cool if you could work the
feedback in and do a repost asking Andrew to take it. Would be cool to
have this merged with 2.6.36.

	Joerg

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[H. Peter Anvin]

On 05/28/2010 12:11 AM, Roedel, Joerg wrote:
> On Thu, May 27, 2010 at 07:42:25AM -0400, Nicolas Palix wrote:
>>> We have submitted and received some feedback on an initial version of 
>>> this, but I'm not completely sure of the current status.
>>
>> You can see the latest feedback we get at
>> http://lkml.org/lkml/2010/5/10/257
>>
>> The initial submission and its comments are at
>> http://lkml.org/lkml/2010/4/26/269
> 
> I've also sent some feedback. Would be cool if you could work the
> feedback in and do a repost asking Andrew to take it. Would be cool to
> have this merged with 2.6.36.
> 

I don't see why scripts that don't *in themselves* change the output
binaries need to wait for .36.  Instead, it would be better to get them
in sooner to make them available to developers in advance of the .36 cycle.

Of course, I'm not Linus, and I don't see him Cc:'d on this, but that
would be the normal rules.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Joerg Roedel]

On Fri, May 28, 2010 at 09:45:16AM -0700, H. Peter Anvin wrote:
> On 05/28/2010 12:11 AM, Roedel, Joerg wrote:
> > On Thu, May 27, 2010 at 07:42:25AM -0400, Nicolas Palix wrote:
> >>> We have submitted and received some feedback on an initial version of 
> >>> this, but I'm not completely sure of the current status.
> >>
> >> You can see the latest feedback we get at
> >> http://lkml.org/lkml/2010/5/10/257
> >>
> >> The initial submission and its comments are at
> >> http://lkml.org/lkml/2010/4/26/269
> > 
> > I've also sent some feedback. Would be cool if you could work the
> > feedback in and do a repost asking Andrew to take it. Would be cool to
> > have this merged with 2.6.36.
> > 
> 
> I don't see why scripts that don't *in themselves* change the output
> binaries need to wait for .36.  Instead, it would be better to get them
> in sooner to make them available to developers in advance of the .36 cycle.
> 
> Of course, I'm not Linus, and I don't see him Cc:'d on this, but that
> would be the normal rules.

Yeah, merging it now would be even better. Since these scripts don't
change the kernel itself there is little risk to break anything. So how
about working in the feedback and sending an updated version to Linus
soon?

	Joerg

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Andrew Morton]

On Wed, 26 May 2010 17:55:59 +0200 (CEST)
Julia Lawall <julia@diku.dk> wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> Add a spin_unlock missing on the error path.  The locks and unlocks are
> balanced in other functions, so it seems that the same should be the case
> here.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression E1;
> @@
> 
> * spin_lock(E1,...);
>   <+... when != E1
>   if (...) {
>     ... when != E1
> *   return ...;
>   }
>   ...+>
> * spin_unlock(E1,...);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  arch/x86/kernel/amd_iommu.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> index fa5a147..b98e1cd 100644
> --- a/arch/x86/kernel/amd_iommu.c
> +++ b/arch/x86/kernel/amd_iommu.c
> @@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
>  
>  	/* Some sanity checks */
>  	if (alias_data->domain != NULL &&
> -	    alias_data->domain != domain)
> +	    alias_data->domain != domain) {
> +		spin_unlock(&domain->lock);
>  		return -EBUSY;
> +	}
>  
>  	if (dev_data->domain != NULL &&
> -	    dev_data->domain != domain)
> +	    dev_data->domain != domain) {
> +		spin_unlock(&domain->lock);
>  		return -EBUSY;
> +	}
>  
>  	/* Do real assignment */
>  	if (dev_data->alias != dev) {

The reason why these bugs occur is that we sprinkle multiple `return'
statements inside the middle of non-trivial functions.  People miss
some or fail to modify some when later changing locking rules and we
gain bugs (or, similarly, resource leaks).

So I'd suggest that when fixing such bugs, we also fix their cause.

ie:

--- a/arch/x86/kernel/amd_iommu.c~arch-x86-kernel-add-missing-spin_unlock
+++ a/arch/x86/kernel/amd_iommu.c
@@ -1487,6 +1487,7 @@ static int __attach_device(struct device
 			   struct protection_domain *domain)
 {
 	struct iommu_dev_data *dev_data, *alias_data;
+	int ret;
 
 	dev_data   = get_dev_data(dev);
 	alias_data = get_dev_data(dev_data->alias);
@@ -1497,14 +1498,17 @@ static int __attach_device(struct device
 	/* lock domain */
 	spin_lock(&domain->lock);
 
+	ret = -EBUSY;
 	/* Some sanity checks */
 	if (alias_data->domain != NULL &&
 	    alias_data->domain != domain)
-		return -EBUSY;
+		goto out;
 
 	if (dev_data->domain != NULL &&
 	    dev_data->domain != domain)
-		return -EBUSY;
+		goto out;
+
+	ret = 0;
 
 	/* Do real assignment */
 	if (dev_data->alias != dev) {
@@ -1522,8 +1526,8 @@ static int __attach_device(struct device
 
 	/* ready */
 	spin_unlock(&domain->lock);
-
-	return 0;
+out:
+	return ret;
 }
 
 /*
_

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Thomas Gleixner]

On Tue, 1 Jun 2010, Andrew Morton wrote:

> On Wed, 26 May 2010 17:55:59 +0200 (CEST)
> Julia Lawall <julia@diku.dk> wrote:
> 
> > From: Julia Lawall <julia@diku.dk>
> > 
> > Add a spin_unlock missing on the error path.  The locks and unlocks are
> > balanced in other functions, so it seems that the same should be the case
> > here.
> > 
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression E1;
> > @@
> > 
> > * spin_lock(E1,...);
> >   <+... when != E1
> >   if (...) {
> >     ... when != E1
> > *   return ...;
> >   }
> >   ...+>
> > * spin_unlock(E1,...);
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  arch/x86/kernel/amd_iommu.c |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> > index fa5a147..b98e1cd 100644
> > --- a/arch/x86/kernel/amd_iommu.c
> > +++ b/arch/x86/kernel/amd_iommu.c
> > @@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
> >  
> >  	/* Some sanity checks */
> >  	if (alias_data->domain != NULL &&
> > -	    alias_data->domain != domain)
> > +	    alias_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	if (dev_data->domain != NULL &&
> > -	    dev_data->domain != domain)
> > +	    dev_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	/* Do real assignment */
> >  	if (dev_data->alias != dev) {
> 
> The reason why these bugs occur is that we sprinkle multiple `return'
> statements inside the middle of non-trivial functions.  People miss
> some or fail to modify some when later changing locking rules and we
> gain bugs (or, similarly, resource leaks).
> 
> So I'd suggest that when fixing such bugs, we also fix their cause.
> 
> ie:
> 
> --- a/arch/x86/kernel/amd_iommu.c~arch-x86-kernel-add-missing-spin_unlock
> +++ a/arch/x86/kernel/amd_iommu.c
> @@ -1487,6 +1487,7 @@ static int __attach_device(struct device
>  			   struct protection_domain *domain)
>  {
>  	struct iommu_dev_data *dev_data, *alias_data;
> +	int ret;
>  
>  	dev_data   = get_dev_data(dev);
>  	alias_data = get_dev_data(dev_data->alias);
> @@ -1497,14 +1498,17 @@ static int __attach_device(struct device
>  	/* lock domain */
>  	spin_lock(&domain->lock);
>  
> +	ret = -EBUSY;
>  	/* Some sanity checks */
>  	if (alias_data->domain != NULL &&
>  	    alias_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
>  
>  	if (dev_data->domain != NULL &&
>  	    dev_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
> +
> +	ret = 0;
>  
>  	/* Do real assignment */
>  	if (dev_data->alias != dev) {
> @@ -1522,8 +1526,8 @@ static int __attach_device(struct device
>  
>  	/* ready */
>  	spin_unlock(&domain->lock);
> -
> -	return 0;
> +out:

Moving the label _before_ spin_unlock() might fix it really. :)

> +	return ret;
>  }

Thanks,

	tglx

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Julia Lawall]

On Tue, 1 Jun 2010, Andrew Morton wrote:

> On Wed, 26 May 2010 17:55:59 +0200 (CEST)
> Julia Lawall <julia@diku.dk> wrote:
> 
> > From: Julia Lawall <julia@diku.dk>
> > 
> > Add a spin_unlock missing on the error path.  The locks and unlocks are
> > balanced in other functions, so it seems that the same should be the case
> > here.
> > 
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @@
> > expression E1;
> > @@
> > 
> > * spin_lock(E1,...);
> >   <+... when != E1
> >   if (...) {
> >     ... when != E1
> > *   return ...;
> >   }
> >   ...+>
> > * spin_unlock(E1,...);
> > // </smpl>
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  arch/x86/kernel/amd_iommu.c |    8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/amd_iommu.c b/arch/x86/kernel/amd_iommu.c
> > index fa5a147..b98e1cd 100644
> > --- a/arch/x86/kernel/amd_iommu.c
> > +++ b/arch/x86/kernel/amd_iommu.c
> > @@ -1499,12 +1499,16 @@ static int __attach_device(struct device *dev,
> >  
> >  	/* Some sanity checks */
> >  	if (alias_data->domain != NULL &&
> > -	    alias_data->domain != domain)
> > +	    alias_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	if (dev_data->domain != NULL &&
> > -	    dev_data->domain != domain)
> > +	    dev_data->domain != domain) {
> > +		spin_unlock(&domain->lock);
> >  		return -EBUSY;
> > +	}
> >  
> >  	/* Do real assignment */
> >  	if (dev_data->alias != dev) {
> 
> The reason why these bugs occur is that we sprinkle multiple `return'
> statements inside the middle of non-trivial functions.  People miss
> some or fail to modify some when later changing locking rules and we
> gain bugs (or, similarly, resource leaks).
> 
> So I'd suggest that when fixing such bugs, we also fix their cause.
> 
> ie:
> 
> --- a/arch/x86/kernel/amd_iommu.c~arch-x86-kernel-add-missing-spin_unlock
> +++ a/arch/x86/kernel/amd_iommu.c
> @@ -1487,6 +1487,7 @@ static int __attach_device(struct device
>  			   struct protection_domain *domain)
>  {
>  	struct iommu_dev_data *dev_data, *alias_data;
> +	int ret;
>  
>  	dev_data   = get_dev_data(dev);
>  	alias_data = get_dev_data(dev_data->alias);
> @@ -1497,14 +1498,17 @@ static int __attach_device(struct device
>  	/* lock domain */
>  	spin_lock(&domain->lock);
>  
> +	ret = -EBUSY;
>  	/* Some sanity checks */
>  	if (alias_data->domain != NULL &&
>  	    alias_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
>  
>  	if (dev_data->domain != NULL &&
>  	    dev_data->domain != domain)
> -		return -EBUSY;
> +		goto out;
> +
> +	ret = 0;
>  
>  	/* Do real assignment */
>  	if (dev_data->alias != dev) {
> @@ -1522,8 +1526,8 @@ static int __attach_device(struct device
>  
>  	/* ready */
>  	spin_unlock(&domain->lock);
> -
> -	return 0;
> +out:
> +	return ret;
>  }

I don't have the impression that this actually fixes the problem, only the 
code structure.  Out should be above the spin_lock, and there should just 
be one return, of ret.  I will send another patch shortly.

julia

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Roedel, Joerg]

Hi Andrew,

On Tue, Jun 01, 2010 at 05:15:29PM -0400, Andrew Morton wrote:
> The reason why these bugs occur is that we sprinkle multiple `return'
> statements inside the middle of non-trivial functions.  People miss
> some or fail to modify some when later changing locking rules and we
> gain bugs (or, similarly, resource leaks).

Right. I changed that in Julia's patch too before merging it into my
tree. It is already in -tip. See
http://git.kernel.org/tip/84fe6c19e4a598e8071e3bd1b2c923454eae1268

	Joerg

Re: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock

[Julia Lawall]

On Wed, 2 Jun 2010, Roedel, Joerg wrote:

> Hi Andrew,
> 
> On Tue, Jun 01, 2010 at 05:15:29PM -0400, Andrew Morton wrote:
> > The reason why these bugs occur is that we sprinkle multiple `return'
> > statements inside the middle of non-trivial functions.  People miss
> > some or fail to modify some when later changing locking rules and we
> > gain bugs (or, similarly, resource leaks).
> 
> Right. I changed that in Julia's patch too before merging it into my
> tree. It is already in -tip. See
> http://git.kernel.org/tip/84fe6c19e4a598e8071e3bd1b2c923454eae1268

Thanks.  That looks fine.

julia