On Tue, Feb 04, 2020 at 01:04:24PM +0100, Martin Pieuchot wrote:
> On 02/02/20(Sun) 09:58, Visa Hankala wrote:
> > tty(4) uses custom code for revoking knotes. It should be changed to use
> > klist_invalidate() to handle the revocation in one place. The following
> > diff does that.
>
> Great, your diff is ok mpi@
>
> > In addition, the patch makes the code store the tty context pointer
> > in kn_hook directly. This simplifies the code.
>
> Does that mean we'll need some reference counting when getting kqueue
> out of the KERNEL_LOCK()? Who is the owner of `kn_hook'?
I think the proper way is to invalidate knotes before the associated
event source object or kqueue is destroyed. That way there is no need
for reference counting. klist_invalidate() does the job when the object
is destroyed kind of involuntarily, like when a device is detached.
Typically, knotes are freed by knote_fdclose() when the file descriptor
is closed, which happens before the object is freed. kqueue_close()
takes care of freeing kqueue's registered knotes when the kqueue itself
is being freed.
I would say the main owner of kn_hook and the knote itself is the object
that owns the kn_selnext list. klist_invalidate() dissolves that
ownership (well, any knotes on the list get freed either when the EOF
event is delivered to userspace or when the kqueue is destroyed).
> > Unlike ttkqflush(), klist_invalidate() can sleep. However, that should
> > not be a problem because the callers of ttyfree() call vdevgone() that
> > can sleep as well.
>
> It took me some time to audit com_detach() because ttyfree() isn't the
> last call. Most of the *_detach() functions have similar problems: they
> assume the execution of the code is atomic and free data structures
> before unregistering handlers.
>
> Diff below improves that, ok?
Right, at least the soft interrupt handler dereferences sc_tty. The
code assumes that softintr_disestablish() has barrier-like behaviour,
which should be the case in this context on most architectures because
of the kernel lock.
OK visa@
> Index: dev/ic/com.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/ic/com.c,v
> retrieving revision 1.170
> diff -u -p -r1.170 com.c
> --- dev/ic/com.c 19 Jul 2019 00:17:15 -0000 1.170
> +++ dev/ic/com.c 4 Feb 2020 11:27:06 -0000
> @@ -186,14 +186,14 @@ com_detach(struct device *self, int flag
> mn |= 0x80;
> vdevgone(maj, mn, mn, VCHR);
>
> + timeout_del(&sc->sc_dtr_tmo);
> + timeout_del(&sc->sc_diag_tmo);
> + softintr_disestablish(sc->sc_si);
> +
> /* Detach and free the tty. */
> if (sc->sc_tty) {
> ttyfree(sc->sc_tty);
> }
> -
> - timeout_del(&sc->sc_dtr_tmo);
> - timeout_del(&sc->sc_diag_tmo);
> - softintr_disestablish(sc->sc_si);
>
> return (0);
> }
>
