[PATCH resend] kcov: allow simultaneous KCOV_ENABLE/KCOV_REMOTE_ENABLE

[PATCH resend] kcov: allow simultaneous KCOV_ENABLE/KCOV_REMOTE_ENABLE

[Jann Horn]

Allow the same userspace thread to simultaneously collect normal coverage
in syscall context (KCOV_ENABLE) and remote coverage of asynchronous work
created by the thread (KCOV_REMOTE_ENABLE).
With this, remote KCOV coverage becomes useful for generic fuzzing and not
just fuzzing of specific data injection interfaces.

This requires that the task_struct::kcov_* fields are separated into ones
that are used by the task that generates coverage, and ones that are used
by the task that requested remote coverage. To split this up:

 - Split task_struct::kcov into kcov and kcov_remote. kcov_task_exit() now
   has to clean up both separately.
 - Only use task_struct::kcov_mode on the task that generates coverage.
 - Only reset task_struct::kcov_handle on the task that requested remote
   coverage.

After this change, fields used by the task that generates coverage are:

 - kcov_mode
 - kcov_size
 - kcov_area
 - kcov
 - kcov_sequence
 - kcov_softirq

Fields used by the task that requested remote coverage are:

 - kcov_remote
 - kcov_handle

Signed-off-by: Jann Horn <jannh@google.com>
---
resending because the previous recipient list was incomplete
---
 include/linux/sched.h |  3 ++
 kernel/kcov.c         | 94 ++++++++++++++++++++++++++++-----------------------
 2 files changed, 55 insertions(+), 42 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 368c7b4d7cb5..c3bc248a0d27 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1514,6 +1514,9 @@ struct task_struct {
 	/* KCOV descriptor wired with this task or NULL: */
 	struct kcov			*kcov;
 
+	/* KCOV descriptor for remote coverage collection from other tasks: */
+	struct kcov			*kcov_remote;
+
 	/* KCOV common handle for remote coverage collection: */
 	u64				kcov_handle;
 
diff --git a/kernel/kcov.c b/kernel/kcov.c
index 0b369e88c7c9..bcbde917d4df 100644
--- a/kernel/kcov.c
+++ b/kernel/kcov.c
@@ -368,6 +368,7 @@ static void kcov_start(struct task_struct *t, struct kcov *kcov,
 	WRITE_ONCE(t->kcov_mode, mode);
 }
 
+/* operates on coverage-generator-owned fields */
 static void kcov_stop(struct task_struct *t)
 {
 	WRITE_ONCE(t->kcov_mode, KCOV_MODE_DISABLED);
@@ -377,16 +378,17 @@ static void kcov_stop(struct task_struct *t)
 	t->kcov_area = NULL;
 }
 
+/* operates on coverage-generator-owned fields */
 static void kcov_task_reset(struct task_struct *t)
 {
 	kcov_stop(t);
 	t->kcov_sequence = 0;
-	t->kcov_handle = 0;
 }
 
 void kcov_task_init(struct task_struct *t)
 {
 	kcov_task_reset(t);
+	t->kcov_remote = NULL;
 	t->kcov_handle = current->kcov_handle;
 }
 
@@ -423,11 +425,14 @@ static void kcov_remote_reset(struct kcov *kcov)
 static void kcov_disable(struct task_struct *t, struct kcov *kcov)
 	__must_hold(&kcov->lock)
 {
-	kcov_task_reset(t);
-	if (kcov->remote)
+	if (kcov->remote) {
+		t->kcov_handle = 0;
+		t->kcov_remote = NULL;
 		kcov_remote_reset(kcov);
-	else
+	} else {
+		kcov_task_reset(t);
 		kcov_reset(kcov);
+	}
 }
 
 static void kcov_get(struct kcov *kcov)
@@ -453,41 +458,47 @@ void kcov_task_exit(struct task_struct *t)
 	unsigned long flags;
 
 	kcov = t->kcov;
-	if (kcov == NULL)
-		return;
-
-	spin_lock_irqsave(&kcov->lock, flags);
-	kcov_debug("t = %px, kcov->t = %px\n", t, kcov->t);
-	/*
-	 * For KCOV_ENABLE devices we want to make sure that t->kcov->t == t,
-	 * which comes down to:
-	 *        WARN_ON(!kcov->remote && kcov->t != t);
-	 *
-	 * For KCOV_REMOTE_ENABLE devices, the exiting task is either:
-	 *
-	 * 1. A remote task between kcov_remote_start() and kcov_remote_stop().
-	 *    In this case we should print a warning right away, since a task
-	 *    shouldn't be exiting when it's in a kcov coverage collection
-	 *    section. Here t points to the task that is collecting remote
-	 *    coverage, and t->kcov->t points to the thread that created the
-	 *    kcov device. Which means that to detect this case we need to
-	 *    check that t != t->kcov->t, and this gives us the following:
-	 *        WARN_ON(kcov->remote && kcov->t != t);
-	 *
-	 * 2. The task that created kcov exiting without calling KCOV_DISABLE,
-	 *    and then again we make sure that t->kcov->t == t:
-	 *        WARN_ON(kcov->remote && kcov->t != t);
-	 *
-	 * By combining all three checks into one we get:
-	 */
-	if (WARN_ON(kcov->t != t)) {
+	if (kcov) {
+		spin_lock_irqsave(&kcov->lock, flags);
+		kcov_debug("t = %px, kcov->t = %px\n", t, kcov->t);
+		/*
+		 * This could be a remote task between kcov_remote_start() and
+		 * kcov_remote_stop().
+		 * In this case we should print a warning right away, since a
+		 * task shouldn't be exiting when it's in a kcov coverage
+		 * collection section.
+		 *
+		 * Otherwise, this should be a task that created a local
+		 * kcov instance and hasn't called KCOV_DISABLE.
+		 * Make sure that t->kcov->t is consistent.
+		 */
+		if (WARN_ON(kcov->remote) || WARN_ON(kcov->t != t)) {
+			spin_unlock_irqrestore(&kcov->lock, flags);
+			return;
+		}
+		/* Just to not leave dangling references behind. */
+		kcov_disable(t, kcov);
 		spin_unlock_irqrestore(&kcov->lock, flags);
-		return;
+		kcov_put(kcov);
+	}
+	kcov = t->kcov_remote;
+	if (kcov) {
+		spin_lock_irqsave(&kcov->lock, flags);
+		kcov_debug("t = %px, kcov->t = %px\n", t, kcov->t);
+		/*
+		 * This is a KCOV_REMOTE_ENABLE device, and the task is the
+		 * user task which has requested remote coverage collection.
+		 * Make sure that t->kcov->t is consistent.
+		 */
+		if (WARN_ON(!kcov->remote) || WARN_ON(kcov->t != t)) {
+			spin_unlock_irqrestore(&kcov->lock, flags);
+			return;
+		}
+		/* Just to not leave dangling references behind. */
+		kcov_disable(t, kcov);
+		spin_unlock_irqrestore(&kcov->lock, flags);
+		kcov_put(kcov);
 	}
-	/* Just to not leave dangling references behind. */
-	kcov_disable(t, kcov);
-	spin_unlock_irqrestore(&kcov->lock, flags);
-	kcov_put(kcov);
 }
 
 static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
@@ -629,9 +640,9 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 	case KCOV_DISABLE:
 		/* Disable coverage for the current task. */
 		unused = arg;
-		if (unused != 0 || current->kcov != kcov)
-			return -EINVAL;
 		t = current;
+		if (unused != 0 || (kcov != t->kcov && kcov != t->kcov_remote))
+			return -EINVAL;
 		if (WARN_ON(kcov->t != t))
 			return -EINVAL;
 		kcov_disable(t, kcov);
@@ -641,7 +652,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		if (kcov->mode != KCOV_MODE_INIT || !kcov->area)
 			return -EINVAL;
 		t = current;
-		if (kcov->t != NULL || t->kcov != NULL)
+		if (kcov->t != NULL || t->kcov_remote != NULL)
 			return -EBUSY;
 		remote_arg = (struct kcov_remote_arg *)arg;
 		mode = kcov_get_mode(remote_arg->trace_mode);
@@ -651,8 +662,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
 		    LONG_MAX / sizeof(unsigned long))
 			return -EINVAL;
 		kcov->mode = mode;
-		t->kcov = kcov;
-	        t->kcov_mode = KCOV_MODE_REMOTE;
+		t->kcov_remote = kcov;
 		kcov->t = t;
 		kcov->remote = true;
 		kcov->remote_size = remote_arg->area_size;

---
base-commit: 57b8e2d666a31fa201432d58f5fe3469a0dd83ba
change-id: 20260429-kcov-simultaneous-remote-a1bc6cf0a01b

--  
Jann Horn <jannh@google.com>

Re: [PATCH resend] kcov: allow simultaneous KCOV_ENABLE/KCOV_REMOTE_ENABLE

[Andrew Morton]

On Tue, 05 May 2026 11:00:46 +0200 Jann Horn <jannh@google.com> wrote:

> Allow the same userspace thread to simultaneously collect normal coverage
> in syscall context (KCOV_ENABLE) and remote coverage of asynchronous work
> created by the thread (KCOV_REMOTE_ENABLE).
> With this, remote KCOV coverage becomes useful for generic fuzzing and not
> just fuzzing of specific data injection interfaces.
> 
> This requires that the task_struct::kcov_* fields are separated into ones
> that are used by the task that generates coverage, and ones that are used
> by the task that requested remote coverage. To split this up:
> 
>  - Split task_struct::kcov into kcov and kcov_remote. kcov_task_exit() now
>    has to clean up both separately.
>  - Only use task_struct::kcov_mode on the task that generates coverage.
>  - Only reset task_struct::kcov_handle on the task that requested remote
>    coverage.
> 
> After this change, fields used by the task that generates coverage are:
> 
>  - kcov_mode
>  - kcov_size
>  - kcov_area
>  - kcov
>  - kcov_sequence
>  - kcov_softirq
> 
> Fields used by the task that requested remote coverage are:
> 
>  - kcov_remote
>  - kcov_handle

Thanks, I queued this for testing while we await reviewer input.

checkpatch asked:

WARNING: Using vsprintf specifier '%px' potentially exposes the kernel memory layout, if you don't really need the address please consider using '%p'.
#141: FILE: kernel/kcov.c:463:
+		kcov_debug("t = %px, kcov->t = %px\n", t, kcov->t);

and Sashiko found a femtobug:
	https://sashiko.dev/#/patchset/20260505-kcov-simultaneous-remote-v1-1-a670ba7cefd2@google.com

Re: [PATCH resend] kcov: allow simultaneous KCOV_ENABLE/KCOV_REMOTE_ENABLE

[Jann Horn]

On Sat, May 9, 2026 at 3:24 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 05 May 2026 11:00:46 +0200 Jann Horn <jannh@google.com> wrote:
> > Allow the same userspace thread to simultaneously collect normal coverage
> > in syscall context (KCOV_ENABLE) and remote coverage of asynchronous work
> > created by the thread (KCOV_REMOTE_ENABLE).
> > With this, remote KCOV coverage becomes useful for generic fuzzing and not
> > just fuzzing of specific data injection interfaces.
> >
> > This requires that the task_struct::kcov_* fields are separated into ones
> > that are used by the task that generates coverage, and ones that are used
> > by the task that requested remote coverage. To split this up:
> >
> >  - Split task_struct::kcov into kcov and kcov_remote. kcov_task_exit() now
> >    has to clean up both separately.
> >  - Only use task_struct::kcov_mode on the task that generates coverage.
> >  - Only reset task_struct::kcov_handle on the task that requested remote
> >    coverage.
> >
> > After this change, fields used by the task that generates coverage are:
> >
> >  - kcov_mode
> >  - kcov_size
> >  - kcov_area
> >  - kcov
> >  - kcov_sequence
> >  - kcov_softirq
> >
> > Fields used by the task that requested remote coverage are:
> >
> >  - kcov_remote
> >  - kcov_handle
>
> Thanks, I queued this for testing while we await reviewer input.
>
> checkpatch asked:
>
> WARNING: Using vsprintf specifier '%px' potentially exposes the kernel memory layout, if you don't really need the address please consider using '%p'.
> #141: FILE: kernel/kcov.c:463:
> +               kcov_debug("t = %px, kcov->t = %px\n", t, kcov->t);

Yep, I saw that when running checkpatch but decided to leave it this
way. I didn't introduce that line, I just moved it around; and other
parts of kcov use %px to identify the task as well, so changing just
this spot to %p would be inconsistent. I guess I should have noted
that below the patch...

(And kcov_debug() maps to pr_debug(), so these things will not
normally show up in the kernel log.)

> and Sashiko found a femtobug:
>         https://sashiko.dev/#/patchset/20260505-kcov-simultaneous-remote-v1-1-a670ba7cefd2@google.com

Hm. In my view, this isn't different from the previous behavior - if
the state is broken, WARN_ON() will print a warning and the function
will bail out immediately without dropping references.

I think if we hit a WARN_ON(), generally this means some state has
been corrupted and/or programmer assumptions have been broken, and it
doesn't really make sense to try to go for full correctness (dropping
references properly and such) because the system is already in a state
the programmer didn't consider. Instead, I think it's best to not
touch the broken state, and prefer leaking memory rather than
potentially making things worse by dropping references (which could
make the situation worse and cause UAF or similar).

Re: [PATCH resend] kcov: allow simultaneous KCOV_ENABLE/KCOV_REMOTE_ENABLE

[Andrew Morton]

On Mon, 11 May 2026 17:03:49 +0200 Jann Horn <jannh@google.com> wrote:

> > checkpatch asked:
> >
> > WARNING: Using vsprintf specifier '%px' potentially exposes the kernel memory layout, if you don't really need the address please consider using '%p'.
> > #141: FILE: kernel/kcov.c:463:
> > +               kcov_debug("t = %px, kcov->t = %px\n", t, kcov->t);
> 
> Yep, I saw that when running checkpatch but decided to leave it this
> way. I didn't introduce that line, I just moved it around; and other
> parts of kcov use %px to identify the task as well, so changing just
> this spot to %p would be inconsistent. I guess I should have noted
> that below the patch...
> 
> (And kcov_debug() maps to pr_debug(), so these things will not
> normally show up in the kernel log.)

yup.

> > and Sashiko found a femtobug:
> >         https://sashiko.dev/#/patchset/20260505-kcov-simultaneous-remote-v1-1-a670ba7cefd2@google.com
> 
> Hm. In my view, this isn't different from the previous behavior - if
> the state is broken, WARN_ON() will print a warning and the function
> will bail out immediately without dropping references.
> 
> I think if we hit a WARN_ON(), generally this means some state has
> been corrupted and/or programmer assumptions have been broken, and it
> doesn't really make sense to try to go for full correctness (dropping
> references properly and such) because the system is already in a state
> the programmer didn't consider. Instead, I think it's best to not
> touch the broken state, and prefer leaking memory rather than
> potentially making things worse by dropping references (which could
> make the situation worse and cause UAF or similar).

yup.

As said, femtobugs.  Things to add to the todo list somewhere...