tty: add TIOCVHANGUP to allow clean tty shutdown of all ttys

PATCH In review Kay Sievers 4 participants 7 messages Feb 17, 2011 – Feb 23, 2011

Subsystems (10)

VFS PowerPC MIPS TTY layer and serial drivers +6 more subsystems

lore

Share & tools

lore

Kay Sievers

author

2011-02-17 17:41 UTC #0 lore raw

PATCH 1/1 tty: add TIOCVHANGUP to allow clean tty shutdown of all ttys

From: Kay Sievers <[email protected]>

tty: add TIOCVHANGUP to allow clean tty shutdown of all ttys

This is useful for system management software so that it can kick off things like gettys and everything that's started from a tty, before we reuse it from/for something else or shut it down.

Without this ioctl it would have to temporarily become the owner of the tty, then call vhangup() and then give it up again.

Cc Lennart Poettering <[email protected]>

Signed-off-by Kay Sievers <[email protected]>

---

 arch/alpha/include/asm/ioctls.h   |    1 +
 arch/mips/include/asm/ioctls.h    |    1 +
 arch/parisc/include/asm/ioctls.h  |    1 +
 arch/powerpc/include/asm/ioctls.h |    1 +
 arch/sh/include/asm/ioctls.h      |    1 +
 arch/sparc/include/asm/ioctls.h   |    1 +
 arch/xtensa/include/asm/ioctls.h  |    1 +
 drivers/tty/tty_io.c              |    5 +++++
 include/asm-generic/ioctls.h      |    1 +
 9 files changed, 13 insertions(+)

arch/alpha/include/asm/ioctls.h +1 -0

index 034b6cf..80e1cee 100644
@@ -94,6 +94,7 @@
 #define TIOCSPTLCK	_IOW('T',0x31, int)  /* Lock/unlock Pty */
 #define TIOCGDEV	_IOR('T',0x32, unsigned int) /* Get primary device node of /dev/console */
 #define TIOCSIG		_IOW('T',0x36, int)  /* Generate signal on Pty slave */
+#define TIOCVHANGUP	0x5437
 
 #define TIOCSERCONFIG	0x5453
 #define TIOCSERGWILD	0x5454

arch/mips/include/asm/ioctls.h +1 -0

index d967b89..92403c3 100644
@@ -85,6 +85,7 @@
 #define TIOCSPTLCK	_IOW('T', 0x31, int)  /* Lock/unlock Pty */
 #define TIOCGDEV	_IOR('T', 0x32, unsigned int) /* Get primary device node of /dev/console */
 #define TIOCSIG		_IOW('T', 0x36, int)  /* Generate signal on Pty slave */
+#define TIOCVHANGUP	0x5437
 
 /* I hope the range from 0x5480 on is free ... */
 #define TIOCSCTTY	0x5480		/* become controlling tty */

arch/parisc/include/asm/ioctls.h +1 -0

index 6ba80d0..054ec06 100644
@@ -54,6 +54,7 @@
 #define TIOCSPTLCK	_IOW('T',0x31, int)  /* Lock/unlock Pty */
 #define TIOCGDEV	_IOR('T',0x32, int)  /* Get primary device node of /dev/console */
 #define TIOCSIG		_IOW('T',0x36, int)  /* Generate signal on Pty slave */
+#define TIOCVHANGUP	0x5437
 
 #define FIONCLEX	0x5450  /* these numbers need to be adjusted. */
 #define FIOCLEX		0x5451

arch/powerpc/include/asm/ioctls.h +1 -0

index c7dc17c..e9b7887 100644
@@ -96,6 +96,7 @@
 #define TIOCSPTLCK	_IOW('T',0x31, int)  /* Lock/unlock Pty */
 #define TIOCGDEV	_IOR('T',0x32, unsigned int) /* Get primary device node of /dev/console */
 #define TIOCSIG		_IOW('T',0x36, int)  /* Generate signal on Pty slave */
+#define TIOCVHANGUP	0x5437
 
 #define TIOCSERCONFIG	0x5453
 #define TIOCSERGWILD	0x5454

arch/sh/include/asm/ioctls.h +1 -0

index 84e85a7..a6769f3 100644
@@ -87,6 +87,7 @@
 #define TIOCSPTLCK	_IOW('T',0x31, int)  /* Lock/unlock Pty */
 #define TIOCGDEV	_IOR('T',0x32, unsigned int) /* Get primary device node of /dev/console */
 #define TIOCSIG		_IOW('T',0x36, int)  /* Generate signal on Pty slave */
+#define TIOCVHANGUP	_IO('T', 0x37)
 
 #define TIOCSERCONFIG	_IO('T', 83) /* 0x5453 */
 #define TIOCSERGWILD	_IOR('T', 84,  int) /* 0x5454 */

arch/sparc/include/asm/ioctls.h +1 -0

index ed3807b..28d0c8b 100644
@@ -20,6 +20,7 @@
 #define TCSETSW2	_IOW('T', 14, struct termios2)
 #define TCSETSF2	_IOW('T', 15, struct termios2)
 #define TIOCGDEV	_IOR('T',0x32, unsigned int) /* Get primary device node of /dev/console */
+#define TIOCVHANGUP	_IO('T', 0x37)
 
 /* Note that all the ioctls that are not available in Linux have a 
  * double underscore on the front to: a) avoid some programs to

arch/xtensa/include/asm/ioctls.h +1 -0

index ccf1800..fd1d136 100644
@@ -100,6 +100,7 @@
 #define TIOCSPTLCK	_IOW('T',0x31, int)  /* Lock/unlock Pty */
 #define TIOCGDEV	_IOR('T',0x32, unsigned int) /* Get primary device node of /dev/console */
 #define TIOCSIG		_IOW('T',0x36, int)  /* Generate signal on Pty slave */
+#define TIOCVHANGUP	_IO('T', 0x37)
 
 #define TIOCSERCONFIG	_IO('T', 83)
 #define TIOCSERGWILD	_IOR('T', 84,  int)

drivers/tty/tty_io.c +5 -0

index 0065da4..f8be893 100644
@@ -2627,6 +2627,11 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		return put_user(tty->ldisc->ops->num, (int __user *)p);
 	case TIOCSETD:
 		return tiocsetd(tty, p);
+	case TIOCVHANGUP:
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+		tty_vhangup(tty);
+		return 0;
 	case TIOCGDEV:
 	{
 		unsigned int ret = new_encode_dev(tty_devnum(real_tty));

include/asm-generic/ioctls.h +1 -0

index 3f3f2d1..199975f 100644
@@ -73,6 +73,7 @@
 #define TCSETXF		0x5434
 #define TCSETXW		0x5435
 #define TIOCSIG		_IOW('T', 0x36, int)  /* pty: generate signal */
+#define TIOCVHANGUP	0x5437
 
 #define FIONCLEX	0x5450
 #define FIOCLEX		0x5451

Alan Cox

2011-02-18 09:50 UTC #1 lore raw

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

Quoted 2 lines

Without this ioctl it would have to temporarily become the owner of the tty, then call vhangup() and then give it up again.

This is a hack - it's also unfortunately not actually sufficient or complete which is why we didn't do it years ago. Sorry but if it was easy it would have been in a long time back !

drivers/tty/tty_io.c long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 2631

Quoted 2 lines

drivers/tty/tty_io.c +2 -0

+	case TIOCVHANGUP:
+		if (!capable(CAP_SYS_ADMIN))

Is there any reason for not allowing revocation of a tty that you are the owner of (ie one you could anyway take ownership of and hangup ?)

drivers/tty/tty_io.c long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 2633

Quoted 2 lines

drivers/tty/tty_io.c +2 -0

+			return -EPERM;
+		tty_vhangup(tty);

That raises a few locking questions. From a brief review of the code I think its directly 1:1 equivalent to allowing a parallel vhangup and carrier drop which we already do. The tty locks appear to cover this correctly for the basic stuff although I further review of the ldisc hangup area from someone with a strong stomache would be good.

You would still need to be very careful how you used it from the admin side because the parallel

CPU1 CPU2
vhangup() chmod()
process vhangup
return
chown to user1
chmod to 777 syscall ends (fd revocation takes effect)

Oops, 0wned

case is not handled by the paths you are using. So to actually do this you need rather more infrastructure work to ensure the existing calls have completed before returning.

At that point you've essentially provided revoke() for the tty layer so it might well be implemented to be called as revoke() as well.

So I don't actually think the patch should be applied in this form, because it simply adds an interface that will cause problems. Fixed to return after the revocation is truely complete would be good though.

I'd guess you need to add a counter to the tty f_ops entry/exit points to know when that occurs, and wake_up the revoke path when ready (remembering two revokes in parallel shouldn't deadlock! so need counting too)

In its current form the proposal isn't secure, so NAK, but I'd love to see it resubmitted with the the other bits done to return at the right moment.

Alan

Greg KH

2011-02-22 23:15 UTC #2 lore raw

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

Quoted 8 lines · Alan Cox

Quoted 2 lines
Without this ioctl it would have to temporarily become the owner of the tty, then call vhangup() and then give it up again.
This is a hack - it's also unfortunately not actually sufficient or complete which is why we didn't do it years ago. Sorry but if it was easy it would have been in a long time back !

Quoted 5 lines

drivers/tty/tty_io.c long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 2631
Quoted 2 lines
drivers/tty/tty_io.c +2 -0
+	case TIOCVHANGUP:
+		if (!capable(CAP_SYS_ADMIN))
Is there any reason for not allowing revocation of a tty that you are the owner of (ie one you could anyway take ownership of and hangup ?)

You could do that already today with the vhangup() syscall, right?

Quoted 21 lines

drivers/tty/tty_io.c long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 2633
Quoted 2 lines
drivers/tty/tty_io.c +2 -0
+			return -EPERM;
+		tty_vhangup(tty);
That raises a few locking questions. From a brief review of the code I think its directly 1:1 equivalent to allowing a parallel vhangup and carrier drop which we already do. The tty locks appear to cover this correctly for the basic stuff although I further review of the ldisc hangup area from someone with a strong stomache would be good.
You would still need to be very careful how you used it from the admin side because the parallel
CPU1 CPU2
vhangup() chmod()
process vhangup
return
chown to user1
chmod to 777 syscall ends (fd revocation takes effect)

Quoted 5 lines

Oops, 0wned
case is not handled by the paths you are using. So to actually do this you need rather more infrastructure work to ensure the existing calls have completed before returning.

But wouldn't this race still happen no matter if vhangup() is in the mix or not? I don't see how adding this ioctl changes anything here, what am I missing?

Quoted 2 lines

At that point you've essentially provided revoke() for the tty layer so it might well be implemented to be called as revoke() as well.

It's not a "real" revoke, more like vhangup(file_descriptor) only. revoke() involves a lot more than just this.

It would be great to have a real revoke() but that seems beyond this need at the moment.

Quoted 8 lines

So I don't actually think the patch should be applied in this form, because it simply adds an interface that will cause problems. Fixed to return after the revocation is truely complete would be good though.
I'd guess you need to add a counter to the tty f_ops entry/exit points to know when that occurs, and wake_up the revoke path when ready (remembering two revokes in parallel shouldn't deadlock! so need counting too)

Again, I'm confused, how does the locking differ from vhangup() today?

thanks,

greg k-h

Alan Cox

2011-02-23 00:08 UTC #3 lore raw

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

Quoted 1 line

You could do that already today with the vhangup() syscall, right?

Yes - hence the permission checks are excessive right now.

Quoted 14 lines

Quoted 14 lines
You would still need to be very careful how you used it from the admin side because the parallel
CPU1 CPU2
vhangup() chmod()
process vhangup
return
chown to user1
chmod to 777 syscall ends (fd revocation takes effect)
Oops, 0wned

Quoted 7 lines

Quoted 3 lines
case is not handled by the paths you are using. So to actually do this you need rather more infrastructure work to ensure the existing calls have completed before returning.
But wouldn't this race still happen no matter if vhangup() is in the mix or not? I don't see how adding this ioctl changes anything here, what am I missing?

The current users actually handle this - mostly by accident partly by their nature.

Quoted 2 lines

It's not a "real" revoke, more like vhangup(file_descriptor) only. revoke() involves a lot more than just this.

Real revoke is no different. General purpose real revoke for arbitary objects is a nightmare - but guess what *NO* Unixalike implements that.

Quoted 6 lines

Quoted 4 lines
I'd guess you need to add a counter to the tty f_ops entry/exit points to know when that occurs, and wake_up the revoke path when ready (remembering two revokes in parallel shouldn't deadlock! so need counting too)
Again, I'm confused, how does the locking differ from vhangup() today?

Our vhangup users are not under the delusion that the interface is race free. Really it wants fixing anyway, but exposing the race with an inviting new way to screw up isn't good - and the usage model of vhangup(fd) is such that you can't actually use the ABI for anything interesting without fixing it.

Also vhangup(fd) with the bug fixed *IS* revoke() so lets stop pretending it isn't. It's far better to add revoke to the VFS at this point and wire that to the vhangup + race fixes code that has to be done anyway. In fact I think it has to be done at that level ultimately to make sure any open versus hangup races are caught.

It's a new file op to which everyone else (for now) says NULL = no can do. Really for desktop switching we also want it for some other devices, so the infrastructure is needed anyway.

revoke is hard, vhangup(fd) is revoke, the rest inevitably follows, but once it's done then you can add it to a few other cdevs and do all the nice desktop switching stuff right as well.

It's basically 3 things
- Lennarts bits for vhangup on an fd
- A revoke vfs op
- Counting people in and out of the tty syscalls - which is a hit but its not gigabit stuff so won't hurt. Again very easy to implement, and if it causes performance corner cases on 4000 CPU boxes smart people can fix it later for that. We should do this one anyway
- Ideally dealing with parallel open/revoke paths, which needs the cdev code involved

Its not a quick patch - that's why its not happened yet, vhangup(fd) quickfix Lennart style is unfortunately a useless bodge job which like most bodge jobs is simply going to spring leaks and need fixing again.

Alan

Lennart Poettering

2011-02-23 00:23 UTC #4 lore raw

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

Quoted 2 lines · Alan Cox

It's basically 3 things
- Lennarts bits for vhangup on an fd

Uh? Me? I didn't write this patch.

(Though I do like to see patch merged and I would use it, and I have trouble following your logic.)

vhangup() is different from revoke(). vhangup() does weird SIGHUP handling and stuff, which I think goes way beyond what revoke() would eventually do. And that different behaviour becomes visible in various smaller places. e.g. vhangup() results in POLLHUP on the fd, although I assume that revoke() would more likely result in POLLERR. And there's more... Let's not pretend this is really the same thing, because it isn't.

Quoted 3 lines

Its not a quick patch - that's why its not happened yet, vhangup(fd) quickfix Lennart style is unfortunately a useless bodge job which like most bodge jobs is simply going to spring leaks and need fixing again.

Thanks. If you are trying to insult me, doesn't really work, because I didn't do this "bodge job". I'll take it as a compliment though that you say there's a "Lennart style".

Lennart, style icon

Signature · 1 line

Lennart Poettering - Red Hat, Inc.

Alan Cox

2011-02-23 00:28 UTC #5 lore raw

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

Quoted 3 lines

vhangup() is different from revoke(). vhangup() does weird SIGHUP handling and stuff, which I think goes way beyond what revoke() would eventually do. And that different behaviour becomes visible in various

Revoke() will also do this because you remove the controlling process from the tty. What happens then is mandated by SuS/POSIX.

Quoted 5 lines

Quoted 3 lines
Its not a quick patch - that's why its not happened yet, vhangup(fd) quickfix Lennart style is unfortunately a useless bodge job which like most bodge jobs is simply going to spring leaks and need fixing again.
Thanks. If you are trying to insult me, doesn't really work,

Not intended as an insult. Believe me I wish a fix like that was viable, but it's not - which is why its not been in the tree for years.

Alan

Lennart Poettering

2011-02-23 00:35 UTC #6 lore raw

Re: [PATCH] tty: add TIOCVHANGUP: time for revoke() in f_ops ?

In reply to Greg KH

Quoted 10 lines · Greg KH

Quoted 8 lines · Alan Cox
Quoted 2 lines
Without this ioctl it would have to temporarily become the owner of the tty, then call vhangup() and then give it up again.
This is a hack - it's also unfortunately not actually sufficient or complete which is why we didn't do it years ago. Sorry but if it was easy it would have been in a long time back !

Quoted 7 lines

Quoted 5 lines
drivers/tty/tty_io.c long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg) · line 2631
Quoted 2 lines
drivers/tty/tty_io.c +2 -0
+	case TIOCVHANGUP:
+		if (!capable(CAP_SYS_ADMIN))
Is there any reason for not allowing revocation of a tty that you are the owner of (ie one you could anyway take ownership of and hangup ?)
You could do that already today with the vhangup() syscall, right?

BTW, the reason why this isn't allowed is probably that you really don't want to allow unprivileged folks to kick privileged users of a TTY. TTYs can be opened by multiple parties, and stuff such as /dev/ttyS0 might be used by user logins as well as for logging, and you don't want to allow users to kick off all loggers just like that.

Lennart

Signature · 1 line

Lennart Poettering - Red Hat, Inc.