2.4.18 Eicon ISDN driver fix.
PATCH 1/1 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);Review
Quoted 9 lines · petter wahlman
+++
linux-2.4.18-pw/drivers/isdn/eicon/eicon_mod.cMon Feb 25(quoted diff) +2
-1- if (user) + if (user) { + spin_unlock_irqrestore(&eicon_lock,flags); copy_to_user(p, skb->data, cnt);(quoted diff) +2
-0+ 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 ?
Signature · 2 lines
| Dave Jones. http://www.codemonkey.org.uk | SuSE Labs
Quoted 11 lines · Dave Jones
Review
Quoted 9 lines · petter wahlman
+++
linux-2.4.18-pw/drivers/isdn/eicon/eicon_mod.cMon Feb 25(quoted diff) +2
-1- if (user) + if (user) { + spin_unlock_irqrestore(&eicon_lock,flags); copy_to_user(p, skb->data, cnt);(quoted diff) +2
-0+ spin_lock_irqsave(&eicon_lock, flags); + }
Quoted 5 lines
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.
Quoted 4 lines
Signature · 2 lines
| Dave Jones. http://www.codemonkey.org.uk | SuSE Labs
Petter Wahlman.
Quoted 2 lines
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
Quoted 5 lines · petter wahlman
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);Quoted 6 lines · Armin Schindler
The patch below fixes the race condition with
copy_to_userand 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.
Quoted 9 lines · Marcelo Tosatti
Quoted 5 lines · Armin Schindler
The patch below fixes the race condition with
copy_to_userand 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.
Quoted 8 lines
Quoted 1 line
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);Quoted 9 lines · Marcelo Tosatti
Quoted 5 lines · Armin Schindler
The patch below fixes the race condition with
copy_to_userand 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.
Quoted 7 lines
Quoted 1 line
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
drivers/isdn/eicon/eicon_mod.c +4 -1
@@ -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);