Commit f85d39dd7ed8 ("kcov, usb: disable interrupts in
kcov_remote_start_usb_softirq") introduced a local_irq_save() in the
kcov_remote_start_usb_softirq() wrapper, placing kcov_remote_start() in
atomic context.

The previous patch addressed this by converting the global
kcov_remote_lock to a non-sleeping raw_spinlock_t. However, per-CPU
data in kcov_remote_start() and kcov_remote_stop() remains protected
by kcov_percpu_data.lock, which is a local_lock_t.

On PREEMPT_RT kernels, local_lock_t is implemented as a sleeping lock.
Acquiring it from atomic context triggers warnings or crashes due to
invalid sleeping behavior.

The original use of local_lock_t assumed that kcov_remote_start() would
never be called in atomic context. Now that this assumption no longer
holds, replace it with local_irq_save() and local_irq_restore(), which are
safe in all contexts and compatible with the use of raw_spinlock_t.

With this change, both global and per-CPU synchronization primitives are
guaranteed to be non-sleeping, making kcov_remote_start() safe for
use in atomic contexts.

Signed-off-by: Yunseong Kim <ysk@kzalloc.com>
---
 kernel/kcov.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/kernel/kcov.c b/kernel/kcov.c
index 7d9b53385d81..faad3b288ca7 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -90,7 +90,6 @@ static struct list_head kcov_remote_areas = LIST_HEAD_INIT(kcov_remote_areas);
 
 struct kcov_percpu_data {
 	void			*irq_area;
-	local_lock_t		lock;
 
 	unsigned int		saved_mode;
 	unsigned int		saved_size;
@@ -99,9 +98,7 @@ struct kcov_percpu_data {
 	int			saved_sequence;
 };
 
-static DEFINE_PER_CPU(struct kcov_percpu_data, kcov_percpu_data) = {
-	.lock = INIT_LOCAL_LOCK(lock),
-};
+static DEFINE_PER_CPU(struct kcov_percpu_data, kcov_percpu_data);
 
 /* Must be called with kcov_remote_lock locked. */
 static struct kcov_remote *kcov_remote_find(u64 handle)
@@ -862,7 +859,7 @@ void kcov_remote_start(u64 handle)
 	if (!in_task() && !in_softirq_really())
 		return;
 
-	local_lock_irqsave(&kcov_percpu_data.lock, flags);
+	local_irq_save(flags);
 
 	/*
 	 * Check that kcov_remote_start() is not called twice in background
@@ -870,7 +867,7 @@ void kcov_remote_start(u64 handle)
 	 */
 	mode = READ_ONCE(t->kcov_mode);
 	if (WARN_ON(in_task() && kcov_mode_enabled(mode))) {
-		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
+		local_irq_restore(flags);
 		return;
 	}
 	/*
@@ -879,7 +876,7 @@ void kcov_remote_start(u64 handle)
 	 * happened while collecting coverage from a background thread.
 	 */
 	if (WARN_ON(in_serving_softirq() && t->kcov_softirq)) {
-		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
+		local_irq_restore(flags);
 		return;
 	}
 
@@ -887,7 +884,7 @@ void kcov_remote_start(u64 handle)
 	remote = kcov_remote_find(handle);
 	if (!remote) {
 		raw_spin_unlock(&kcov_remote_lock);
-		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
+		local_irq_restore(flags);
 		return;
 	}
 	kcov_debug("handle = %llx, context: %s\n", handle,
@@ -912,13 +909,13 @@ void kcov_remote_start(u64 handle)
 
 	/* Can only happen when in_task(). */
 	if (!area) {
-		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
+		local_irq_restore(flags);
 		area = vmalloc(size * sizeof(unsigned long));
 		if (!area) {
 			kcov_put(kcov);
 			return;
 		}
-		local_lock_irqsave(&kcov_percpu_data.lock, flags);
+		local_irq_save(flags);
 	}
 
 	/* Reset coverage size. */
@@ -930,7 +927,7 @@ void kcov_remote_start(u64 handle)
 	}
 	kcov_start(t, kcov, size, area, mode, sequence);
 
-	local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
+	local_irq_restore(flags);
 
 }
 EXPORT_SYMBOL(kcov_remote_start);
@@ -1004,12 +1001,12 @@ void kcov_remote_stop(void)
 	if (!in_task() && !in_softirq_really())
 		return;
 
-	local_lock_irqsave(&kcov_percpu_data.lock, flags);
+	local_irq_save(flags);
 
 	mode = READ_ONCE(t->kcov_mode);
 	barrier();
 	if (!kcov_mode_enabled(mode)) {
-		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
+		local_irq_restore(flags);
 		return;
 	}
 	/*
@@ -1017,12 +1014,12 @@ void kcov_remote_stop(void)
 	 * actually found the remote handle and started collecting coverage.
 	 */
 	if (in_serving_softirq() && !t->kcov_softirq) {
-		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
+		local_irq_restore(flags);
 		return;
 	}
 	/* Make sure that kcov_softirq is only set when in softirq. */
 	if (WARN_ON(!in_serving_softirq() && t->kcov_softirq)) {
-		local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
+		local_irq_restore(flags);
 		return;
 	}
 
@@ -1052,7 +1049,7 @@ void kcov_remote_stop(void)
 		raw_spin_unlock(&kcov_remote_lock);
 	}
 
-	local_unlock_irqrestore(&kcov_percpu_data.lock, flags);
+	local_irq_restore(flags);
 
 	/* Get in kcov_remote_start(). */
 	kcov_put(kcov);
-- 
2.50.0

On Sun,  3 Aug 2025 07:20:45 +0000
Yunseong Kim <ysk@kzalloc.com> wrote:

> Commit f85d39dd7ed8 ("kcov, usb: disable interrupts in
> kcov_remote_start_usb_softirq") introduced a local_irq_save() in the
> kcov_remote_start_usb_softirq() wrapper, placing kcov_remote_start() in
> atomic context.
> 
> The previous patch addressed this by converting the global

Don't ever use the phrase "The previous patch" in a change log. These get
added to git and it's very hard to find any order of one patch to another.
When doing a git blame 5 years from now, "The previous patch" will be
meaningless.

> kcov_remote_lock to a non-sleeping raw_spinlock_t. However, per-CPU
> data in kcov_remote_start() and kcov_remote_stop() remains protected
> by kcov_percpu_data.lock, which is a local_lock_t.

Instead, you should say something like:

  As kcov_remote_start() is now in atomic context, the kcov_remote lock was
  converted to a non-sleeping raw_spinlock. However, per-cpu ...


> 
> On PREEMPT_RT kernels, local_lock_t is implemented as a sleeping lock.
> Acquiring it from atomic context triggers warnings or crashes due to
> invalid sleeping behavior.
> 
> The original use of local_lock_t assumed that kcov_remote_start() would
> never be called in atomic context. Now that this assumption no longer
> holds, replace it with local_irq_save() and local_irq_restore(), which are
> safe in all contexts and compatible with the use of raw_spinlock_t.

Hmm, if the local_lock_t() is called inside of the taking of the
raw_spinlock_t, then this patch should probably be first. Why introduce a
different bug when fixing another one?

Then the change log of this and the previous patch can both just mention
being called from atomic context.

This change log would probably then say, "in order to convert the kcov locks
to raw_spinlocks, the local_lock_irqsave()s need to be converted over to
local_irq_save()".

-- Steve

> 
> With this change, both global and per-CPU synchronization primitives are
> guaranteed to be non-sleeping, making kcov_remote_start() safe for
> use in atomic contexts.
> 
> Signed-off-by: Yunseong Kim <ysk@kzalloc.com>
> ---

Hi Steve,

Thanks for the detailed feedback and suggestions.

On 8/5/25 1:37 오전, Steven Rostedt wrote:
> On Sun,  3 Aug 2025 07:20:45 +0000
> Yunseong Kim <ysk@kzalloc.com> wrote:
> 
>> Commit f85d39dd7ed8 ("kcov, usb: disable interrupts in
>> kcov_remote_start_usb_softirq") introduced a local_irq_save() in the
>> kcov_remote_start_usb_softirq() wrapper, placing kcov_remote_start() in
>> atomic context.
>>
>> The previous patch addressed this by converting the global
> 
> Don't ever use the phrase "The previous patch" in a change log. These get
> added to git and it's very hard to find any order of one patch to another.
> When doing a git blame 5 years from now, "The previous patch" will be
> meaningless.

I agree that using phrases like "The previous patch" in changelogs is not a
good practice, especially considering future maintenance and git blame
scenarios.

>> kcov_remote_lock to a non-sleeping raw_spinlock_t. However, per-CPU
>> data in kcov_remote_start() and kcov_remote_stop() remains protected
>> by kcov_percpu_data.lock, which is a local_lock_t.
> 
> Instead, you should say something like:
> 
>   As kcov_remote_start() is now in atomic context, the kcov_remote lock was
>   converted to a non-sleeping raw_spinlock. However, per-cpu ...

I’ll revise the commit messages in the next iteration to explicitly
describe the context.

>> On PREEMPT_RT kernels, local_lock_t is implemented as a sleeping lock.
>> Acquiring it from atomic context triggers warnings or crashes due to
>> invalid sleeping behavior.
>>
>> The original use of local_lock_t assumed that kcov_remote_start() would
>> never be called in atomic context. Now that this assumption no longer
>> holds, replace it with local_irq_save() and local_irq_restore(), which are
>> safe in all contexts and compatible with the use of raw_spinlock_t.
> 
> Hmm, if the local_lock_t() is called inside of the taking of the
> raw_spinlock_t, then this patch should probably be first. Why introduce a
> different bug when fixing another one?

Regarding the patch ordering and the potential for introducing new bugs if the
local_lock_t conversions come after the raw_spinlock conversion, that’s a very
good point. I’ll review the patch sequence carefully to ensure the fixes apply
cleanly without regressions.

> Then the change log of this and the previous patch can both just mention
> being called from atomic context.
> 
> This change log would probably then say, "in order to convert the kcov locks
> to raw_spinlocks, the local_lock_irqsave()s need to be converted over to
> local_irq_save()".
> 
> -- Steve

Also, I will update the changelog to clearly state.

Thanks again for your thorough review and guidance!

Best regards,
Yunseong Kim