From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sysctl(2), video(4): unlock sysctl_video()
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: kirill@openbsd.org, tech@openbsd.org
Date: Mon, 16 Dec 2024 21:56:26 +0100

On Sun, Dec 15, 2024 at 11:47:20PM GMT, Vitaliy Makkoveev wrote:

> This sysctl(2) path contains only `video_record_enable', which is
> atomically accessed boolean integer.

ok mglocker@
 
> Index: sys/dev/video.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/video.c,v
> diff -u -p -r1.58 video.c
> --- sys/dev/video.c	15 Dec 2024 18:23:56 -0000	1.58
> +++ sys/dev/video.c	15 Dec 2024 20:28:01 -0000
> @@ -38,6 +38,7 @@
>  
>  /*
>   * Locks used to protect struct members and global data
> + *	a	atomic
>   *	m	sc_mtx
>   */
>  
> @@ -93,7 +94,7 @@ struct cfdriver video_cd = {
>  /*
>   * Global flag to control if video recording is enabled by kern.video.record.
>   */
> -int video_record_enable = 0;
> +int video_record_enable = 0;	/* [a] */
>  
>  int
>  videoprobe(struct device *parent, void *match, void *aux)
> @@ -234,7 +235,7 @@ videoread(dev_t dev, struct uio *uio, in
>  
>  	/* move no more than 1 frame to userland, as per specification */
>  	size = ulmin(uio->uio_resid, sc->sc_fsize);
> -	if (!video_record_enable)
> +	if (!atomic_load_int(&video_record_enable))
>  		bzero(sc->sc_fbuffer, size);
>  	error = uiomove(sc->sc_fbuffer, size, uio);
>  	if (error)
> @@ -371,7 +372,7 @@ videoioctl(dev_t dev, u_long cmd, caddr_
>  		}
>  		error = (sc->hw_if->dqbuf)(sc->hw_hdl,
>  		    (struct v4l2_buffer *)data);
> -		if (!video_record_enable)
> +		if (!atomic_load_int(&video_record_enable))
>  			bzero(sc->sc_fbuffer_mmap + vb->m.offset, vb->length);
>  		mtx_enter(&sc->sc_mtx);
>  		sc->sc_frames_ready--;
> Index: sys/kern/kern_sysctl.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> diff -u -p -r1.457 kern_sysctl.c
> --- sys/kern/kern_sysctl.c	15 Dec 2024 18:25:12 -0000	1.457
> +++ sys/kern/kern_sysctl.c	15 Dec 2024 20:28:01 -0000
> @@ -161,6 +161,10 @@ void fill_kproc(struct process *, struct
>  
>  int kern_sysctl_locked(int *, u_int, void *, size_t *, void *, size_t,
>  	struct proc *);
> +int kern_sysctl_dirs(int, int *, u_int, void *, size_t *, void *,
> +	size_t, struct proc *);
> +int kern_sysctl_dirs_locked(int, int *, u_int, void *, size_t *, void *,
> +	size_t, struct proc *);
>  int hw_sysctl_locked(int *, u_int, void *, size_t *,void *, size_t,
>  	struct proc *);
>  
> @@ -395,6 +399,38 @@ int
>  kern_sysctl_dirs(int top_name, int *name, u_int namelen,
>      void *oldp, size_t *oldlenp, void *newp, size_t newlen, struct proc *p)
>  {
> +	size_t savelen;
> +	int error;
> +
> +	switch (top_name) {
> +#if NAUDIO > 0
> +	case KERN_AUDIO:
> +		return (sysctl_audio(name, namelen, oldp, oldlenp,
> +		    newp, newlen));
> +#endif
> +#if NVIDEO > 0
> +	case KERN_VIDEO:
> +		return (sysctl_video(name, namelen, oldp, oldlenp,
> +		    newp, newlen));
> +#endif
> +	default:
> +		break;
> +	}
> +
> +	savelen = *oldlenp;
> +	if ((error = sysctl_vslock(oldp, savelen)))
> +		return (error);
> +	error = kern_sysctl_dirs_locked(top_name, name, namelen,
> +	    oldp, oldlenp, newp, newlen, p);
> +	sysctl_vsunlock(oldp, savelen);
> +
> +	return (error);
> +}
> +
> +int
> +kern_sysctl_dirs_locked(int top_name, int *name, u_int namelen,
> +    void *oldp, size_t *oldlenp, void *newp, size_t newlen, struct proc *p)
> +{
>  	switch (top_name) {
>  #ifndef SMALL_KERNEL
>  	case KERN_PROC:
> @@ -462,11 +498,6 @@ kern_sysctl_dirs(int top_name, int *name
>  		return witness_sysctl(name, namelen, oldp, oldlenp,
>  		    newp, newlen);
>  #endif
> -#if NVIDEO > 0
> -	case KERN_VIDEO:
> -		return (sysctl_video(name, namelen, oldp, oldlenp,
> -		    newp, newlen));
> -#endif
>  	case KERN_CPUSTATS:
>  		return (sysctl_cpustats(name, namelen, oldp, oldlenp,
>  		    newp, newlen));
> @@ -489,25 +520,9 @@ kern_sysctl(int *name, u_int namelen, vo
>  	size_t savelen;
>  
>  	/* dispatch the non-terminal nodes first */
> -	if (namelen != 1) {
> -		switch (name[0]) {
> -#if NAUDIO > 0
> -		case KERN_AUDIO:
> -			return (sysctl_audio(name + 1, namelen - 1,
> -			    oldp, oldlenp, newp, newlen));
> -#endif
> -		default:
> -			break;
> -		}
> -
> -		savelen = *oldlenp;
> -		if ((error = sysctl_vslock(oldp, savelen)))
> -			return (error);
> -		error = kern_sysctl_dirs(name[0], name + 1, namelen - 1,
> -		    oldp, oldlenp, newp, newlen, p);
> -		sysctl_vsunlock(oldp, savelen);
> -		return (error);
> -	}
> +	if (namelen != 1)
> +		return (kern_sysctl_dirs(name[0], name + 1, namelen - 1,
> +		    oldp, oldlenp, newp, newlen, p));
>  
>  	switch (name[0]) {
>  	case KERN_ALLOWKMEM:

-- 

[ -- Marcus Glocker, marcus@nazgul.ch, https://nazgul.ch ------------- ]