tty: do not recurse on ttydev_close()

[kib]

Summary

When the terminal cdev is closed due to revoke, ttydev_close() destroys
t_inpoll and t_outpoll selinfos.  Since corresponding knotes reference
files pointing to the same tty cdev, it fdrop()s them.  But then the
VOP_CLOSE() call would recurse into the ttydev_close() for the same tty.
More, because the devfs vnode is already doomed, each close call gets
the FREVOKE flag set.

As result, the kernel is recursing as deep into the ttydev_close() as
there are opened files referencing the same tty, which have the knotes
installed.  Basically, the recursion level is controlled by userspace.

Prevent it by marking the tty that is handled by ttydev_close(), with
the TF_INDEVCLOSE flag.  Do nothing in ttydev_close() when the flag is
already set, avoiding recursion.

[kevans]

I'll look at adding a test for this scenario, and will take a closer look at this in a few hours

[markj]

I guess this is Fixes: acd5638e268a6?

[markj]

sys/kern/tty.c

402

Are we leaking the flag here?

[kib]

Clear TF_INDEVCLOSE in one more place.

[kevans]

Writing a test for this seems to be a little harder than I anticipated, but I'm also wondering now if deleting the knotes was infact the right thing to do. From revoke(2):

 [...].  Subsequent operations on any such
descriptors fail, with the exceptions that a read() from a character
device file which has been revoked returns a count of zero (end of file),
and a close() system call will succeed.  [...]

I think the philosophically correct thing to do would be the exact same thing we do for a pipe whose other end has been closed: surface the tty as readable (for all of select/poll/kqueue) so that the consumer can observe the EOF/revocation event and cleanup accordingly. That's kind of challenging, though, so maybe this is the least bad option.

[kib]

In D58706#1348419, @kevans wrote:

Writing a test for this seems to be a little harder than I anticipated, but I'm also wondering now if deleting the knotes was infact the right thing to do. From revoke(2):

 [...].  Subsequent operations on any such
descriptors fail, with the exceptions that a read() from a character
device file which has been revoked returns a count of zero (end of file),
and a close() system call will succeed.  [...]

I think the philosophically correct thing to do would be the exact same thing we do for a pipe whose other end has been closed: surface the tty as readable (for all of select/poll/kqueue) so that the consumer can observe the EOF/revocation event and cleanup accordingly. That's kind of challenging, though, so maybe this is the least bad option.

Yes, it would require either making struct tty type-stable, or even more complicated, refcount it from the knotes, I do not think it is worth the trouble.

[markj]

sys/kern/tty.c

234

What's the purpose of the flag parameter? Isn't it equivalent to (tp->t_flags & TF_INDEVCLOSE) != 0? Same question about tty_rel_free().

[kib]

sys/kern/tty.c

234

I do not think that it is equivalent. Consider ttydev_open(): after the tty is unlocked in ttydev_close() for fdrop() inside knlist_delete(), other thread might call ttydev_open() and observe _our_ flag.

Similarly, tty_rel_session() and other callers of tty_rel_free() might call while we ttydev_close() dropped the tty lock, and these calls must not clear our flag.

[rew]

In D58706#1348195, @markj wrote:

I guess this is Fixes: acd5638e268a6?

it's not clear to me off-hand but how did acd5638e268a6 cause the recursion behavior that this review fixes?

hindsight, I probably should have bisected the commit that acd5638e268a6 fixed

[kib]

In D58706#1349924, @rew wrote:

In D58706#1348195, @markj wrote:

I guess this is Fixes: acd5638e268a6?

it's not clear to me off-hand but how did acd5638e268a6 cause the recursion behavior that this review fixes?

hindsight, I probably should have bisected the commit that acd5638e268a6 fixed

knlist_delete() calls fddrop() for each knote on the list, which might recurse into ttydev_close() if the fd was the last reference on the different file opened on the same tty. The commit message provides more elaborate explanation.