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@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 555e8a8f7f14('ALSA: fireworks: Add command/response functionality into hwdep interface')
> Signed-off-by: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
> ---
> 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.
Takashi
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html