From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: ktrace vs KERNEL_LOCK
To: Martin Pieuchot <mpi@grenadille.net>
Cc: tech@openbsd.org
Date: Wed, 25 Dec 2024 12:52:14 +0100

> Date: Tue, 24 Dec 2024 12:21:24 +0100
> From: Martin Pieuchot <mpi@grenadille.net>
> 
> Diff below is extract from my previous VFS syscalls unlocking diff.  It
> ensures the KERNEL_LOCK() is taken around all ktrwrite{,2}() calls.
> 
> This should be enough to unlock remaining syscalls without having to
> care about ktrace bits.

Reduces the scope of the kernel lock a bit as well.

> ok?

A few minor style issues below.  Either way,

ok kettenis@

> Index: kern/kern_ktrace.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_ktrace.c,v
> diff -u -p -r1.114 kern_ktrace.c
> --- kern/kern_ktrace.c	15 Dec 2023 15:12:08 -0000	1.114
> +++ kern/kern_ktrace.c	24 Dec 2024 11:14:56 -0000
> @@ -179,7 +179,9 @@ ktrsyscall(struct proc *p, register_t co
>  		*argp++ = args[i];
>  	if (nargs && copyin((void *)args[0], argp, nargs * sizeof(int)))
>  		memset(argp, 0, nargs * sizeof(int));
> +	KERNEL_LOCK();
>  	ktrwrite(p, &kth, ktp, len);
> +	KERNEL_UNLOCK();
>  	free(ktp, M_TEMP, len);
>  	atomic_clearbits_int(&p->p_flag, P_INKTR);
>  }
> @@ -203,7 +205,9 @@ ktrsysret(struct proc *p, register_t cod
>  		len = sizeof(long long);
>  	else
>  		len = sizeof(register_t);
> +	KERNEL_LOCK();
>  	ktrwrite2(p, &kth, &ktp, sizeof(ktp), retval, len);
> +	KERNEL_UNLOCK();
>  	atomic_clearbits_int(&p->p_flag, P_INKTR);
>  }
>  
> @@ -214,7 +218,9 @@ ktrnamei(struct proc *p, char *path)
>  
>  	atomic_setbits_int(&p->p_flag, P_INKTR);
>  	ktrinitheader(&kth, p, KTR_NAMEI);
> +	KERNEL_LOCK();
>  	ktrwrite(p, &kth, path, strlen(path));
> +	KERNEL_UNLOCK();
>  	atomic_clearbits_int(&p->p_flag, P_INKTR);
>  }
>  
> @@ -351,7 +357,7 @@ void
>  ktrexec(struct proc *p, int type, const char *data, ssize_t len)
>  {
>  	struct ktr_header kth;
> -	int count;
> +	int count, error;
>  	int buflen;
>  
>  	assert(type == KTR_EXECARGS || type == KTR_EXECENV);
> @@ -373,7 +379,10 @@ ktrexec(struct proc *p, int type, const 
>  		sched_pause(preempt);
>  
>  		count = lmin(len, buflen);
> -		if (ktrwrite(p, &kth, data, count) != 0)
> +		KERNEL_LOCK();
> +		error = ktrwrite(p, &kth, data, count);
> +		KERNEL_UNLOCK();
> +		if (error != 0)

The code in this file is already inconsistent but I think:

                if (error)

is more expressive.

>  			break;
>  
>  		len -= count;
> Index: sys//syscall_mi.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/syscall_mi.h,v
> diff -u -p -r1.36 syscall_mi.h
> --- sys//syscall_mi.h	29 Oct 2024 12:40:17 -0000	1.36
> +++ sys//syscall_mi.h	24 Dec 2024 11:13:18 -0000
> @@ -151,9 +151,7 @@ mi_syscall(struct proc *p, register_t co
>  #ifdef KTRACE
>  	if (KTRPOINT(p, KTR_SYSCALL)) {
>  		/* convert to mask, then include with code */
> -		KERNEL_LOCK();
>  		ktrsyscall(p, code, callp->sy_argsize, argp);
> -		KERNEL_UNLOCK();
>  	}
>  #endif
>  
> @@ -203,9 +201,7 @@ mi_syscall_return(struct proc *p, regist
>  
>  #ifdef KTRACE
>  	if (KTRPOINT(p, KTR_SYSRET)) {
> -		KERNEL_LOCK();
>  		ktrsysret(p, code, error, retval);
> -		KERNEL_UNLOCK();
>  	}

You can drop the curly braces here.

>  #endif
>  }
> @@ -238,9 +234,7 @@ mi_child_return(struct proc *p)
>  
>  #ifdef KTRACE
>  	if (KTRPOINT(p, KTR_SYSRET)) {
> -		KERNEL_LOCK();
>  		ktrsysret(p, code, 0, child_retval);
> -		KERNEL_UNLOCK();
>  	}

And here.

>  #endif
>  }
> 
> 
>