lkml.org 
[lkml]   [2010]   [Jun]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 6/17] arch/x86/kernel: Add missing spin_unlock
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


\
 
 \ /
  Last update: 2010-06-01 23:51    [W:0.097 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site