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);
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
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.
> 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 ?
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);
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.
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);
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);