FAILED: patch "[PATCH] tracing: Disable interrupt or preemption before acquiring" failed to apply to 5.4-stable tree

FAILED: patch "[PATCH] tracing: Disable interrupt or preemption before acquiring" failed to apply to 5.4-stable tree

[gregkh]

The patch below does not apply to the 5.4-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.

Possible dependencies:

c0a581d7126c ("tracing: Disable interrupt or preemption before acquiring arch_spinlock_t")

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From c0a581d7126c0bbc96163276f585fd7b4e4d8d0e Mon Sep 17 00:00:00 2001
From: Waiman Long <longman@redhat.com>
Date: Thu, 22 Sep 2022 10:56:22 -0400
Subject: [PATCH] tracing: Disable interrupt or preemption before acquiring
 arch_spinlock_t

It was found that some tracing functions in kernel/trace/trace.c acquire
an arch_spinlock_t with preemption and irqs enabled. An example is the
tracing_saved_cmdlines_size_read() function which intermittently causes
a "BUG: using smp_processor_id() in preemptible" warning when the LTP
read_all_proc test is run.

That can be problematic in case preemption happens after acquiring the
lock. Add the necessary preemption or interrupt disabling code in the
appropriate places before acquiring an arch_spinlock_t.

The convention here is to disable preemption for trace_cmdline_lock and
interupt for max_lock.

Link: https://lkml.kernel.org/r/20220922145622.1744826-1-longman@redhat.com

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: stable@vger.kernel.org
Fixes: a35873a0993b ("tracing: Add conditional snapshot")
Fixes: 939c7a4f04fc ("tracing: Introduce saved_cmdlines_size file")
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Waiman Long <longman@redhat.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d3005279165d..aed7ea6e6045 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1193,12 +1193,14 @@ void *tracing_cond_snapshot_data(struct trace_array *tr)
 {
 	void *cond_data = NULL;
 
+	local_irq_disable();
 	arch_spin_lock(&tr->max_lock);
 
 	if (tr->cond_snapshot)
 		cond_data = tr->cond_snapshot->cond_data;
 
 	arch_spin_unlock(&tr->max_lock);
+	local_irq_enable();
 
 	return cond_data;
 }
@@ -1334,9 +1336,11 @@ int tracing_snapshot_cond_enable(struct trace_array *tr, void *cond_data,
 		goto fail_unlock;
 	}
 
+	local_irq_disable();
 	arch_spin_lock(&tr->max_lock);
 	tr->cond_snapshot = cond_snapshot;
 	arch_spin_unlock(&tr->max_lock);
+	local_irq_enable();
 
 	mutex_unlock(&trace_types_lock);
 
@@ -1363,6 +1367,7 @@ int tracing_snapshot_cond_disable(struct trace_array *tr)
 {
 	int ret = 0;
 
+	local_irq_disable();
 	arch_spin_lock(&tr->max_lock);
 
 	if (!tr->cond_snapshot)
@@ -1373,6 +1378,7 @@ int tracing_snapshot_cond_disable(struct trace_array *tr)
 	}
 
 	arch_spin_unlock(&tr->max_lock);
+	local_irq_enable();
 
 	return ret;
 }
@@ -2200,6 +2206,11 @@ static size_t tgid_map_max;
 
 #define SAVED_CMDLINES_DEFAULT 128
 #define NO_CMDLINE_MAP UINT_MAX
+/*
+ * Preemption must be disabled before acquiring trace_cmdline_lock.
+ * The various trace_arrays' max_lock must be acquired in a context
+ * where interrupt is disabled.
+ */
 static arch_spinlock_t trace_cmdline_lock = __ARCH_SPIN_LOCK_UNLOCKED;
 struct saved_cmdlines_buffer {
 	unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
@@ -2412,7 +2423,11 @@ static int trace_save_cmdline(struct task_struct *tsk)
 	 * the lock, but we also don't want to spin
 	 * nor do we want to disable interrupts,
 	 * so if we miss here, then better luck next time.
+	 *
+	 * This is called within the scheduler and wake up, so interrupts
+	 * had better been disabled and run queue lock been held.
 	 */
+	lockdep_assert_preemption_disabled();
 	if (!arch_spin_trylock(&trace_cmdline_lock))
 		return 0;
 
@@ -5890,9 +5905,11 @@ tracing_saved_cmdlines_size_read(struct file *filp, char __user *ubuf,
 	char buf[64];
 	int r;
 
+	preempt_disable();
 	arch_spin_lock(&trace_cmdline_lock);
 	r = scnprintf(buf, sizeof(buf), "%u\n", savedcmd->cmdline_num);
 	arch_spin_unlock(&trace_cmdline_lock);
+	preempt_enable();
 
 	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
 }
@@ -5917,10 +5934,12 @@ static int tracing_resize_saved_cmdlines(unsigned int val)
 		return -ENOMEM;
 	}
 
+	preempt_disable();
 	arch_spin_lock(&trace_cmdline_lock);
 	savedcmd_temp = savedcmd;
 	savedcmd = s;
 	arch_spin_unlock(&trace_cmdline_lock);
+	preempt_enable();
 	free_saved_cmdlines_buffer(savedcmd_temp);
 
 	return 0;
@@ -6373,10 +6392,12 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
 
 #ifdef CONFIG_TRACER_SNAPSHOT
 	if (t->use_max_tr) {
+		local_irq_disable();
 		arch_spin_lock(&tr->max_lock);
 		if (tr->cond_snapshot)
 			ret = -EBUSY;
 		arch_spin_unlock(&tr->max_lock);
+		local_irq_enable();
 		if (ret)
 			goto out;
 	}
@@ -7436,10 +7457,12 @@ tracing_snapshot_write(struct file *filp, const char __user *ubuf, size_t cnt,
 		goto out;
 	}
 
+	local_irq_disable();
 	arch_spin_lock(&tr->max_lock);
 	if (tr->cond_snapshot)
 		ret = -EBUSY;
 	arch_spin_unlock(&tr->max_lock);
+	local_irq_enable();
 	if (ret)
 		goto out;
 

Re: FAILED: patch "[PATCH] tracing: Disable interrupt or preemption before acquiring" failed to apply to 5.4-stable tree

[Waiman Long]

On 10/16/22 11:37, gregkh@linuxfoundation.org wrote:
> The patch below does not apply to the 5.4-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.
>
> Possible dependencies:
>
> c0a581d7126c ("tracing: Disable interrupt or preemption before acquiring arch_spinlock_t")
>
> thanks,
>
> greg k-h

You just have to remove the lockdep_assert_preemption_disabled() line as 
this macro isn't defined in v5.4. It is a debug statement and its 
removal won't have any functional impact.

Cheers,
Longman

>
> ------------------ original commit in Linus's tree ------------------
>
>  From c0a581d7126c0bbc96163276f585fd7b4e4d8d0e Mon Sep 17 00:00:00 2001
> From: Waiman Long <longman@redhat.com>
> Date: Thu, 22 Sep 2022 10:56:22 -0400
> Subject: [PATCH] tracing: Disable interrupt or preemption before acquiring
>   arch_spinlock_t
>
> It was found that some tracing functions in kernel/trace/trace.c acquire
> an arch_spinlock_t with preemption and irqs enabled. An example is the
> tracing_saved_cmdlines_size_read() function which intermittently causes
> a "BUG: using smp_processor_id() in preemptible" warning when the LTP
> read_all_proc test is run.
>
> That can be problematic in case preemption happens after acquiring the
> lock. Add the necessary preemption or interrupt disabling code in the
> appropriate places before acquiring an arch_spinlock_t.
>
> The convention here is to disable preemption for trace_cmdline_lock and
> interupt for max_lock.
>
> Link: https://lkml.kernel.org/r/20220922145622.1744826-1-longman@redhat.com
>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: stable@vger.kernel.org
> Fixes: a35873a0993b ("tracing: Add conditional snapshot")
> Fixes: 939c7a4f04fc ("tracing: Introduce saved_cmdlines_size file")
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Waiman Long <longman@redhat.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d3005279165d..aed7ea6e6045 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1193,12 +1193,14 @@ void *tracing_cond_snapshot_data(struct trace_array *tr)
>   {
>   	void *cond_data = NULL;
>   
> +	local_irq_disable();
>   	arch_spin_lock(&tr->max_lock);
>   
>   	if (tr->cond_snapshot)
>   		cond_data = tr->cond_snapshot->cond_data;
>   
>   	arch_spin_unlock(&tr->max_lock);
> +	local_irq_enable();
>   
>   	return cond_data;
>   }
> @@ -1334,9 +1336,11 @@ int tracing_snapshot_cond_enable(struct trace_array *tr, void *cond_data,
>   		goto fail_unlock;
>   	}
>   
> +	local_irq_disable();
>   	arch_spin_lock(&tr->max_lock);
>   	tr->cond_snapshot = cond_snapshot;
>   	arch_spin_unlock(&tr->max_lock);
> +	local_irq_enable();
>   
>   	mutex_unlock(&trace_types_lock);
>   
> @@ -1363,6 +1367,7 @@ int tracing_snapshot_cond_disable(struct trace_array *tr)
>   {
>   	int ret = 0;
>   
> +	local_irq_disable();
>   	arch_spin_lock(&tr->max_lock);
>   
>   	if (!tr->cond_snapshot)
> @@ -1373,6 +1378,7 @@ int tracing_snapshot_cond_disable(struct trace_array *tr)
>   	}
>   
>   	arch_spin_unlock(&tr->max_lock);
> +	local_irq_enable();
>   
>   	return ret;
>   }
> @@ -2200,6 +2206,11 @@ static size_t tgid_map_max;
>   
>   #define SAVED_CMDLINES_DEFAULT 128
>   #define NO_CMDLINE_MAP UINT_MAX
> +/*
> + * Preemption must be disabled before acquiring trace_cmdline_lock.
> + * The various trace_arrays' max_lock must be acquired in a context
> + * where interrupt is disabled.
> + */
>   static arch_spinlock_t trace_cmdline_lock = __ARCH_SPIN_LOCK_UNLOCKED;
>   struct saved_cmdlines_buffer {
>   	unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
> @@ -2412,7 +2423,11 @@ static int trace_save_cmdline(struct task_struct *tsk)
>   	 * the lock, but we also don't want to spin
>   	 * nor do we want to disable interrupts,
>   	 * so if we miss here, then better luck next time.
> +	 *
> +	 * This is called within the scheduler and wake up, so interrupts
> +	 * had better been disabled and run queue lock been held.
>   	 */
> +	lockdep_assert_preemption_disabled();
>   	if (!arch_spin_trylock(&trace_cmdline_lock))
>   		return 0;
>   
> @@ -5890,9 +5905,11 @@ tracing_saved_cmdlines_size_read(struct file *filp, char __user *ubuf,
>   	char buf[64];
>   	int r;
>   
> +	preempt_disable();
>   	arch_spin_lock(&trace_cmdline_lock);
>   	r = scnprintf(buf, sizeof(buf), "%u\n", savedcmd->cmdline_num);
>   	arch_spin_unlock(&trace_cmdline_lock);
> +	preempt_enable();
>   
>   	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
>   }
> @@ -5917,10 +5934,12 @@ static int tracing_resize_saved_cmdlines(unsigned int val)
>   		return -ENOMEM;
>   	}
>   
> +	preempt_disable();
>   	arch_spin_lock(&trace_cmdline_lock);
>   	savedcmd_temp = savedcmd;
>   	savedcmd = s;
>   	arch_spin_unlock(&trace_cmdline_lock);
> +	preempt_enable();
>   	free_saved_cmdlines_buffer(savedcmd_temp);
>   
>   	return 0;
> @@ -6373,10 +6392,12 @@ int tracing_set_tracer(struct trace_array *tr, const char *buf)
>   
>   #ifdef CONFIG_TRACER_SNAPSHOT
>   	if (t->use_max_tr) {
> +		local_irq_disable();
>   		arch_spin_lock(&tr->max_lock);
>   		if (tr->cond_snapshot)
>   			ret = -EBUSY;
>   		arch_spin_unlock(&tr->max_lock);
> +		local_irq_enable();
>   		if (ret)
>   			goto out;
>   	}
> @@ -7436,10 +7457,12 @@ tracing_snapshot_write(struct file *filp, const char __user *ubuf, size_t cnt,
>   		goto out;
>   	}
>   
> +	local_irq_disable();
>   	arch_spin_lock(&tr->max_lock);
>   	if (tr->cond_snapshot)
>   		ret = -EBUSY;
>   	arch_spin_unlock(&tr->max_lock);
> +	local_irq_enable();
>   	if (ret)
>   		goto out;
>   
>

Re: FAILED: patch "[PATCH] tracing: Disable interrupt or preemption before acquiring" failed to apply to 5.4-stable tree

[Greg KH]

On Sun, Oct 16, 2022 at 12:26:22PM -0400, Waiman Long wrote:
> 
> On 10/16/22 11:37, gregkh@linuxfoundation.org wrote:
> > The patch below does not apply to the 5.4-stable tree.
> > If someone wants it applied there, or to any other stable or longterm
> > tree, then please email the backport, including the original git commit
> > id to <stable@vger.kernel.org>.
> > 
> > Possible dependencies:
> > 
> > c0a581d7126c ("tracing: Disable interrupt or preemption before acquiring arch_spinlock_t")
> > 
> > thanks,
> > 
> > greg k-h
> 
> You just have to remove the lockdep_assert_preemption_disabled() line as
> this macro isn't defined in v5.4. It is a debug statement and its removal
> won't have any functional impact.

Wonderful, can you please submit a fixed up version like this so that I
can apply it?

thanks,

greg k-h

Re: FAILED: patch "[PATCH] tracing: Disable interrupt or preemption before acquiring" failed to apply to 5.4-stable tree

[Waiman Long]

[-- Attachment #1: Type: text/plain, Size: 914 bytes --]

On 10/16/22 12:35, Greg KH wrote:
> On Sun, Oct 16, 2022 at 12:26:22PM -0400, Waiman Long wrote:
>> On 10/16/22 11:37, gregkh@linuxfoundation.org wrote:
>>> The patch below does not apply to the 5.4-stable tree.
>>> If someone wants it applied there, or to any other stable or longterm
>>> tree, then please email the backport, including the original git commit
>>> id to <stable@vger.kernel.org>.
>>>
>>> Possible dependencies:
>>>
>>> c0a581d7126c ("tracing: Disable interrupt or preemption before acquiring arch_spinlock_t")
>>>
>>> thanks,
>>>
>>> greg k-h
>> You just have to remove the lockdep_assert_preemption_disabled() line as
>> this macro isn't defined in v5.4. It is a debug statement and its removal
>> won't have any functional impact.
> Wonderful, can you please submit a fixed up version like this so that I
> can apply it?
>
> thanks,
>
> greg k-h

Sure. See the attached patch.

Cheers,
Longman

[-- Attachment #2: 0001-tracing-Disable-interrupt-or-preemption-before-acqui.patch --]
[-- Type: text/x-patch, Size: 4945 bytes --]

From feea22a0e08eace4977eae5204abea2ba6c76c6c Mon Sep 17 00:00:00 2001
From: Waiman Long <longman@redhat.com>
Date: Sun, 16 Oct 2022 16:00:29 -0400
Subject: [PATCH] tracing: Disable interrupt or preemption before acquiring
 arch_spinlock_t

From: Waiman Long <longman@redhat.com>

commit c0a581d7126c0bbc96163276f585fd7b4e4d8d0e upstream.

It was found that some tracing functions in kernel/trace/trace.c acquire
an arch_spinlock_t with preemption and irqs enabled. An example is the
tracing_saved_cmdlines_size_read() function which intermittently causes
a "BUG: using smp_processor_id() in preemptible" warning when the LTP
read_all_proc test is run.

That can be problematic in case preemption happens after acquiring the
lock. Add the necessary preemption or interrupt disabling code in the
appropriate places before acquiring an arch_spinlock_t.

The convention here is to disable preemption for trace_cmdline_lock and
interupt for max_lock.

Link: https://lkml.kernel.org/r/20220922145622.1744826-1-longman@redhat.com

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will@kernel.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: stable@vger.kernel.org
Fixes: a35873a0993b ("tracing: Add conditional snapshot")
Fixes: 939c7a4f04fc ("tracing: Introduce saved_cmdlines_size file")
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Waiman Long <longman@redhat.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 55da88f18342..62fd8798b0c4 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1015,12 +1015,14 @@ void *tracing_cond_snapshot_data(struct trace_array *tr)
 {
 	void *cond_data = NULL;
 
+	local_irq_disable();
 	arch_spin_lock(&tr->max_lock);
 
 	if (tr->cond_snapshot)
 		cond_data = tr->cond_snapshot->cond_data;
 
 	arch_spin_unlock(&tr->max_lock);
+	local_irq_enable();
 
 	return cond_data;
 }
@@ -1156,9 +1158,11 @@ int tracing_snapshot_cond_enable(struct trace_array *tr, void *cond_data,
 		goto fail_unlock;
 	}
 
+	local_irq_disable();
 	arch_spin_lock(&tr->max_lock);
 	tr->cond_snapshot = cond_snapshot;
 	arch_spin_unlock(&tr->max_lock);
+	local_irq_enable();
 
 	mutex_unlock(&trace_types_lock);
 
@@ -1185,6 +1189,7 @@ int tracing_snapshot_cond_disable(struct trace_array *tr)
 {
 	int ret = 0;
 
+	local_irq_disable();
 	arch_spin_lock(&tr->max_lock);
 
 	if (!tr->cond_snapshot)
@@ -1195,6 +1200,7 @@ int tracing_snapshot_cond_disable(struct trace_array *tr)
 	}
 
 	arch_spin_unlock(&tr->max_lock);
+	local_irq_enable();
 
 	return ret;
 }
@@ -1951,6 +1957,11 @@ static size_t tgid_map_max;
 
 #define SAVED_CMDLINES_DEFAULT 128
 #define NO_CMDLINE_MAP UINT_MAX
+/*
+ * Preemption must be disabled before acquiring trace_cmdline_lock.
+ * The various trace_arrays' max_lock must be acquired in a context
+ * where interrupt is disabled.
+ */
 static arch_spinlock_t trace_cmdline_lock = __ARCH_SPIN_LOCK_UNLOCKED;
 struct saved_cmdlines_buffer {
 	unsigned map_pid_to_cmdline[PID_MAX_DEFAULT+1];
@@ -2163,6 +2174,9 @@ static int trace_save_cmdline(struct task_struct *tsk)
 	 * the lock, but we also don't want to spin
 	 * nor do we want to disable interrupts,
 	 * so if we miss here, then better luck next time.
+	 *
+	 * This is called within the scheduler and wake up, so interrupts
+	 * had better been disabled and run queue lock been held.
 	 */
 	if (!arch_spin_trylock(&trace_cmdline_lock))
 		return 0;
@@ -5199,9 +5213,11 @@ tracing_saved_cmdlines_size_read(struct file *filp, char __user *ubuf,
 	char buf[64];
 	int r;
 
+	preempt_disable();
 	arch_spin_lock(&trace_cmdline_lock);
 	r = scnprintf(buf, sizeof(buf), "%u\n", savedcmd->cmdline_num);
 	arch_spin_unlock(&trace_cmdline_lock);
+	preempt_enable();
 
 	return simple_read_from_buffer(ubuf, cnt, ppos, buf, r);
 }
@@ -5226,10 +5242,12 @@ static int tracing_resize_saved_cmdlines(unsigned int val)
 		return -ENOMEM;
 	}
 
+	preempt_disable();
 	arch_spin_lock(&trace_cmdline_lock);
 	savedcmd_temp = savedcmd;
 	savedcmd = s;
 	arch_spin_unlock(&trace_cmdline_lock);
+	preempt_enable();
 	free_saved_cmdlines_buffer(savedcmd_temp);
 
 	return 0;
@@ -5684,10 +5702,12 @@ static int tracing_set_tracer(struct trace_array *tr, const char *buf)
 
 #ifdef CONFIG_TRACER_SNAPSHOT
 	if (t->use_max_tr) {
+		local_irq_disable();
 		arch_spin_lock(&tr->max_lock);
 		if (tr->cond_snapshot)
 			ret = -EBUSY;
 		arch_spin_unlock(&tr->max_lock);
+		local_irq_enable();
 		if (ret)
 			goto out;
 	}
@@ -6767,10 +6787,12 @@ tracing_snapshot_write(struct file *filp, const char __user *ubuf, size_t cnt,
 		goto out;
 	}
 
+	local_irq_disable();
 	arch_spin_lock(&tr->max_lock);
 	if (tr->cond_snapshot)
 		ret = -EBUSY;
 	arch_spin_unlock(&tr->max_lock);
+	local_irq_enable();
 	if (ret)
 		goto out;
 
-- 
2.31.1

Re: FAILED: patch "[PATCH] tracing: Disable interrupt or preemption before acquiring" failed to apply to 5.4-stable tree

[Greg KH]

On Sun, Oct 16, 2022 at 04:02:29PM -0400, Waiman Long wrote:
> On 10/16/22 12:35, Greg KH wrote:
> > On Sun, Oct 16, 2022 at 12:26:22PM -0400, Waiman Long wrote:
> > > On 10/16/22 11:37, gregkh@linuxfoundation.org wrote:
> > > > The patch below does not apply to the 5.4-stable tree.
> > > > If someone wants it applied there, or to any other stable or longterm
> > > > tree, then please email the backport, including the original git commit
> > > > id to <stable@vger.kernel.org>.
> > > > 
> > > > Possible dependencies:
> > > > 
> > > > c0a581d7126c ("tracing: Disable interrupt or preemption before acquiring arch_spinlock_t")
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > You just have to remove the lockdep_assert_preemption_disabled() line as
> > > this macro isn't defined in v5.4. It is a debug statement and its removal
> > > won't have any functional impact.
> > Wonderful, can you please submit a fixed up version like this so that I
> > can apply it?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Sure. See the attached patch.

Thanks, now queued up!

greg k-h