Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

[Dave Jones]

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

[PATCH] 2.4.18 Eicon ISDN driver fix.

[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);

Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

[petter wahlman]

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.

Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

[Alan Cox]

> 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 ?

Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

[Armin Schindler]

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

Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

[Marcelo Tosatti]

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.

Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

[petter wahlman]

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

Re: [PATCH] 2.4.18 Eicon ISDN driver fix.

[Armin Schindler]

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