lkml.org 
[lkml]   [2022]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.14 13/14] ALSA: pcm: Fix potential AB/BA lock with buffer_mutex and mmap_lock
    Date
    From: Takashi Iwai <tiwai@suse.de>

    commit bc55cfd5718c7c23e5524582e9fa70b4d10f2433 upstream.

    syzbot caught a potential deadlock between the PCM
    runtime->buffer_mutex and the mm->mmap_lock. It was brought by the
    recent fix to cover the racy read/write and other ioctls, and in that
    commit, I overlooked a (hopefully only) corner case that may take the
    revert lock, namely, the OSS mmap. The OSS mmap operation
    exceptionally allows to re-configure the parameters inside the OSS
    mmap syscall, where mm->mmap_mutex is already held. Meanwhile, the
    copy_from/to_user calls at read/write operations also take the
    mm->mmap_lock internally, hence it may lead to a AB/BA deadlock.

    A similar problem was already seen in the past and we fixed it with a
    refcount (in commit b248371628aa). The former fix covered only the
    call paths with OSS read/write and OSS ioctls, while we need to cover
    the concurrent access via both ALSA and OSS APIs now.

    This patch addresses the problem above by replacing the buffer_mutex
    lock in the read/write operations with a refcount similar as we've
    used for OSS. The new field, runtime->buffer_accessing, keeps the
    number of concurrent read/write operations. Unlike the former
    buffer_mutex protection, this protects only around the
    copy_from/to_user() calls; the other codes are basically protected by
    the PCM stream lock. The refcount can be a negative, meaning blocked
    by the ioctls. If a negative value is seen, the read/write aborts
    with -EBUSY. In the ioctl side, OTOH, they check this refcount, too,
    and set to a negative value for blocking unless it's already being
    accessed.

    Reported-by: syzbot+6e5c88838328e99c7e1c@syzkaller.appspotmail.com
    Fixes: dca947d4d26d ("ALSA: pcm: Fix races among concurrent read/write and buffer changes")
    Cc: <stable@vger.kernel.org>
    Link: https://lore.kernel.org/r/000000000000381a0d05db622a81@google.com
    Link: https://lore.kernel.org/r/20220330120903.4738-1-tiwai@suse.de
    Signed-off-by: Takashi Iwai <tiwai@suse.de>
    [OP: backport to 4.14: adjusted context]
    Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    include/sound/pcm.h | 1 +
    sound/core/pcm.c | 1 +
    sound/core/pcm_lib.c | 9 +++++----
    sound/core/pcm_native.c | 39 ++++++++++++++++++++++++++++++++-------
    4 files changed, 39 insertions(+), 11 deletions(-)

    --- a/include/sound/pcm.h
    +++ b/include/sound/pcm.h
    @@ -397,6 +397,7 @@ struct snd_pcm_runtime {
    wait_queue_head_t tsleep; /* transfer sleep */
    struct fasync_struct *fasync;
    struct mutex buffer_mutex; /* protect for buffer changes */
    + atomic_t buffer_accessing; /* >0: in r/w operation, <0: blocked */

    /* -- private section -- */
    void *private_data;
    --- a/sound/core/pcm.c
    +++ b/sound/core/pcm.c
    @@ -1033,6 +1033,7 @@ int snd_pcm_attach_substream(struct snd_

    runtime->status->state = SNDRV_PCM_STATE_OPEN;
    mutex_init(&runtime->buffer_mutex);
    + atomic_set(&runtime->buffer_accessing, 0);

    substream->runtime = runtime;
    substream->private_data = pcm->private_data;
    --- a/sound/core/pcm_lib.c
    +++ b/sound/core/pcm_lib.c
    @@ -1878,11 +1878,9 @@ static int wait_for_avail(struct snd_pcm
    if (avail >= runtime->twake)
    break;
    snd_pcm_stream_unlock_irq(substream);
    - mutex_unlock(&runtime->buffer_mutex);

    tout = schedule_timeout(wait_time);

    - mutex_lock(&runtime->buffer_mutex);
    snd_pcm_stream_lock_irq(substream);
    set_current_state(TASK_INTERRUPTIBLE);
    switch (runtime->status->state) {
    @@ -2176,7 +2174,6 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(str

    nonblock = !!(substream->f_flags & O_NONBLOCK);

    - mutex_lock(&runtime->buffer_mutex);
    snd_pcm_stream_lock_irq(substream);
    err = pcm_accessible_state(runtime);
    if (err < 0)
    @@ -2229,10 +2226,15 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(str
    snd_pcm_stream_unlock_irq(substream);
    return -EINVAL;
    }
    + if (!atomic_inc_unless_negative(&runtime->buffer_accessing)) {
    + err = -EBUSY;
    + goto _end_unlock;
    + }
    snd_pcm_stream_unlock_irq(substream);
    err = writer(substream, appl_ofs, data, offset, frames,
    transfer);
    snd_pcm_stream_lock_irq(substream);
    + atomic_dec(&runtime->buffer_accessing);
    if (err < 0)
    goto _end_unlock;
    err = pcm_accessible_state(runtime);
    @@ -2262,7 +2264,6 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(str
    if (xfer > 0 && err >= 0)
    snd_pcm_update_state(substream, runtime);
    snd_pcm_stream_unlock_irq(substream);
    - mutex_unlock(&runtime->buffer_mutex);
    return xfer > 0 ? (snd_pcm_sframes_t)xfer : err;
    }
    EXPORT_SYMBOL(__snd_pcm_lib_xfer);
    --- a/sound/core/pcm_native.c
    +++ b/sound/core/pcm_native.c
    @@ -634,6 +634,24 @@ static int snd_pcm_hw_params_choose(stru
    return 0;
    }

    +/* acquire buffer_mutex; if it's in r/w operation, return -EBUSY, otherwise
    + * block the further r/w operations
    + */
    +static int snd_pcm_buffer_access_lock(struct snd_pcm_runtime *runtime)
    +{
    + if (!atomic_dec_unless_positive(&runtime->buffer_accessing))
    + return -EBUSY;
    + mutex_lock(&runtime->buffer_mutex);
    + return 0; /* keep buffer_mutex, unlocked by below */
    +}
    +
    +/* release buffer_mutex and clear r/w access flag */
    +static void snd_pcm_buffer_access_unlock(struct snd_pcm_runtime *runtime)
    +{
    + mutex_unlock(&runtime->buffer_mutex);
    + atomic_inc(&runtime->buffer_accessing);
    +}
    +
    #if IS_ENABLED(CONFIG_SND_PCM_OSS)
    #define is_oss_stream(substream) ((substream)->oss.oss)
    #else
    @@ -644,14 +662,16 @@ static int snd_pcm_hw_params(struct snd_
    struct snd_pcm_hw_params *params)
    {
    struct snd_pcm_runtime *runtime;
    - int err = 0, usecs;
    + int err, usecs;
    unsigned int bits;
    snd_pcm_uframes_t frames;

    if (PCM_RUNTIME_CHECK(substream))
    return -ENXIO;
    runtime = substream->runtime;
    - mutex_lock(&runtime->buffer_mutex);
    + err = snd_pcm_buffer_access_lock(runtime);
    + if (err < 0)
    + return err;
    snd_pcm_stream_lock_irq(substream);
    switch (runtime->status->state) {
    case SNDRV_PCM_STATE_OPEN:
    @@ -756,7 +776,7 @@ static int snd_pcm_hw_params(struct snd_
    substream->ops->hw_free(substream);
    }
    unlock:
    - mutex_unlock(&runtime->buffer_mutex);
    + snd_pcm_buffer_access_unlock(runtime);
    return err;
    }

    @@ -789,7 +809,9 @@ static int snd_pcm_hw_free(struct snd_pc
    if (PCM_RUNTIME_CHECK(substream))
    return -ENXIO;
    runtime = substream->runtime;
    - mutex_lock(&runtime->buffer_mutex);
    + result = snd_pcm_buffer_access_lock(runtime);
    + if (result < 0)
    + return result;
    snd_pcm_stream_lock_irq(substream);
    switch (runtime->status->state) {
    case SNDRV_PCM_STATE_SETUP:
    @@ -809,7 +831,7 @@ static int snd_pcm_hw_free(struct snd_pc
    snd_pcm_set_state(substream, SNDRV_PCM_STATE_OPEN);
    pm_qos_remove_request(&substream->latency_pm_qos_req);
    unlock:
    - mutex_unlock(&runtime->buffer_mutex);
    + snd_pcm_buffer_access_unlock(runtime);
    return result;
    }

    @@ -1176,12 +1198,15 @@ static int snd_pcm_action_nonatomic(cons
    int res;

    down_read(&snd_pcm_link_rwsem);
    - mutex_lock(&substream->runtime->buffer_mutex);
    + res = snd_pcm_buffer_access_lock(substream->runtime);
    + if (res < 0)
    + goto unlock;
    if (snd_pcm_stream_linked(substream))
    res = snd_pcm_action_group(ops, substream, state, 0);
    else
    res = snd_pcm_action_single(ops, substream, state);
    - mutex_unlock(&substream->runtime->buffer_mutex);
    + snd_pcm_buffer_access_unlock(substream->runtime);
    + unlock:
    up_read(&snd_pcm_link_rwsem);
    return res;
    }

    \
     
     \ /
      Last update: 2022-05-13 16:27    [W:4.897 / U:0.316 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site