From: Volker Schlecht Subject: Re: kqueue: EVFILT_USER fixes To: Visa Hankala , tech@openbsd.org, mvs@openbsd.org Date: Tue, 20 May 2025 18:28:58 +0200 As already stated to visa@ off-list: This solves the problem I saw with an experimental update to the latest version of devel/libinotify and while I don't think I should be ok'ing kernel patches, I can confirm that it solves a current problem, and hope that the fix gets in :-) On 5/19/25 4:55 PM, Visa Hankala wrote: > 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++) { >