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

[Greg KH <greg@kroah.com>]

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