ASoC: SOF: Protect swidget->use_count with mutex for kcontrol access race

The use_count of the swidget is protect by ALSA core PCM locking with the
exception when an associated kcontrol is changed.

It has been observed that a rightly timed kcontrol access during stream
stop can result of an attempt to send a control update to a widget which
has been freed up between the check of the use_count and the message
sending.

We need to protect the entire sof_widget_setup() and sof_widget_free()
execution to make it safe to rely on the use_count.
Move the code under an _unlocked() function and use a mutex to protect
the execution of the functions for concurrency.
On the control path we need to use the lock only for the kcontrol access,
the widget_kcontrol_setup() op is called with the lock already held.

Reported-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Link: https://lore.kernel.org/r/20230127120031.10709-18-peter.ujfalusi@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>

diff --git a/sound/soc/sof/ipc3-control.c b/sound/soc/sof/ipc3-control.c
index 3fdc0d8..217ac55 100644 (file)
--- a/sound/soc/sof/ipc3-control.c
+++ b/sound/soc/sof/ipc3-control.c
@@ -12,7 +12,8 @@
 #include "ipc3-priv.h"
 
 /* IPC set()/get() for kcontrols. */
-static int sof_ipc3_set_get_kcontrol_data(struct snd_sof_control *scontrol, bool set)
+static int sof_ipc3_set_get_kcontrol_data(struct snd_sof_control *scontrol,
+                                         bool set, bool lock)
 {
        struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scontrol->scomp);
        struct sof_ipc_ctrl_data *cdata = scontrol->ipc_control_data;
@@ -21,6 +22,7 @@ static int sof_ipc3_set_get_kcontrol_data(struct snd_sof_control *scontrol, bool
        struct snd_sof_widget *swidget;
        bool widget_found = false;
        u32 ipc_cmd, msg_bytes;
+       int ret = 0;
 
        list_for_each_entry(swidget, &sdev->widget_list, list) {
                if (swidget->comp_id == scontrol->comp_id) {
@@ -35,13 +37,18 @@ static int sof_ipc3_set_get_kcontrol_data(struct snd_sof_control *scontrol, bool
                return -EINVAL;
        }
 
+       if (lock)
+               mutex_lock(&swidget->setup_mutex);
+       else
+               lockdep_assert_held(&swidget->setup_mutex);
+
        /*
-        * Volatile controls should always be part of static pipelines and the widget use_count
-        * would always be > 0 in this case. For the others, just return the cached value if the
-        * widget is not set up.
+        * Volatile controls should always be part of static pipelines and the
+        * widget use_count would always be > 0 in this case. For the others,
+        * just return the cached value if the widget is not set up.
         */
        if (!swidget->use_count)
-               return 0;
+               goto unlock;
 
        /*
         * Select the IPC cmd and the ctrl_type based on the ctrl_cmd and the
@@ -81,13 +88,20 @@ static int sof_ipc3_set_get_kcontrol_data(struct snd_sof_control *scontrol, bool
                             sizeof(struct sof_abi_hdr);
                break;
        default:
-               return -EINVAL;
+               ret = -EINVAL;
+               goto unlock;
        }
 
        cdata->rhdr.hdr.size = msg_bytes;
        cdata->elems_remaining = 0;
 
-       return iops->set_get_data(sdev, cdata, cdata->rhdr.hdr.size, set);
+       ret = iops->set_get_data(sdev, cdata, cdata->rhdr.hdr.size, set);
+
+unlock:
+       if (lock)
+               mutex_unlock(&swidget->setup_mutex);
+
+       return ret;
 }
 
 static void snd_sof_refresh_control(struct snd_sof_control *scontrol)
@@ -108,7 +122,7 @@ static void snd_sof_refresh_control(struct snd_sof_control *scontrol)
 
        /* refresh the component data from DSP */
        scontrol->comp_data_dirty = false;
-       ret = sof_ipc3_set_get_kcontrol_data(scontrol, false);
+       ret = sof_ipc3_set_get_kcontrol_data(scontrol, false, true);
        if (ret < 0) {
                dev_err(scomp->dev, "Failed to get control data: %d\n", ret);
 
@@ -156,7 +170,7 @@ static bool sof_ipc3_volume_put(struct snd_sof_control *scontrol,
 
        /* notify DSP of mixer updates */
        if (pm_runtime_active(scomp->dev)) {
-               int ret = sof_ipc3_set_get_kcontrol_data(scontrol, true);
+               int ret = sof_ipc3_set_get_kcontrol_data(scontrol, true, true);
 
                if (ret < 0) {
                        dev_err(scomp->dev, "Failed to set mixer updates for %s\n",
@@ -204,7 +218,7 @@ static bool sof_ipc3_switch_put(struct snd_sof_control *scontrol,
 
        /* notify DSP of mixer updates */
        if (pm_runtime_active(scomp->dev)) {
-               int ret = sof_ipc3_set_get_kcontrol_data(scontrol, true);
+               int ret = sof_ipc3_set_get_kcontrol_data(scontrol, true, true);
 
                if (ret < 0) {
                        dev_err(scomp->dev, "Failed to set mixer updates for %s\n",
@@ -252,7 +266,7 @@ static bool sof_ipc3_enum_put(struct snd_sof_control *scontrol,
 
        /* notify DSP of enum updates */
        if (pm_runtime_active(scomp->dev)) {
-               int ret = sof_ipc3_set_get_kcontrol_data(scontrol, true);
+               int ret = sof_ipc3_set_get_kcontrol_data(scontrol, true, true);
 
                if (ret < 0) {
                        dev_err(scomp->dev, "Failed to set enum updates for %s\n",
@@ -324,7 +338,7 @@ static int sof_ipc3_bytes_put(struct snd_sof_control *scontrol,
 
        /* notify DSP of byte control updates */
        if (pm_runtime_active(scomp->dev))
-               return sof_ipc3_set_get_kcontrol_data(scontrol, true);
+               return sof_ipc3_set_get_kcontrol_data(scontrol, true, true);
 
        return 0;
 }
@@ -438,7 +452,7 @@ static int sof_ipc3_bytes_ext_put(struct snd_sof_control *scontrol,
 
        /* notify DSP of byte control updates */
        if (pm_runtime_active(scomp->dev))
-               return sof_ipc3_set_get_kcontrol_data(scontrol, true);
+               return sof_ipc3_set_get_kcontrol_data(scontrol, true, true);
 
        return 0;
 }
@@ -468,7 +482,7 @@ static int sof_ipc3_bytes_ext_volatile_get(struct snd_sof_control *scontrol,
        cdata->data->abi = SOF_ABI_VERSION;
 
        /* get all the component data from DSP */
-       ret = sof_ipc3_set_get_kcontrol_data(scontrol, false);
+       ret = sof_ipc3_set_get_kcontrol_data(scontrol, false, true);
        if (ret < 0)
                return ret;
 
@@ -647,7 +661,7 @@ static int sof_ipc3_widget_kcontrol_setup(struct snd_sof_dev *sdev,
        list_for_each_entry(scontrol, &sdev->kcontrol_list, list)
                if (scontrol->comp_id == swidget->comp_id) {
                        /* set kcontrol data in DSP */
-                       ret = sof_ipc3_set_get_kcontrol_data(scontrol, true);
+                       ret = sof_ipc3_set_get_kcontrol_data(scontrol, true, false);
                        if (ret < 0) {
                                dev_err(sdev->dev,
                                        "kcontrol %d set up failed for widget %s\n",
@@ -664,7 +678,7 @@ static int sof_ipc3_widget_kcontrol_setup(struct snd_sof_dev *sdev,
                        if (swidget->dynamic_pipeline_widget)
                                continue;
 
-                       ret = sof_ipc3_set_get_kcontrol_data(scontrol, false);
+                       ret = sof_ipc3_set_get_kcontrol_data(scontrol, false, false);
                        if (ret < 0)
                                dev_warn(sdev->dev,
                                         "kcontrol %d read failed for widget %s\n",

diff --git a/sound/soc/sof/ipc3-topology.c b/sound/soc/sof/ipc3-topology.c
index 88b9b95..dceb78b 100644 (file)
--- a/sound/soc/sof/ipc3-topology.c
+++ b/sound/soc/sof/ipc3-topology.c
@@ -2316,7 +2316,9 @@ static int sof_ipc3_tear_down_all_pipelines(struct snd_sof_dev *sdev, bool verif
                /* Do not free widgets for static pipelines with FW older than SOF2.2 */
                if (!verify && !swidget->dynamic_pipeline_widget &&
                    SOF_FW_VER(v->major, v->minor, v->micro) < SOF_FW_VER(2, 2, 0)) {
+                       mutex_lock(&swidget->setup_mutex);
                        swidget->use_count = 0;
+                       mutex_unlock(&swidget->setup_mutex);
                        if (swidget->spipe)
                                swidget->spipe->complete = 0;
                        continue;

diff --git a/sound/soc/sof/ipc4-control.c b/sound/soc/sof/ipc4-control.c
index 0d5a578..67bd223 100644 (file)
--- a/sound/soc/sof/ipc4-control.c
+++ b/sound/soc/sof/ipc4-control.c
@@ -12,7 +12,8 @@
 #include "ipc4-priv.h"
 #include "ipc4-topology.h"
 
-static int sof_ipc4_set_get_kcontrol_data(struct snd_sof_control *scontrol, bool set)
+static int sof_ipc4_set_get_kcontrol_data(struct snd_sof_control *scontrol,
+                                         bool set, bool lock)
 {
        struct sof_ipc4_control_data *cdata = scontrol->ipc_control_data;
        struct snd_soc_component *scomp = scontrol->scomp;
@@ -21,6 +22,7 @@ static int sof_ipc4_set_get_kcontrol_data(struct snd_sof_control *scontrol, bool
        struct sof_ipc4_msg *msg = &cdata->msg;
        struct snd_sof_widget *swidget;
        bool widget_found = false;
+       int ret = 0;
 
        /* find widget associated with the control */
        list_for_each_entry(swidget, &sdev->widget_list, list) {
@@ -35,23 +37,34 @@ static int sof_ipc4_set_get_kcontrol_data(struct snd_sof_control *scontrol, bool
                return -ENOENT;
        }
 
+       if (lock)
+               mutex_lock(&swidget->setup_mutex);
+       else
+               lockdep_assert_held(&swidget->setup_mutex);
+
        /*
-        * Volatile controls should always be part of static pipelines and the widget use_count
-        * would always be > 0 in this case. For the others, just return the cached value if the
-        * widget is not set up.
+        * Volatile controls should always be part of static pipelines and the
+        * widget use_count would always be > 0 in this case. For the others,
+        * just return the cached value if the widget is not set up.
         */
        if (!swidget->use_count)
-               return 0;
+               goto unlock;
 
        msg->primary &= ~SOF_IPC4_MOD_INSTANCE_MASK;
        msg->primary |= SOF_IPC4_MOD_INSTANCE(swidget->instance_id);
 
-       return iops->set_get_data(sdev, msg, msg->data_size, set);
+       ret = iops->set_get_data(sdev, msg, msg->data_size, set);
+
+unlock:
+       if (lock)
+               mutex_unlock(&swidget->setup_mutex);
+
+       return ret;
 }
 
 static int
 sof_ipc4_set_volume_data(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget,
-                        struct snd_sof_control *scontrol)
+                        struct snd_sof_control *scontrol, bool lock)
 {
        struct sof_ipc4_control_data *cdata = scontrol->ipc_control_data;
        struct sof_ipc4_gain *gain = swidget->private;
@@ -90,7 +103,7 @@ sof_ipc4_set_volume_data(struct snd_sof_dev *sdev, struct snd_sof_widget *swidge
                msg->data_ptr = &data;
                msg->data_size = sizeof(data);
 
-               ret = sof_ipc4_set_get_kcontrol_data(scontrol, true);
+               ret = sof_ipc4_set_get_kcontrol_data(scontrol, true, lock);
                msg->data_ptr = NULL;
                msg->data_size = 0;
                if (ret < 0) {
@@ -145,7 +158,7 @@ static bool sof_ipc4_volume_put(struct snd_sof_control *scontrol,
                return false;
        }
 
-       ret = sof_ipc4_set_volume_data(sdev, swidget, scontrol);
+       ret = sof_ipc4_set_volume_data(sdev, swidget, scontrol, true);
        if (ret < 0)
                return false;
 
@@ -175,7 +188,7 @@ static int sof_ipc4_widget_kcontrol_setup(struct snd_sof_dev *sdev, struct snd_s
 
        list_for_each_entry(scontrol, &sdev->kcontrol_list, list)
                if (scontrol->comp_id == swidget->comp_id) {
-                       ret = sof_ipc4_set_volume_data(sdev, swidget, scontrol);
+                       ret = sof_ipc4_set_volume_data(sdev, swidget, scontrol, false);
                        if (ret < 0) {
                                dev_err(sdev->dev, "%s: kcontrol %d set up failed for widget %s\n",
                                        __func__, scontrol->comp_id, swidget->widget->name);

diff --git a/sound/soc/sof/sof-audio.c b/sound/soc/sof/sof-audio.c
index 1e785e7..d350fa4 100644 (file)
--- a/sound/soc/sof/sof-audio.c
+++ b/sound/soc/sof/sof-audio.c
@@ -28,7 +28,8 @@ static void sof_reset_route_setup_status(struct snd_sof_dev *sdev, struct snd_so
                }
 }
 
-int sof_widget_free(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget)
+static int sof_widget_free_unlocked(struct snd_sof_dev *sdev,
+                                   struct snd_sof_widget *swidget)
 {
        const struct sof_ipc_tplg_ops *tplg_ops = sof_ipc_get_ops(sdev, tplg);
        struct snd_sof_widget *pipe_widget;
@@ -70,7 +71,7 @@ int sof_widget_free(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget)
         * skip for static pipelines
         */
        if (swidget->dynamic_pipeline_widget && swidget->id != snd_soc_dapm_scheduler) {
-               ret = sof_widget_free(sdev, pipe_widget);
+               ret = sof_widget_free_unlocked(sdev, pipe_widget);
                if (ret < 0 && !err)
                        err = ret;
        }
@@ -84,9 +85,21 @@ int sof_widget_free(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget)
 
        return err;
 }
+
+int sof_widget_free(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget)
+{
+       int ret;
+
+       mutex_lock(&swidget->setup_mutex);
+       ret = sof_widget_free_unlocked(sdev, swidget);
+       mutex_unlock(&swidget->setup_mutex);
+
+       return ret;
+}
 EXPORT_SYMBOL(sof_widget_free);
 
-int sof_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget)
+static int sof_widget_setup_unlocked(struct snd_sof_dev *sdev,
+                                    struct snd_sof_widget *swidget)
 {
        const struct sof_ipc_tplg_ops *tplg_ops = sof_ipc_get_ops(sdev, tplg);
        bool use_count_decremented = false;
@@ -116,7 +129,7 @@ int sof_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget)
                        goto use_count_dec;
                }
 
-               ret = sof_widget_setup(sdev, swidget->spipe->pipe_widget);
+               ret = sof_widget_setup_unlocked(sdev, swidget->spipe->pipe_widget);
                if (ret < 0)
                        goto use_count_dec;
        }
@@ -160,19 +173,30 @@ int sof_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget)
 
 widget_free:
        /* widget use_count and core ref_count will both be decremented by sof_widget_free() */
-       sof_widget_free(sdev, swidget);
+       sof_widget_free_unlocked(sdev, swidget);
        use_count_decremented = true;
 core_put:
        snd_sof_dsp_core_put(sdev, swidget->core);
 pipe_widget_free:
        if (swidget->id != snd_soc_dapm_scheduler)
-               sof_widget_free(sdev, swidget->spipe->pipe_widget);
+               sof_widget_free_unlocked(sdev, swidget->spipe->pipe_widget);
 use_count_dec:
        if (!use_count_decremented)
                swidget->use_count--;
 
        return ret;
 }
+
+int sof_widget_setup(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget)
+{
+       int ret;
+
+       mutex_lock(&swidget->setup_mutex);
+       ret = sof_widget_setup_unlocked(sdev, swidget);
+       mutex_unlock(&swidget->setup_mutex);
+
+       return ret;
+}
 EXPORT_SYMBOL(sof_widget_setup);
 
 int sof_route_setup(struct snd_sof_dev *sdev, struct snd_soc_dapm_widget *wsource,

diff --git a/sound/soc/sof/sof-audio.h b/sound/soc/sof/sof-audio.h
index f1bbd1a..b0593b4 100644 (file)
--- a/sound/soc/sof/sof-audio.h
+++ b/sound/soc/sof/sof-audio.h
@@ -387,7 +387,16 @@ struct snd_sof_widget {
         * up in the DSP.
         */
        bool prepared;
-       int use_count; /* use_count will be protected by the PCM mutex held by the core */
+
+       struct mutex setup_mutex; /* to protect the swidget setup and free operations */
+
+       /*
+        * use_count is protected by the PCM mutex held by the core and the
+        * setup_mutex against non stream domain races (kcontrol access for
+        * example)
+        */
+       int use_count;
+
        int core;
        int id; /* id is the DAPM widget type */
        /*

diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
index 33ca306..e2f8cd9 100644 (file)
--- a/sound/soc/sof/topology.c
+++ b/sound/soc/sof/topology.c
@@ -1405,6 +1405,8 @@ static int sof_widget_ready(struct snd_soc_component *scomp, int index,
        swidget->id = w->id;
        swidget->pipeline_id = index;
        swidget->private = NULL;
+       mutex_init(&swidget->setup_mutex);
+
        ida_init(&swidget->src_queue_ida);
        ida_init(&swidget->sink_queue_ida);