Re: [LOR] kqueue()

Craig Rodrigues wrote this message on Sun, Jul 17, 2005 at 12:59 -0400:
> On Sun, Jul 17, 2005 at 11:39:36AM -0400, Craig Rodrigues wrote:
> > On Sat, Jul 16, 2005 at 10:44:43PM +0000, Wojciech A. Koszek wrote:
> > > http://FreeBSD.czest.pl/dunstan/kqueuetest.c
> >
> > I can reproduce this LOR:
> >
> > It looks like on line 1545 of kern_event.c, we have a KQ_LOCK(kq);,
> > and then on line 1039, the KQ_NOTOWNED(kq); assertion fails, because
> > the lock is never released.
>
> Can you try this patch:
>
> --- /usr/src/sys/kern/kern_event.c.orig	Sun Jul 17 12:32:58 2005
> +++ /usr/src/sys/kern/kern_event.c	Sun Jul 17 12:41:54 2005
> _at__at_ -410,7 +410,15 _at__at_
>  		kev.fflags = kn->kn_sfflags;
>  		kev.data = kn->kn_id;			/* parent */
>  		kev.udata = kn->kn_kevent.udata;	/* preserve udata */
> +
> +		if (kn->kn_status & KN_HASKQLOCK)
> +			KQ_UNLOCK(kn->kn_kq);
> +
>  		error = kqueue_register(kn->kn_kq, &kev, NULL, 0);
> +
> +		if (kn->kn_status & KN_HASKQLOCK)
> +			KQ_LOCK(kn->kn_kq);
> +
>  		if (error)
>  			kn->kn_fflags |= NOTE_TRACKERR;
>  	}

This is not a correct fix..  This will cause the error message to go
away, but will open a race...  You can only unlock the kq while holding
a reference to the knote when the knote is INFLUX....  If it's not
INFLUX, the knote could disappear out from under you by another thread
deleting it and you'd have a pointer to stale memory....

It maybe possible to exploit the fact that the knote has the KN_HASKQLOCK
set in it's kn_status, but looking at kqueue_register, this would be
difficult, since we might sleep and need to unlock the kqueue...

the most correct solution would be to attach a work item to the process
and then have another thread (possibly even the kqueue thread) process
the event add request out of band where we won't have a problem with the
locks...  The only issue with this is that there may not be the ability
to immediately note the error if the knote is unable to be attached....

> I don't get the crash any more, but sometimes I get this LOR
> in dmesg:
>
> lock order reversal
>  1st 0xc1b0aaa4 process lock (process lock) _at_ /usr/src/sys/kern/kern_fork.c:690
>  2nd 0xc092dba0 allproc (allproc) _at_ /usr/src/sys/kern/kern_proc.c:229

Which is another race opened by the patch that can get you into trouble...

--
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."