2011-02-17 17:41:55

by Kay Sievers

[permalink] [raw]
Subject: [PATCH] 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(+)

diff --git a/arch/alpha/include/asm/ioctls.h b/arch/alpha/include/asm/ioctls.h
index 034b6cf..80e1cee 100644
--- a/arch/alpha/include/asm/ioctls.h
+++ b/arch/alpha/include/asm/ioctls.h
@@ -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
diff --git a/arch/mips/include/asm/ioctls.h b/arch/mips/include/asm/ioctls.h
index d967b89..92403c3 100644
--- a/arch/mips/include/asm/ioctls.h
+++ b/arch/mips/include/asm/ioctls.h
@@ -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 */
diff --git a/arch/parisc/include/asm/ioctls.h b/arch/parisc/include/asm/ioctls.h
index 6ba80d0..054ec06 100644
--- a/arch/parisc/include/asm/ioctls.h
+++ b/arch/parisc/include/asm/ioctls.h
@@ -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
diff --git a/arch/powerpc/include/asm/ioctls.h b/arch/powerpc/include/asm/ioctls.h
index c7dc17c..e9b7887 100644
--- a/arch/powerpc/include/asm/ioctls.h
+++ b/arch/powerpc/include/asm/ioctls.h
@@ -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
diff --git a/arch/sh/include/asm/ioctls.h b/arch/sh/include/asm/ioctls.h
index 84e85a7..a6769f3 100644
--- a/arch/sh/include/asm/ioctls.h
+++ b/arch/sh/include/asm/ioctls.h
@@ -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 */
diff --git a/arch/sparc/include/asm/ioctls.h b/arch/sparc/include/asm/ioctls.h
index ed3807b..28d0c8b 100644
--- a/arch/sparc/include/asm/ioctls.h
+++ b/arch/sparc/include/asm/ioctls.h
@@ -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
diff --git a/arch/xtensa/include/asm/ioctls.h b/arch/xtensa/include/asm/ioctls.h
index ccf1800..fd1d136 100644
--- a/arch/xtensa/include/asm/ioctls.h
+++ b/arch/xtensa/include/asm/ioctls.h
@@ -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)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 0065da4..f8be893 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -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));
diff --git a/include/asm-generic/ioctls.h b/include/asm-generic/ioctls.h
index 3f3f2d1..199975f 100644
--- a/include/asm-generic/ioctls.h
+++ b/include/asm-generic/ioctls.h
@@ -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


2011-02-18 09:50:13

by Alan

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

> 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 !


> + 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 ?)


> + 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

2011-02-22 23:23:49

by Greg KH

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

On Fri, Feb 18, 2011 at 09:50:48AM +0000, Alan Cox wrote:
> > 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 !
>
>
> > + 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?

> > + 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.

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?

> 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.

> 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

2011-02-23 00:08:00

by Alan

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

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

Yes - hence the permission checks are excessive right now.

> > 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.
>
> 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.

> 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.

> > 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

2011-02-23 00:28:34

by Alan

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

> 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.

> > 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

2011-02-23 00:31:29

by Lennart Poettering

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

On Wed, 23.02.11 00:09, Alan Cox ([email protected]) wrote:

> 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.

> 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

--
Lennart Poettering - Red Hat, Inc.

2011-02-23 00:36:27

by Lennart Poettering

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

On Tue, 22.02.11 15:15, Greg KH ([email protected]) wrote:

>
> On Fri, Feb 18, 2011 at 09:50:48AM +0000, Alan Cox wrote:
> > > 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 !
> >
> >
> > > + 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

--
Lennart Poettering - Red Hat, Inc.