[alsa-devel] [PATCH v3 2/2] ALSA: fireworks: accessing to user space outside spinlock

[Takashi Sakamoto]

On Aug 31 2016 21:21, Takashi Iwai wrote:
> On Wed, 31 Aug 2016 13:15:33 +0200,
> Takashi Sakamoto wrote:
>>
>> In hwdep interface of fireworks driver, accessing to user space is in a
>> critical section with disabled local interrupt. Depending on architecture,
>> accessing to user space can cause page fault exception. Then local
>> processor stores machine status and handles the synchronous event. A
>> handler corresponding to the event can call task scheduler to wait for
>> preparing pages. In a case of usage of single core processor, the state to
>> disable local interrupt is worse because it don't handle usual interrupts
>> from hardware.
>>
>> This commit fixes this bug, performing the accessing outside spinlock.
>>
>> Reported-by: Vaishali Thakkar <vaishali.thakkar at oracle.com>
>> Cc: stable at vger.kernel.org
>> Fixes: 555e8a8f7f14('ALSA: fireworks: Add command/response functionality into hwdep interface')
>> Signed-off-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
>> ---
>>  sound/firewire/fireworks/fireworks_hwdep.c       | 52 ++++++++++++++++++------
>>  sound/firewire/fireworks/fireworks_transaction.c |  4 +-
>>  2 files changed, 42 insertions(+), 14 deletions(-)
>>
>> diff --git a/sound/firewire/fireworks/fireworks_hwdep.c b/sound/firewire/fireworks/fireworks_hwdep.c
>> index 33df865..2563490 100644
>> --- a/sound/firewire/fireworks/fireworks_hwdep.c
>> +++ b/sound/firewire/fireworks/fireworks_hwdep.c
>> @@ -25,6 +25,7 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained,
>>  {
>>  	unsigned int length, till_end, type;
>>  	struct snd_efw_transaction *t;
>> +	u8 *pull_ptr;
>>  	long count = 0;
>>  
>>  	if (remained < sizeof(type) + sizeof(struct snd_efw_transaction))
>> @@ -38,8 +39,17 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained,
>>  	buf += sizeof(type);
>>  
>>  	/* write into buffer as many responses as possible */
>> +	spin_lock_irq(&efw->lock);
>> +
>> +	/*
>> +	 * When another task reaches here during this task's access to user
>> +	 * space, it picks up current position in buffer and can read the same
>> +	 * series of responses.
>> +	 */
>> +	pull_ptr = efw->pull_ptr;
>> +
>>  	while (efw->resp_queues > 0) {
>> -		t = (struct snd_efw_transaction *)(efw->pull_ptr);
>> +		t = (struct snd_efw_transaction *)(pull_ptr);
>>  		length = be32_to_cpu(t->length) * sizeof(__be32);
>>  
>>  		/* confirm enough space for this response */
>> @@ -49,16 +59,19 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained,
>>  		/* copy from ring buffer to user buffer */
>>  		while (length > 0) {
>>  			till_end = snd_efw_resp_buf_size -
>> -				(unsigned int)(efw->pull_ptr - efw->resp_buf);
>> +				(unsigned int)(pull_ptr - efw->resp_buf);
>>  			till_end = min_t(unsigned int, length, till_end);
>>  
>> -			if (copy_to_user(buf, efw->pull_ptr, till_end))
>> +			spin_unlock_irq(&efw->lock);
>> +
>> +			if (copy_to_user(buf, pull_ptr, till_end))
>>  				return -EFAULT;
>>  
>> -			efw->pull_ptr += till_end;
>> -			if (efw->pull_ptr >= efw->resp_buf +
>> -					     snd_efw_resp_buf_size)
>> -				efw->pull_ptr -= snd_efw_resp_buf_size;
>> +			spin_lock_irq(&efw->lock);
>> +
>> +			pull_ptr += till_end;
>> +			if (pull_ptr >= efw->resp_buf + snd_efw_resp_buf_size)
>> +				pull_ptr -= snd_efw_resp_buf_size;
>>  
>>  			length -= till_end;
>>  			buf += till_end;
>> @@ -69,6 +82,18 @@ hwdep_read_resp_buf(struct snd_efw *efw, char __user *buf, long remained,
>>  		efw->resp_queues--;
>>  	}
>>  
>> +	/*
>> +	 * All of tasks can read from the buffer nearly simultaneously, but the
>> +	 * position of each task is different depending on the length of given
>> +	 * buffer. Here, for simplicity, a position of buffer is set by the
>> +	 * latest task. It's better for a listening application to allow one
>> +	 * thread to read from the buffer. Unless, each task can read different
>> +	 * sequence of responses depending on variation of buffer length.
>> +	 */
>> +	efw->pull_ptr = pull_ptr;
>> +
>> +	spin_unlock_irq(&efw->lock);
> 
> Hrm, I'm afraid that it still doesn't work properly when accessed
> concurrently.  In your code, efw->pull_ptr isn't updated until the end
> of the loop, while dfw->resp_queues are decremented in the loop.
> 
> Suppose resp_queues = 2, and two threads read concurrently.  What
> happens?   Both threads read from the first element only once, and
> resp_queues are decremented twice (one per each).  And now both
> threads go out of the loops, and both set the pull_ptr to the same
> next item, although the second item hasn't been processed.

Just from my overlooking.


Thanks

Takashi Sakamoto