All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH AUTOSEL 6.9 17/40] ASoC: topology: Fix route memory corruption
@ 2024-08-05 16:17 Vitaly Chikunov
  2024-08-05 17:09 ` Pierre-Louis Bossart
  0 siblings, 1 reply; 14+ messages in thread
From: Vitaly Chikunov @ 2024-08-05 16:17 UTC (permalink / raw)
  To: Sasha Levin, Greg Kroah-Hartman, stable
  Cc: linux-kernel, Amadeusz Sławiński, Pierre-Louis Bossart,
	Péter Ujfalusi, Mark Brown, lgirdwood, perex, tiwai,
	linux-sound

Sasha, Greg,

On Tue, Jul 09, 2024 at 12:18:57PM GMT, Sasha Levin wrote:
> From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> 
> [ Upstream commit 0298f51652be47b79780833e0b63194e1231fa34 ]
> 
> It was reported that recent fix for memory corruption during topology
> load, causes corruption in other cases. Instead of being overeager with
> checking topology, assume that it is properly formatted and just
> duplicate strings.

Can this backport actually be applied to the 6.9/6.6/6.1 stable branches?

I have multiple bug reports about sound not working and memory
corruption on some laptops (for example ICL RAYbook Si1516). See for
example bug reports[1][2], and the fix discussion [3].

dmesg messages from Lenovo ThinkBook 13 gen 1:


  [ 3.555191] sof-audio-pci-intel-cnl 0000:00:1f.3: Firmware info: version 2:2:0-57864
  [ 3.555206] sof-audio-pci-intel-cnl 0000:00:1f.3: Firmware: ABI 3:22:1 Kernel ABI 3:23:0
  [ 3.574043] sof-audio-pci-intel-cnl 0000:00:1f.3: Topology: ABI 3:22:1 Kernel ABI 3:23:0
  [ 3.575180] sof-audio-pci-intel-cnl 0000:00:1f.3: error: sink MIXER1.0> not found
  [ 3.575772] sof-audio-pci-intel-cnl 0000:00:1f.3: error: tplg component load failed -22
  [ 3.575793] sof-audio-pci-intel-cnl 0000:00:1f.3: error: failed to load DSP topology -22
  [ 3.575801] sof-audio-pci-intel-cnl 0000:00:1f.3: ASoC: error at snd_soc_component_probe on 0000:00:1f.3: -22

Error messages from other boots showing memory corruption:

  [ 3.904397] sof-audio-pci-intel-cnl 0000:00:1f.3: error: sink PCM0C03-std-def-alt0.p11@jh\x86Ŝ\xff\xff@\xc8\xff\x82Ŝ\xff\xff`P\x82\xbb\xff\xff\xff\xff\x94$A\xbc\xff\xff\xff\xff\x06 not found
  [ 3.966777] sof-audio-pci-intel-cnl 0000:00:1f.3: error: sink PGA1.0\x01 not found
  [ 3.899748] sof-audio-pci-intel-cnl 0000:00:1f.3: error: source BUF2.0 not found
  [ 3.975359] sof-audio-pci-intel-cnl 0000:00:1f.3: error: source PCM0P\x01pcsc-lite.conf not found
  [ 7.275851] sof-audio-pci-intel-tgl 0000:00:1f.3: error: source HDA1.IN/0123456789:;<=>? not found

[1] https://github.com/thesofproject/sof/issues/9339
[2] https://github.com/thesofproject/sof/issues/9341
[3] https://lore.kernel.org/linux-sound/171812236450.201359.3019210915105428447.b4-ty@kernel.org/T/#m8c4bd5abf453960fde6f826c4b7f84881da63e9d

Thanks,

> 
> Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Closes: https://lore.kernel.org/linux-sound/171812236450.201359.3019210915105428447.b4-ty@kernel.org/T/#m8c4bd5abf453960fde6f826c4b7f84881da63e9d
> Suggested-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> Link: https://lore.kernel.org/r/20240613090126.841189-1-amadeuszx.slawinski@linux.intel.com
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  sound/soc/soc-topology.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 52752e0a5dc27..27aba69894b17 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -1052,21 +1052,15 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>  			break;
>  		}
>  
> -		route->source = devm_kmemdup(tplg->dev, elem->source,
> -					     min(strlen(elem->source), maxlen),
> -					     GFP_KERNEL);
> -		route->sink = devm_kmemdup(tplg->dev, elem->sink,
> -					   min(strlen(elem->sink), maxlen),
> -					   GFP_KERNEL);
> +		route->source = devm_kstrdup(tplg->dev, elem->source, GFP_KERNEL);
> +		route->sink = devm_kstrdup(tplg->dev, elem->sink, GFP_KERNEL);
>  		if (!route->source || !route->sink) {
>  			ret = -ENOMEM;
>  			break;
>  		}
>  
>  		if (strnlen(elem->control, maxlen) != 0) {
> -			route->control = devm_kmemdup(tplg->dev, elem->control,
> -						      min(strlen(elem->control), maxlen),
> -						      GFP_KERNEL);
> +			route->control = devm_kstrdup(tplg->dev, elem->control, GFP_KERNEL);
>  			if (!route->control) {
>  				ret = -ENOMEM;
>  				break;
> -- 
> 2.43.0
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH AUTOSEL 6.9 01/40] workqueue: Refactor worker ID formatting and make wq_worker_comm() use full ID string
@ 2024-07-09 16:18 Sasha Levin
  2024-07-09 16:18 ` [PATCH AUTOSEL 6.9 17/40] ASoC: topology: Fix route memory corruption Sasha Levin
  0 siblings, 1 reply; 14+ messages in thread
From: Sasha Levin @ 2024-07-09 16:18 UTC (permalink / raw)
  To: linux-kernel, stable
  Cc: Tejun Heo, Jan Engelhardt, Linus Torvalds, Sasha Levin

From: Tejun Heo <tj@kernel.org>

[ Upstream commit 2a1b02bcba78f8498ab00d6142e1238d85b01591 ]

Currently, worker ID formatting is open coded in create_worker(),
init_rescuer() and worker_thread() (for %WORKER_DIE case). The formatted ID
is saved into task->comm and wq_worker_comm() uses it as the base name to
append extra information to when generating the name to be shown to
userspace.

However, TASK_COMM_LEN is only 16 leading to badly truncated names for
rescuers. For example, the rescuer for the inet_frag_wq workqueue becomes:

  $ ps -ef | grep '[k]worker/R-inet'
  root         483       2  0 Apr26 ?        00:00:00 [kworker/R-inet_]

Even for non-rescue workers, it's easy to run over 15 characters on
moderately large machines.

Fit it by consolidating worker ID formatting into a new helper
format_worker_id() and calling it from wq_worker_comm() to obtain the
untruncated worker ID string.

  $ ps -ef | grep '[k]worker/R-inet'
  root          60       2  0 12:10 ?        00:00:00 [kworker/R-inet_frag_wq]

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-and-tested-by: Jan Engelhardt <jengelh@inai.de>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 kernel/workqueue.c | 51 ++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index d2dbe099286b9..7634fc32ee05a 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -124,6 +124,7 @@ enum wq_internal_consts {
 	HIGHPRI_NICE_LEVEL	= MIN_NICE,
 
 	WQ_NAME_LEN		= 32,
+	WORKER_ID_LEN		= 10 + WQ_NAME_LEN, /* "kworker/R-" + WQ_NAME_LEN */
 };
 
 /*
@@ -2778,6 +2779,26 @@ static void worker_detach_from_pool(struct worker *worker)
 		complete(detach_completion);
 }
 
+static int format_worker_id(char *buf, size_t size, struct worker *worker,
+			    struct worker_pool *pool)
+{
+	if (worker->rescue_wq)
+		return scnprintf(buf, size, "kworker/R-%s",
+				 worker->rescue_wq->name);
+
+	if (pool) {
+		if (pool->cpu >= 0)
+			return scnprintf(buf, size, "kworker/%d:%d%s",
+					 pool->cpu, worker->id,
+					 pool->attrs->nice < 0  ? "H" : "");
+		else
+			return scnprintf(buf, size, "kworker/u%d:%d",
+					 pool->id, worker->id);
+	} else {
+		return scnprintf(buf, size, "kworker/dying");
+	}
+}
+
 /**
  * create_worker - create a new workqueue worker
  * @pool: pool the new worker will belong to
@@ -2794,7 +2815,6 @@ static struct worker *create_worker(struct worker_pool *pool)
 {
 	struct worker *worker;
 	int id;
-	char id_buf[23];
 
 	/* ID is needed to determine kthread name */
 	id = ida_alloc(&pool->worker_ida, GFP_KERNEL);
@@ -2813,17 +2833,14 @@ static struct worker *create_worker(struct worker_pool *pool)
 	worker->id = id;
 
 	if (!(pool->flags & POOL_BH)) {
-		if (pool->cpu >= 0)
-			snprintf(id_buf, sizeof(id_buf), "%d:%d%s", pool->cpu, id,
-				 pool->attrs->nice < 0  ? "H" : "");
-		else
-			snprintf(id_buf, sizeof(id_buf), "u%d:%d", pool->id, id);
+		char id_buf[WORKER_ID_LEN];
 
+		format_worker_id(id_buf, sizeof(id_buf), worker, pool);
 		worker->task = kthread_create_on_node(worker_thread, worker,
-					pool->node, "kworker/%s", id_buf);
+						      pool->node, "%s", id_buf);
 		if (IS_ERR(worker->task)) {
 			if (PTR_ERR(worker->task) == -EINTR) {
-				pr_err("workqueue: Interrupted when creating a worker thread \"kworker/%s\"\n",
+				pr_err("workqueue: Interrupted when creating a worker thread \"%s\"\n",
 				       id_buf);
 			} else {
 				pr_err_once("workqueue: Failed to create a worker thread: %pe",
@@ -3386,7 +3403,6 @@ static int worker_thread(void *__worker)
 		raw_spin_unlock_irq(&pool->lock);
 		set_pf_worker(false);
 
-		set_task_comm(worker->task, "kworker/dying");
 		ida_free(&pool->worker_ida, worker->id);
 		worker_detach_from_pool(worker);
 		WARN_ON_ONCE(!list_empty(&worker->entry));
@@ -5430,6 +5446,7 @@ static int wq_clamp_max_active(int max_active, unsigned int flags,
 static int init_rescuer(struct workqueue_struct *wq)
 {
 	struct worker *rescuer;
+	char id_buf[WORKER_ID_LEN];
 	int ret;
 
 	if (!(wq->flags & WQ_MEM_RECLAIM))
@@ -5443,7 +5460,9 @@ static int init_rescuer(struct workqueue_struct *wq)
 	}
 
 	rescuer->rescue_wq = wq;
-	rescuer->task = kthread_create(rescuer_thread, rescuer, "kworker/R-%s", wq->name);
+	format_worker_id(id_buf, sizeof(id_buf), rescuer, NULL);
+
+	rescuer->task = kthread_create(rescuer_thread, rescuer, "%s", id_buf);
 	if (IS_ERR(rescuer->task)) {
 		ret = PTR_ERR(rescuer->task);
 		pr_err("workqueue: Failed to create a rescuer kthread for wq \"%s\": %pe",
@@ -6272,19 +6291,15 @@ void show_freezable_workqueues(void)
 /* used to show worker information through /proc/PID/{comm,stat,status} */
 void wq_worker_comm(char *buf, size_t size, struct task_struct *task)
 {
-	int off;
-
-	/* always show the actual comm */
-	off = strscpy(buf, task->comm, size);
-	if (off < 0)
-		return;
-
 	/* stabilize PF_WQ_WORKER and worker pool association */
 	mutex_lock(&wq_pool_attach_mutex);
 
 	if (task->flags & PF_WQ_WORKER) {
 		struct worker *worker = kthread_data(task);
 		struct worker_pool *pool = worker->pool;
+		int off;
+
+		off = format_worker_id(buf, size, worker, pool);
 
 		if (pool) {
 			raw_spin_lock_irq(&pool->lock);
@@ -6303,6 +6318,8 @@ void wq_worker_comm(char *buf, size_t size, struct task_struct *task)
 			}
 			raw_spin_unlock_irq(&pool->lock);
 		}
+	} else {
+		strscpy(buf, task->comm, size);
 	}
 
 	mutex_unlock(&wq_pool_attach_mutex);
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-08-14 14:08 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-05 16:17 [PATCH AUTOSEL 6.9 17/40] ASoC: topology: Fix route memory corruption Vitaly Chikunov
2024-08-05 17:09 ` Pierre-Louis Bossart
2024-08-12  9:53   ` Thorsten Leemhuis
2024-08-12 10:01     ` Amadeusz Sławiński
2024-08-12 10:25       ` Greg Kroah-Hartman
2024-08-12 10:38         ` Vitaly Chikunov
2024-08-12 14:11           ` Greg Kroah-Hartman
2024-08-13 14:42             ` Amadeusz Sławiński
2024-08-14  0:00               ` Vitaly Chikunov
2024-08-14 10:33                 ` Mark Brown
2024-08-14 14:07                   ` Amadeusz Sławiński
2024-08-12 10:24     ` Greg Kroah-Hartman
2024-08-14  2:18     ` Sasha Levin
  -- strict thread matches above, loose matches on Subject: below --
2024-07-09 16:18 [PATCH AUTOSEL 6.9 01/40] workqueue: Refactor worker ID formatting and make wq_worker_comm() use full ID string Sasha Levin
2024-07-09 16:18 ` [PATCH AUTOSEL 6.9 17/40] ASoC: topology: Fix route memory corruption Sasha Levin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.