2002-02-26 19:39:53

by petter wahlman

Subject: [PATCH] 2.4.18 Eicon ISDN driver fix.

The following code is calling a possibly blocking operation while
holding a spinlock.


Petter Wahlman


--- linux-2.4.18/drivers/isdn/eicon/eicon_mod.c Fri Dec 21 18:41:54 2001
+++ linux-2.4.18-pw/drivers/isdn/eicon/eicon_mod.c Mon Feb 25
23:45:05 2002
@@ -665,8 +665,11 @@
else
cnt = skb->len;

- if (user)
+ if (user) {
+ spin_unlock_irqrestore(&eicon_lock,
flags);
copy_to_user(p, skb->data, cnt);
+ spin_lock_irqsave(&eicon_lock, flags);
+ }
else
memcpy(p, skb->data, cnt);

2002-02-26 19:55:14

by Dave Jones

Subject: Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

On Tue, Feb 26, 2002 at 08:26:18PM +0100, petter wahlman wrote:
> +++ linux-2.4.18-pw/drivers/isdn/eicon/eicon_mod.c Mon Feb 25

> - if (user)
> + if (user) {
> + spin_unlock_irqrestore(&eicon_lock,
> flags);
> copy_to_user(p, skb->data, cnt);
> + spin_lock_irqsave(&eicon_lock, flags);
> + }

What happens if something else adds/removes to card->statq, or
frees the skb after you drop the lock? I'm not familiar with
this code, but from a quick look, it looks like this introduces
a race no ?

--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs

2002-02-26 20:02:54

by petter wahlman

Subject: Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

On Tue, 2002-02-26 at 20:54, Dave Jones wrote:
> On Tue, Feb 26, 2002 at 08:26:18PM +0100, petter wahlman wrote:
> > +++ linux-2.4.18-pw/drivers/isdn/eicon/eicon_mod.c Mon Feb 25
>
> > - if (user)
> > + if (user) {
> > + spin_unlock_irqrestore(&eicon_lock,
> > flags);
> > copy_to_user(p, skb->data, cnt);
> > + spin_lock_irqsave(&eicon_lock, flags);
> > + }
>
> What happens if something else adds/removes to card->statq, or
> frees the skb after you drop the lock? I'm not familiar with
> this code, but from a quick look, it looks like this introduces
> a race no ?
>

Yes, it will introduce a new race.
I did not actually intend to send this unfinished patch, but fscked up
:)
Anyway, calling copy_to_user while holding a spinlock is defiantly a bad
idea.


> --
> | Dave Jones. http://www.codemonkey.org.uk
> | SuSE Labs
>

Petter Wahlman.

2002-02-26 20:36:05

by Alan

Subject: Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

> The following code is calling a possibly blocking operation while
> holding a spinlock.

Definitely a bug - but can you prove dropping the lock there is safe ?

2002-02-27 07:58:52

by Armin Schindler

Subject: Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

The patch below fixes the race condition with copy_to_user and will
not introduce a new race. What can happen is that two reader-processes
may get mixed-up messages, but more than one reader isn't allowed here
anyway.

Please apply this patch to 2.4 and 2.2, it works for both.

Thanx,
Armin

On 26 Feb 2002, petter wahlman wrote:
> The following code is calling a possibly blocking operation while
> holding a spinlock.
>
>
> Petter Wahlman


--- linux-2.4.18/drivers/isdn/eicon/eicon_mod.c Fri Dec 21 18:41:54 2001
+++ linux-2.4.18-pw/drivers/isdn/eicon/eicon_mod.c Mon Feb 25
23:45:05 2002
@@ -665,8 +665,11 @@
else
cnt = skb->len;

- if (user)
+ if (user) {
+ spin_unlock_irqrestore(&eicon_lock,
flags);
copy_to_user(p, skb->data, cnt);
+ spin_lock_irqsave(&eicon_lock, flags);
+ }
else
memcpy(p, skb->data, cnt);

2002-02-28 18:55:46

by Marcelo Tosatti

Subject: Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

On Wed, 27 Feb 2002, Armin Schindler wrote:

> The patch below fixes the race condition with copy_to_user and will
> not introduce a new race. What can happen is that two reader-processes
> may get mixed-up messages, but more than one reader isn't allowed here
> anyway.
>
> Please apply this patch to 2.4 and 2.2, it works for both.

Armin,

Your patch does not apply cleanly against my tree.

Please regenerate it.

2002-02-28 21:32:53

by petter wahlman

Subject: Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

On Thu, 2002-02-28 at 18:41, Marcelo Tosatti wrote:
>
>
> On Wed, 27 Feb 2002, Armin Schindler wrote:
>
> > The patch below fixes the race condition with copy_to_user and will
> > not introduce a new race. What can happen is that two reader-processes
> > may get mixed-up messages, but more than one reader isn't allowed here
> > anyway.
> >
> > Please apply this patch to 2.4 and 2.2, it works for both.
>
> Armin,
>
> Your patch does not apply cleanly against my tree.
>
> Please regenerate it.
>

--- linux/drivers/isdn/eicon/eicon_mod.c Fri Dec 21 18:41:54 2001
+++ linux-2.4.18-pw/drivers/isdn/eicon/eicon_mod.c Thu Feb 28
21:48:32 2002
@@ -665,8 +665,11 @@
else
cnt = skb->len;

- if (user)
+ if (user) {
+ spin_unlock_irqrestore(&eicon_lock,
flags);
copy_to_user(p, skb->data, cnt);
+ spin_lock_irqsave(&eicon_lock, flags);
+ }
else
memcpy(p, skb->data, cnt);

2002-03-01 07:33:18

by Armin Schindler

Subject: Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

On Thu, 28 Feb 2002, Marcelo Tosatti wrote:
>
>
> On Wed, 27 Feb 2002, Armin Schindler wrote:
>
> > The patch below fixes the race condition with copy_to_user and will
> > not introduce a new race. What can happen is that two reader-processes
> > may get mixed-up messages, but more than one reader isn't allowed here
> > anyway.
> >
> > Please apply this patch to 2.4 and 2.2, it works for both.
>
> Armin,
>
> Your patch does not apply cleanly against my tree.
>
> Please regenerate it.

Hi Marcelo,

I don't why the patch does not apply, maybe whitespace problems ?
Okay, here it is again. I tested it with 2.4.18.

Thanks Armin


diff -Nurb pristine/drivers/isdn/eicon/eicon_mod.c linux/drivers/isdn/eicon/eicon_mod.c
--- pristine/drivers/isdn/eicon/eicon_mod.c Fri Dec 21 18:41:54 2001
+++ linux/drivers/isdn/eicon/eicon_mod.c Fri Mar 1 08:07:44 2002
@@ -665,8 +665,11 @@
else
cnt = skb->len;

- if (user)
+ if (user) {
+ spin_unlock_irqrestore(&eicon_lock, flags);
copy_to_user(p, skb->data, cnt);
+ spin_lock_irqsave(&eicon_lock, flags);
+ }
else
memcpy(p, skb->data, cnt);