From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: kqueue: EVFILT_USER fixes
To: Visa Hankala <visa@hankala.org>
Cc: tech@openbsd.org
Date: Sun, 18 May 2025 21:27:12 +0300

On Sun, May 18, 2025 at 05:21:14PM +0000, Visa Hankala wrote:
> The EVFILT_USER implementation does not quite align with FreeBSD.
> One difference is that FreeBSD updates the `data' and `udata' fields
> when a user event is triggered. FreeBSD also preserves `fflags' when
> the event is "untriggered". The diff below fixes these.
> 
> In addition, use kn_sfflags and kn_sdata instead of kn_fflags and
> kn_data to make the implementation a bit more similar to FreeBSD.
> 
> OK?
> 

FreeBSD and NetBSD clean `kn_data' and `kn_fflag' if EV_CLEAR flag is
set. Shouldn't you do the same?

        case EVENT_REGISTER:
		kn->kn_sdata = kev->data;
		if (kev->flags & EV_CLEAR) {
			kn->kn_hookid = 0;
			kn->kn_data = 0;
			kn->kn_fflags = 0;
		}
		break;

        case EVENT_PROCESS:
		*kev = kn->kn_kevent;
		kev->fflags = kn->kn_sfflags;
		kev->data = kn->kn_sdata;
		if (kn->kn_flags & EV_CLEAR) {
			kn->kn_hookid = 0;
			kn->kn_data = 0;
			kn->kn_fflags = 0;
		}


> Index: sys/kern/kern_event.c
> ===================================================================
> RCS file: src/sys/kern/kern_event.c,v
> retrieving revision 1.204
> diff -u -p -r1.204 kern_event.c
> --- sys/kern/kern_event.c	15 May 2025 19:56:08 -0000	1.204
> +++ sys/kern/kern_event.c	18 May 2025 17:17:04 -0000
> @@ -809,8 +809,6 @@ filt_userattach(struct knote *kn)
>  	}
>  
>  	kn->kn_ptr.p_useract = ((kn->kn_sfflags & NOTE_TRIGGER) != 0);
> -	kn->kn_fflags = kn->kn_sfflags & NOTE_FFLAGSMASK;
> -	kn->kn_data = kn->kn_sdata;
>  
>  	return (0);
>  }
> @@ -837,29 +835,25 @@ filt_usermodify(struct kevent *kev, stru
>  	case NOTE_FFNOP:
>  		break;
>  	case NOTE_FFAND:
> -		kn->kn_fflags &= fflags;
> +		kn->kn_sfflags &= fflags;
>  		break;
>  	case NOTE_FFOR:
> -		kn->kn_fflags |= fflags;
> +		kn->kn_sfflags |= fflags;
>  		break;
>  	case NOTE_FFCOPY:
> -		kn->kn_fflags = fflags;
> +		kn->kn_sfflags = fflags;
>  		break;
>  	default:
>  		/* ignored, should not happen */
>  		break;
>  	}
>  
> -	if (kev->flags & EV_ADD) {
> -		kn->kn_data = kev->data;
> -		kn->kn_udata = kev->udata;
> -	}
> +	kn->kn_sdata = kev->data;
> +	kn->kn_udata = kev->udata;
>  
>  	/* Allow clearing of an activated event. */
> -	if (kev->flags & EV_CLEAR) {
> +	if (kev->flags & EV_CLEAR)
>  		kn->kn_ptr.p_useract = 0;
> -		kn->kn_data = 0;
> -	}
>  
>  	return (kn->kn_ptr.p_useract);
>  }
> @@ -872,11 +866,10 @@ filt_userprocess(struct knote *kn, struc
>  	active = kn->kn_ptr.p_useract;
>  	if (active && kev != NULL) {
>  		*kev = kn->kn_kevent;
> -		if (kn->kn_flags & EV_CLEAR) {
> +		kev->fflags = kn->kn_sfflags & NOTE_FFLAGSMASK;
> +		kev->data = kn->kn_sdata;
> +		if (kn->kn_flags & EV_CLEAR)
>  			kn->kn_ptr.p_useract = 0;
> -			kn->kn_fflags = 0;
> -			kn->kn_data = 0;
> -		}
>  	}
>  
>  	return (active);
> Index: regress/sys/kern/kqueue/kqueue-user.c
> ===================================================================
> RCS file: src/regress/sys/kern/kqueue/kqueue-user.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 kqueue-user.c
> --- regress/sys/kern/kqueue/kqueue-user.c	10 May 2025 09:44:39 -0000	1.1
> +++ regress/sys/kern/kqueue/kqueue-user.c	18 May 2025 17:17:04 -0000
> @@ -44,10 +44,7 @@ do_user(void)
>  	n = kevent(kq, NULL, 0, kev, 2, &ts);
>  	ASSX(n == 0);
>  
> -	/*
> -	 * Activate the event.
> -	 * Fields `data' and `udata' do not get updated without EV_ADD.
> -	 */
> +	/* Activate the event. This updates `data' and `udata'. */
>  	EV_SET(&kev[0], 1, EVFILT_USER, 0, NOTE_TRIGGER | NOTE_FFNOP,
>  	    123, &dummy);
>  	n = kevent(kq, kev, 1, NULL, 0, NULL);
> @@ -58,20 +55,6 @@ do_user(void)
>  	ASSX(n == 1);
>  	ASSX(kev[0].ident == 1);
>  	ASSX(kev[0].fflags == NOTE_FFLAGSMASK);
> -	ASSX(kev[0].data == 0);
> -	ASSX(kev[0].udata == NULL);
> -
> -	/* Activate the event. Update `data' and `udata'. */
> -	EV_SET(&kev[0], 1, EVFILT_USER, EV_ADD, NOTE_TRIGGER | NOTE_FFNOP,
> -	    123, &dummy);
> -	n = kevent(kq, kev, 1, NULL, 0, NULL);
> -	ASSX(n == 0);
> -
> -	/* Check active events. */
> -	n = kevent(kq, NULL, 0, kev, 2, &ts);
> -	ASSX(n == 1);
> -	ASSX(kev[0].ident == 1);
> -	ASSX(kev[0].fflags == NOTE_FFLAGSMASK);
>  	ASSX(kev[0].data == 123);
>  	ASSX(kev[0].udata == &dummy);
>  
> @@ -122,8 +105,8 @@ do_user(void)
>  	ASSX(n == 1);
>  	ASSX(kev[0].ident == 2);
>  	ASSX(kev[0].fflags == 0x11);
> -	ASSX(kev[0].data == 42);
> -	ASSX(kev[0].udata == &dummy);
> +	ASSX(kev[0].data == 24);
> +	ASSX(kev[0].udata == &dummy2);
>  
>  	n = kevent(kq, NULL, 0, kev, 2, &ts);
>  	ASSX(n == 0);
> @@ -132,9 +115,9 @@ do_user(void)
>  	n = kevent(kq, kev, 1, kev, 2, &ts);
>  	ASSX(n == 1);
>  	ASSX(kev[0].ident == 2);
> -	ASSX(kev[0].fflags == 0);
> -	ASSX(kev[0].data == 0);
> -	ASSX(kev[0].udata == &dummy);
> +	ASSX(kev[0].fflags == 0x11);
> +	ASSX(kev[0].data == 9);
> +	ASSX(kev[0].udata == &dummy2);
>  
>  	EV_SET(&kev[0], 2, EVFILT_USER, EV_DELETE, 0, 0, NULL);
>  	n = kevent(kq, kev, 1, kev, 2, &ts);
> @@ -154,7 +137,7 @@ do_user(void)
>  	ASSX(kev[0].ident == 1);
>  	ASSX(kev[0].fflags == 0x0faaf0);
>  	ASSX(kev[0].data == 0);
> -	ASSX(kev[0].udata == &dummy);
> +	ASSX(kev[0].udata == NULL);
>  
>  	/* Test event limit. */
>  	for (i = 0;; i++) {
>