Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41

Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41

[Abramo Bagnara]

Jaroslav Kysela ha scritto:
> Modified Files:
> 	rawmidi.c 
> Log Message:
> copy_*_user() function cannot be called from spinlock context
> 
> Index: rawmidi.c
> ===================================================================
> RCS file: /cvsroot/alsa/alsa-kernel/core/rawmidi.c,v
> retrieving revision 1.40
> retrieving revision 1.41
> diff -u -r1.40 -r1.41
> --- rawmidi.c	23 Oct 2003 14:34:52 -0000	1.40
> +++ rawmidi.c	1 Feb 2004 16:34:28 -0000	1.41
> @@ -909,10 +909,11 @@
>  		if (kernel) {
>  			memcpy(buf + result, runtime->buffer + runtime->appl_ptr, count1);
>  		} else {
> +			spin_unlock_irqrestore(&runtime->lock, flags);
>  			if (copy_to_user(buf + result, runtime->buffer + runtime->appl_ptr, count1)) {
> -				spin_unlock_irqrestore(&runtime->lock, flags);
>  				return result > 0 ? result : -EFAULT;
>  			}
> +			spin_lock_irqsave(&runtime->lock, flags);
>  		}
>  		runtime->appl_ptr += count1;
>  		runtime->appl_ptr %= runtime->buffer_size;
> @@ -1133,10 +1134,13 @@
>  		if (kernel) {
>  			memcpy(runtime->buffer + runtime->appl_ptr, buf, count1);
>  		} else {
> +			spin_unlock_irqrestore(&runtime->lock, flags);
>  			if (copy_from_user(runtime->buffer + runtime->appl_ptr, buf, count1)) {
> +				spin_lock_irqsave(&runtime->lock, flags);
>  				result = result > 0 ? result : -EFAULT;
>  				goto __end;
>  			}
> +			spin_lock_irqsave(&runtime->lock, flags);
>  		}
>  		runtime->appl_ptr += count1;
>  		runtime->appl_ptr %= runtime->buffer_size;


With this patch you break write and read atomicity, you should:

1) read and save appl_ptr
2) update appl_ptr and avail
3) leave critical area
4) copy from/to user using saved appl_ptr
5) reenter critical area

I've just checked and a similar mistake has been done too for PCM (and 
other places?).


-- 
Abramo Bagnara                       mailto:abramo.bagnara@tin.it

Opera Unica                          Phone: +39.0546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy


-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn

Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41

[Jaroslav Kysela]

On Sun, 1 Feb 2004, Abramo Bagnara wrote:

> Jaroslav Kysela ha scritto:
> > Modified Files:
> > 	rawmidi.c 
> > Log Message:
> > copy_*_user() function cannot be called from spinlock context
> > 
> > Index: rawmidi.c
> > ===================================================================
> > RCS file: /cvsroot/alsa/alsa-kernel/core/rawmidi.c,v
> > retrieving revision 1.40
> > retrieving revision 1.41
> > diff -u -r1.40 -r1.41
> > --- rawmidi.c	23 Oct 2003 14:34:52 -0000	1.40
> > +++ rawmidi.c	1 Feb 2004 16:34:28 -0000	1.41
> > @@ -909,10 +909,11 @@
> >  		if (kernel) {
> >  			memcpy(buf + result, runtime->buffer + runtime->appl_ptr, count1);
> >  		} else {
> > +			spin_unlock_irqrestore(&runtime->lock, flags);
> >  			if (copy_to_user(buf + result, runtime->buffer + runtime->appl_ptr, count1)) {
> > -				spin_unlock_irqrestore(&runtime->lock, flags);
> >  				return result > 0 ? result : -EFAULT;
> >  			}
> > +			spin_lock_irqsave(&runtime->lock, flags);
> >  		}
> >  		runtime->appl_ptr += count1;
> >  		runtime->appl_ptr %= runtime->buffer_size;
> > @@ -1133,10 +1134,13 @@
> >  		if (kernel) {
> >  			memcpy(runtime->buffer + runtime->appl_ptr, buf, count1);
> >  		} else {
> > +			spin_unlock_irqrestore(&runtime->lock, flags);
> >  			if (copy_from_user(runtime->buffer + runtime->appl_ptr, buf, count1)) {
> > +				spin_lock_irqsave(&runtime->lock, flags);
> >  				result = result > 0 ? result : -EFAULT;
> >  				goto __end;
> >  			}
> > +			spin_lock_irqsave(&runtime->lock, flags);
> >  		}
> >  		runtime->appl_ptr += count1;
> >  		runtime->appl_ptr %= runtime->buffer_size;
> 
> 
> With this patch you break write and read atomicity, you should:
> 
> 1) read and save appl_ptr
> 2) update appl_ptr and avail
> 3) leave critical area
> 4) copy from/to user using saved appl_ptr

But hardware can overwrite data (read) or send data which are not yet 
available (write) when we are not in the spinlock context and data are not 
copied.

If we need atomic read() and write() we must probably add a semaphore.

> 5) reenter critical area
> 
> I've just checked and a similar mistake has been done too for PCM (and 
> other places?).

Yes, but the question is, in which case is atomicity necessary. So far,
I see the problem only for the O_APPEND mode (rawmidi).

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs


-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn

Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41

[Abramo Bagnara]

Jaroslav Kysela ha scritto:
> On Sun, 1 Feb 2004, Abramo Bagnara wrote:
> 
> 
>>Jaroslav Kysela ha scritto:
>>
>>>Modified Files:
>>>	rawmidi.c 
>>>Log Message:
>>>copy_*_user() function cannot be called from spinlock context
>>>
>>>Index: rawmidi.c
>>>===================================================================
>>>RCS file: /cvsroot/alsa/alsa-kernel/core/rawmidi.c,v
>>>retrieving revision 1.40
>>>retrieving revision 1.41
>>>diff -u -r1.40 -r1.41
>>>--- rawmidi.c	23 Oct 2003 14:34:52 -0000	1.40
>>>+++ rawmidi.c	1 Feb 2004 16:34:28 -0000	1.41
>>>@@ -909,10 +909,11 @@
>>> 		if (kernel) {
>>> 			memcpy(buf + result, runtime->buffer + runtime->appl_ptr, count1);
>>> 		} else {
>>>+			spin_unlock_irqrestore(&runtime->lock, flags);
>>> 			if (copy_to_user(buf + result, runtime->buffer + runtime->appl_ptr, count1)) {
>>>-				spin_unlock_irqrestore(&runtime->lock, flags);
>>> 				return result > 0 ? result : -EFAULT;
>>> 			}
>>>+			spin_lock_irqsave(&runtime->lock, flags);
>>> 		}
>>> 		runtime->appl_ptr += count1;
>>> 		runtime->appl_ptr %= runtime->buffer_size;
>>>@@ -1133,10 +1134,13 @@
>>> 		if (kernel) {
>>> 			memcpy(runtime->buffer + runtime->appl_ptr, buf, count1);
>>> 		} else {
>>>+			spin_unlock_irqrestore(&runtime->lock, flags);
>>> 			if (copy_from_user(runtime->buffer + runtime->appl_ptr, buf, count1)) {
>>>+				spin_lock_irqsave(&runtime->lock, flags);
>>> 				result = result > 0 ? result : -EFAULT;
>>> 				goto __end;
>>> 			}
>>>+			spin_lock_irqsave(&runtime->lock, flags);
>>> 		}
>>> 		runtime->appl_ptr += count1;
>>> 		runtime->appl_ptr %= runtime->buffer_size;
>>
>>
>>With this patch you break write and read atomicity, you should:
>>
>>1) read and save appl_ptr
>>2) update appl_ptr and avail
>>3) leave critical area
>>4) copy from/to user using saved appl_ptr
> 
> 
> But hardware can overwrite data (read) or send data which are not yet 
> available (write) when we are not in the spinlock context and data are not 
> copied.

I see.

> If we need atomic read() and write() we must probably add a semaphore.

Too heavyweight, I propose:

1) size_t ptr = appl_ptr_next
2) update appl_ptr_next etc.
3) pending++
4) spin_unlock
5) copy from/to user
6) spin_lock
7) if (--pending) appl_ptr = appl_ptr_next

-- 
Abramo Bagnara                       mailto:abramo.bagnara@tin.it

Opera Unica                          Phone: +39.0546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy


-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn

Re: Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41

[Abramo Bagnara]

Abramo Bagnara ha scritto:
> Jaroslav Kysela ha scritto:
> 
>> On Sun, 1 Feb 2004, Abramo Bagnara wrote:
>>
>>
>>> Jaroslav Kysela ha scritto:
>>>
>>>> Modified Files:
>>>>     rawmidi.c Log Message:
>>>> copy_*_user() function cannot be called from spinlock context
>>>>
>>>> Index: rawmidi.c
>>>> ===================================================================
>>>> RCS file: /cvsroot/alsa/alsa-kernel/core/rawmidi.c,v
>>>> retrieving revision 1.40
>>>> retrieving revision 1.41
>>>> diff -u -r1.40 -r1.41
>>>> --- rawmidi.c    23 Oct 2003 14:34:52 -0000    1.40
>>>> +++ rawmidi.c    1 Feb 2004 16:34:28 -0000    1.41
>>>> @@ -909,10 +909,11 @@
>>>>         if (kernel) {
>>>>             memcpy(buf + result, runtime->buffer + 
>>>> runtime->appl_ptr, count1);
>>>>         } else {
>>>> +            spin_unlock_irqrestore(&runtime->lock, flags);
>>>>             if (copy_to_user(buf + result, runtime->buffer + 
>>>> runtime->appl_ptr, count1)) {
>>>> -                spin_unlock_irqrestore(&runtime->lock, flags);
>>>>                 return result > 0 ? result : -EFAULT;
>>>>             }
>>>> +            spin_lock_irqsave(&runtime->lock, flags);
>>>>         }
>>>>         runtime->appl_ptr += count1;
>>>>         runtime->appl_ptr %= runtime->buffer_size;
>>>> @@ -1133,10 +1134,13 @@
>>>>         if (kernel) {
>>>>             memcpy(runtime->buffer + runtime->appl_ptr, buf, count1);
>>>>         } else {
>>>> +            spin_unlock_irqrestore(&runtime->lock, flags);
>>>>             if (copy_from_user(runtime->buffer + runtime->appl_ptr, 
>>>> buf, count1)) {
>>>> +                spin_lock_irqsave(&runtime->lock, flags);
>>>>                 result = result > 0 ? result : -EFAULT;
>>>>                 goto __end;
>>>>             }
>>>> +            spin_lock_irqsave(&runtime->lock, flags);
>>>>         }
>>>>         runtime->appl_ptr += count1;
>>>>         runtime->appl_ptr %= runtime->buffer_size;
>>>
>>>
>>>
>>> With this patch you break write and read atomicity, you should:
>>>
>>> 1) read and save appl_ptr
>>> 2) update appl_ptr and avail
>>> 3) leave critical area
>>> 4) copy from/to user using saved appl_ptr
>>
>>
>>
>> But hardware can overwrite data (read) or send data which are not yet 
>> available (write) when we are not in the spinlock context and data are 
>> not copied.
> 
> 
> I see.
> 
>> If we need atomic read() and write() we must probably add a semaphore.
> 
> 
> Too heavyweight, I propose:
> 
> 1) size_t ptr = appl_ptr_next
> 2) update appl_ptr_next etc.
> 3) pending++
> 4) spin_unlock
> 5) copy from/to user

Using ptr of course

> 6) spin_lock
> 7) if (--pending) appl_ptr = appl_ptr_next

Sorry, this should be:
7) if (--pending == 0) appl_ptr = appl_ptr_next


-- 
Abramo Bagnara                       mailto:abramo.bagnara@tin.it

Opera Unica                          Phone: +39.0546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy


-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn

Re: Re: [alsa-cvslog] CVS: alsa-kernel/core rawmidi.c,1.40,1.41

[Takashi Iwai]

At Sun, 01 Feb 2004 18:04:14 +0100,
Abramo Bagnara wrote:
(snip)
> With this patch you break write and read atomicity, you should:
> 
> 1) read and save appl_ptr
> 2) update appl_ptr and avail
> 3) leave critical area
> 4) copy from/to user using saved appl_ptr
> 5) reenter critical area
> 
> I've just checked and a similar mistake has been done too for PCM (and 
> other places?).

or simply use semaphore instead of spinlock?
(kernel=1 can be in the interrupt context, so it'd better to down/up
 semaphore in snd_rawmidi_read(), write(), etc.)


Takashi


-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn