Unverified Commit f94f3915 authored by Peter Ujfalusi's avatar Peter Ujfalusi Committed by Mark Brown
Browse files

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: default avatarGuennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: default avatarPeter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: default avatarPierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: default avatarRanjani Sridharan <ranjani.sridharan@linux.intel.com>
Link: https://lore.kernel.org/r/20230127120031.10709-18-peter.ujfalusi@linux.intel.com


Signed-off-by: default avatarMark Brown <broonie@kernel.org>
parent 955a6f13

sound/soc/sof/ipc3-control.c

+30 −16
Original line number Diff line number Diff line
@@ -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",

sound/soc/sof/ipc3-topology.c

+2 −0
Original line number Diff line number Diff line
@@ -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;

sound/soc/sof/ipc4-control.c

+23 −10
Original line number Diff line number Diff line
@@ -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);

sound/soc/sof/sof-audio.c

+30 −6
Original line number Diff line number Diff line
@@ -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,

sound/soc/sof/sof-audio.h

+10 −1
Original line number Diff line number Diff line
@@ -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 */

	/*