On Sun, May 18, 2025 at 09:27:12PM +0300, Vitaliy Makkoveev wrote:
> 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;
> 		}

I think clearing kn_data and kn_fflags has no effect visible
to userland. They are aliases to kn->kn_kevent.data and
kn->kn_kevent.fflags. In the EVENT_PROCESS case above, where the user
event is prepared for copyout to userland, the values of these fields
in *kev are overridden using kn->kn_sfflags and kn->kn_sdata.

As for EV_CLEAR, the knote should preserve the userland-visible value
of `fflags'. Zeroing `data' is not necessary because it gets set again
anyway when the user event is re-activated, and userland cannot read
`data' before the event is active.

After mulling over the details some more, I think it is clearer to keep
using kn_data and kn_flags in filt_user{attach,modify,process}()
after all. This possibly avoids the confusion with kn_{fflags,data}
versus kn_s{fflags,data}. The diff becomes shorter too.

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	19 May 2025 14:23:10 -0000
@@ -850,16 +850,12 @@ filt_usermodify(struct kevent *kev, stru
 		break;
 	}
 
-	if (kev->flags & EV_ADD) {
-		kn->kn_data = kev->data;
-		kn->kn_udata = kev->udata;
-	}
+	kn->kn_data = 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 +868,8 @@ 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) {
+		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	19 May 2025 14:23:10 -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++) {