Prev: Fix SVM longmode guests
Next: [PATCH] i2c-imx: do not allow interruptions when waiting for I2C to complete
From: H. Peter Anvin on 28 May 2010 12:50
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Joerg Roedel on 1 Jun 2010 06:00
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 1 Jun 2010 17:20
On Wed, 26 May 2010 17:55:59 +0200 (CEST)
Julia Lawall <julia(a)diku.dk> wrote:

> From: Julia Lawall <julia(a)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(a)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;
}

/*
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on 1 Jun 2010 17:50
On Tue, 1 Jun 2010, Andrew Morton wrote:

> On Wed, 26 May 2010 17:55:59 +0200 (CEST)
> Julia Lawall <julia(a)diku.dk> wrote:
>
> > From: Julia Lawall <julia(a)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(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Julia Lawall on 2 Jun 2010 01:30
On Tue, 1 Jun 2010, Andrew Morton wrote:

> On Wed, 26 May 2010 17:55:59 +0200 (CEST)
> Julia Lawall <julia(a)diku.dk> wrote:
>
> > From: Julia Lawall <julia(a)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(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3
Prev: Fix SVM longmode guests
Next: [PATCH] i2c-imx: do not allow interruptions when waiting for I2C to complete