kcov: move thread attach from open to start

The current kcov code attaches a kcov object to the thread which opened
it. This is not desirable, as syzkaller opens /dev/kcov in a parent
process and passes it down to the child.

Move the thread attachment from open() time to start time.

kernel-src/include/kernel/kcov.h

@@ -22,7 +22,7 @@
 #define KCOV_CMP_FLAGS_64BIT 6
 
 typedef struct {
-	unsigned int mode;
+	unsigned int mode; // transitions protected by mutex
 	mutex_t mapping_mutex;
 	unsigned int entry_count; // protected by mutex
 	uint64_t *buffer; // pointer protected by mutex
@@ -34,8 +34,8 @@ typedef struct {
 
 void kcov_publish_pc(uint64_t pc);
 void kcov_publish_cmp(uint64_t pc, uint64_t v1, uint64_t v2, uint64_t flags);
-int kcov_set_buffer_size(unsigned int entry_count);
+int kcov_set_buffer_size(kcov_t *kcov, unsigned int entry_count);
-int kcov_set_mode(unsigned int mode);
+int kcov_set_mode(kcov_t *kcov, unsigned int mode);
 void kcov_hold(kcov_t *kcov);
 void kcov_release(kcov_t *kcov);
 void kcov_free_buffer(kcov_t *kcov);

kernel-src/sys/kcov.c

@@ -69,8 +69,7 @@ KCOV_DISABLED static int resize_buffer(kcov_t *kcov, unsigned int entry_count) {
 	return 0;
 }
 
-KCOV_DISABLED int kcov_set_mode(unsigned int mode) {
+KCOV_DISABLED int kcov_set_mode(kcov_t *kcov, unsigned int mode) {
-	kcov_t *kcov = current_thread()->kcov;
 	__assert(kcov);
 
 	if (mode != KCOV_MODE_DISABLED &&
@@ -78,26 +77,44 @@ KCOV_DISABLED int kcov_set_mode(unsigned int mode) {
 	    mode != KCOV_MODE_CMP)
 		return EINVAL;
 
-	if (mode == KCOV_MODE_DISABLED && kcov->mode == KCOV_MODE_DISABLED)
+	MUTEX_ACQUIRE(&kcov->mapping_mutex);
-		return EINVAL;
+	int error = 0;
+	if (mode == KCOV_MODE_DISABLED) {
+		if (current_thread()->kcov != kcov) {
+			error = EINVAL;
+			goto cleanup;
+		}
 
-	if (mode != KCOV_MODE_DISABLED && kcov->mode != KCOV_MODE_DISABLED)
+		current_thread()->kcov = NULL;
-		return EINVAL;
+	} else {
+		if (current_thread()->kcov || kcov->mode != KCOV_MODE_DISABLED) {
+			error = EBUSY;
+			goto cleanup;
+		}
 
-	if (kcov->buffer == NULL)
+		if (kcov->buffer == NULL) {
-		return EINVAL;
+			error = EINVAL;
+			goto cleanup;
+		}
+
+		kcov_hold(kcov);
+	}
 
 	kcov->mode = mode;
-	return 0;
+	if (mode != KCOV_MODE_DISABLED)
+		current_thread()->kcov = kcov;
+
+	cleanup:
+	MUTEX_RELEASE(&kcov->mapping_mutex);
+	if (error == 0 && mode == KCOV_MODE_DISABLED)
+		kcov_release(kcov);
+	return error;
 }
 
-KCOV_DISABLED int kcov_set_buffer_size(unsigned int entry_count) {
+KCOV_DISABLED int kcov_set_buffer_size(kcov_t *kcov, unsigned int entry_count) {
 	if (entry_count < 0 || entry_count > MAX_ENTRIES)
 		return EINVAL;
 
-	kcov_t *kcov = current_thread()->kcov;
-	__assert(kcov);
-
 	MUTEX_ACQUIRE(&kcov->mapping_mutex);
 	int error = 0;
 	if (kcov->mappings || kcov->mode != KCOV_MODE_DISABLED) {

kernel-src/sys/kcov_dev.c

@@ -89,10 +89,6 @@ static int create_node_for_kcov(kcov_t *kcov, int minor, vnode_t **vnodep) {
 	kcov->vnode_handle = vnode;
 	devfs_remove_anonymous(DEV_MAJOR_KCOV, minor);
 
-	// a kcov will have two references initially:
-	// - one for the thread holding it
-	// - one for the vnode
-	kcov_hold(kcov);
 	*vnodep = vnode;
 	return 0;
 }
@@ -103,18 +99,6 @@ static int open(int oldminor, vnode_t **vnodep, int flags) {
 	if (oldminor != KCOV_MAX)
 		return ENODEV;
 
-	// if the thread already has a kcov attached, return it
-	if (current_thread()->kcov) {
-		kcov_t *kcov = current_thread()->kcov;
-
-		int error = create_node_for_kcov(kcov, kcov->minor, vnodep);
-		if (error)
-			return error;
-
-		VOP_RELEASE(old);
-		return 0;
-	}
-
 	int minor;
 	int error = allocate_kcov(&minor);
 	if (error)
@@ -127,7 +111,6 @@ static int open(int oldminor, vnode_t **vnodep, int flags) {
 		return error;
 	}
 
-	current_thread()->kcov = kcov;
 	VOP_RELEASE(old);
 	return 0;
 }
@@ -137,8 +120,8 @@ static int open(int oldminor, vnode_t **vnodep, int flags) {
 #define IOCTL_REQUEST_SET_SIZE 0xF1177200
 
 static int ioctl(int minor, unsigned long request, void *arg, int *result, cred_t *cred) {
-	kcov_t *kcov = current_thread()->kcov;
+	kcov_t *kcov = get_kcov(minor);
-	if (kcov == NULL || kcov->minor != minor)
+	if (kcov == NULL)
 		return EINVAL;
 
 	switch (request) {
@@ -148,24 +131,21 @@ static int ioctl(int minor, unsigned long request, void *arg, int *result, cred_
 			if (error)
 				return error;
 
-			return kcov_set_buffer_size(size);
+			return kcov_set_buffer_size(kcov, size);
 		}
 		case IOCTL_REQUEST_START: {
-			if (kcov->mode != KCOV_MODE_DISABLED)
-				return EBUSY;
-
-			if (kcov->buffer == NULL)
-				return EINVAL;
-
 			int mode;
 			int error = USERCOPY_POSSIBLY_FROM_USER(&mode, arg, sizeof(mode));
 			if (error)
 				return error;
 
-			return kcov_set_mode(mode);
+			if (mode != KCOV_MODE_PC && mode != KCOV_MODE_CMP)
+				return EINVAL;
+
+			return kcov_set_mode(kcov, mode);
 		}
 		case IOCTL_REQUEST_STOP:
-			return kcov_set_mode(KCOV_MODE_DISABLED);
+			return kcov_set_mode(kcov, KCOV_MODE_DISABLED);
 		default:
 			return ENOTTY;
 	}

kernel-src/sys/thread.c

@@ -57,10 +57,6 @@ thread_t *sched_newthread(void *ip, size_t kstacksize, int nice, proc_t *proc, v
 }
 
 void sched_destroythread(thread_t *thread) {
-#ifdef KCOV_ENABLED
-	if (thread->kcov)
-		kcov_release(thread->kcov);
-#endif
 	mm_unmap(thread->kernelstack, thread->kernelstacksize, 0);
 	arch_extracontext_free(&thread->extracontext);
 	slab_free(thread_cache, thread);
@@ -88,6 +84,11 @@ __attribute__((noreturn)) void sched_threadexit() {
 	thread_t *thread = current_thread();
 	proc_t *proc = thread->proc;
 
+#ifdef KCOV_ENABLED
+	if (thread->kcov)
+		kcov_set_mode(thread->kcov, KCOV_MODE_DISABLED);
+#endif
+
 	mm_context_t *oldctx = thread->mmctx;
 	mm_switch_context(&mm_kernel_ctx);
 
@@ -105,11 +106,6 @@ __attribute__((noreturn)) void sched_threadexit() {
 		PROC_RELEASE(proc);
 	}
 
-#ifdef KCOV_ENABLED
-	if (current_thread()->kcov)
-		kcov_set_mode(KCOV_MODE_DISABLED);
-#endif
-
 	interrupt_set(false);
 	arch_context_saveandcall(threadexit_internal, current_cpu()->schedulerstack, NULL);
 	__builtin_unreachable();