[Devel] [RFC rh7 v4] ve/tty: vt -- Implement per VE support for console and terminals

[Cyrill Gorcunov]

On 08/21/2015 05:15 PM, Vladimir Davydov wrote:
...
>> + *
>> + * For every container here is a tty map represented by
>> + * vtty_map_t. It carries @veid of VE and associated slave
>> + * tty peers. Also there is a per-VE spinlock to order
>> + * close() operation.
> 
> I don't see any spinlocks in your code.

Forgot to update comment. I've been using per-ve spinlock
to make sure we're not modifying counters in parallel mode.


>> +#define vtty_printk(fmt, ...)							\
>> +	printk("vtty: %20s: %4d: " fmt,						\
>> +	       __func__, __LINE__, ##__VA_ARGS__)
>> +#define vtty_printk_one(__tty)							\
>> +	vtty_printk("tty %p idx %2d cnt %2d fl 0x%-8lx\n",			\
>> +		    (__tty), (__tty)->index, (__tty)->count, (__tty)->flags)
>> +#define vtty_printk_pair(__tty)							\
>> +	vtty_printk("tty %p idx %2d cnt %2d fl 0x%-8lx "			\
>> +		    "link %p idx %2d cnt %2d fl 0x%-8lx\n",			\
>> +		    (__tty), (__tty)->index, (__tty)->count, (__tty)->flags,	\
>> +		    (__tty)->link, (__tty)->link ? (__tty)->link->index : -1,	\
>> +		    (__tty)->link ? (__tty)->link->count : -1,			\
>> +		    (__tty)->link ? (__tty)->link->flags : -1ul)
> 
> tty_io.c uses pr_debug. Why should we be different?

Because if you run kernel with "debug" by now it would spam the log
too much ;/ Also I would like to not print all this even in debug
mode but only when it's explicitly requested on compile time because
frankly there is no much point in spamming the kernel.

>> +static envid_t vtty_get_context(void)
>> +{
>> +	BUILD_BUG_ON(sizeof(current_veid) < sizeof(envid_t));
>> +	lockdep_assert_held(&tty_mutex);
>> +
>> +	if (likely(current_veid == VTTY_USE_EXEC_VEID))
> 
> Nit: likely is redundant here, it's not a hot path.
> 
> BTW, since vttys should never be used by ve0 we could use 0 instead of
> VTTY_USE_EXEC_VEID. Then the body of this function would look like this:
> 
> 	return current_veid ?: get_exec_env()->veid;
> 
> (I like ?: with the first choice omitted :-)

Yeah, I though about it but it would be a bit vague I think.
If you prefer though I can switch to such scheme but need
to put a big fat comment about zero number.

>> +static struct tty_struct *vtty_lookup(struct tty_driver *driver,
>> +				      struct inode *inode, int idx)
>> +{
>> +	vtty_map_t *map = vtty_map_lookup(vtty_get_context());
>> +	struct tty_struct *tty;
>> +
>> +	WARN_ON_ONCE((driver != vttym_driver) &&
>> +		     (driver != vttys_driver));
>> +
>> +	if (!vtty_match_index(idx))
>> +		return ERR_PTR(-EIO);
>> +
>> +	vtty_printk("driver %s index %d\n", driver->driver_name, idx);
>> +	/*
>> +	 * Nothing ever been opened yet, allocate a new
>> +	 * tty map together with both peers from the scratch
>> +	 * in install procedure.
>> +	 */
>> +	if (!map)
>> +		return NULL;
>> +
>> +	/*
>> +	 * XXX: Strictly speaking there should not
>> +	 * be requests for master peers from
>> +	 * inside of container (ie lookup for
>> +	 * vttym_driver). Even vtty_open_master
>> +	 * may simply fetch tty->link by self
>> +	 * after lookup for slave returned valid
>> +	 * tty pointer. But I leave it this way
>> +	 * simply because not sure yet how to
>> +	 * c/r containers with live connection
>> +	 * from nodes and this provides an easier
>> +	 * way for testing.
>> +	 */
> 
> I wonder if we need to handle this open-master-from-ct case now. This
> won't work if there's no slave, because there is no working install
> method for master, so we will have to write new code anyway if we need
> it. May be, better just zap it for now, and only introduce hunks like
> this when they are really necessary?

I'm not sure yet how we would handle restore case, that's why such
comment is present. But opening master from inside of container will
require more kernel code anyway so I'll drop this comment.

>> +	tty = map->vttys[idx];
>> +	if (tty) {
>> +		if (vtty_is_exiting(tty) || vtty_is_exiting(tty->link))
> 
> 1. There is absolutely no point in checking this for 'tty', because
>    tty_reopen, which is normally called right after ->lookup, already
>    takes care of it.

This check is redundant in case of opening pair from container but
needed when we open pair from node. If you prefer I move it into
vtty_open_master.

> 
> 2. What's the point in checking TTY_HUPPING and TTY_LDISC_CHANGING for
>    'tty->link'? We are opening 'tty' here.

Because if the link is started hupping or changing line discipline
there is no much sence to return such pair, it's almost dead. Simply
notify the user that there is no such tty present at moment and make
it repeat the attempt.

> 
>> +			tty = ERR_PTR(-ENXIO);
> 
> Just curious, why ENXIO? Why not EIO?

You know at first i took it from vt code but I think indeed
-eio will be more appropriate, because ENXIO usually stands
that there some request over nonexisting device which is not
true for this case. Thanks!

> Anyway, returning an error here means tty_open failure. Now suppose
> somebody attempts to open slave vtty while the pair is being destroyed.
> A failure would be unexpected then. AFAIU we should return NULL here for
> tty_open to create a new pair.

Which gives you a way to spam the kernel. Simply start open/close ttys
in a cycle and there is a small window which allow to create more than
12 ttys as far as I can tell. Instead with the approach above we don't
allow to open pair while previous session is not complete. Am I wrong?

>> +	peer = vtty_install_peer(map, vttym_driver, peer_port, tty->index);
>> +	if (IS_ERR(peer)) {
>> +		vtty_map_del(map, tty, tty->index);
> 
> Do we need it here?

nope, thanks!

>> +static int vtty_open(struct tty_struct *tty, struct file *filp)
>> +{
>> +	if (test_bit(TTY_OTHER_CLOSED, &tty->flags))
>> +		return -EIO;
> 
> What's the point in this check? You only set TTY_OTHER_CLOSED when both
> ends are closed and hence the pair is dead.
> 
>> +
>> +	clear_bit(TTY_IO_ERROR, &tty->flags);
> 
> Ditto?
> 
>> +	clear_bit(TTY_OTHER_CLOSED, &tty->link->flags);
>> +	set_bit(TTY_THROTTLED, &tty->flags);
>> +
>> +	vtty_printk_pair(tty);
>> +	return 0;
>> +}
>> +
>> +static void vtty_close(struct tty_struct *tty, struct file *filp)
>> +{
>> +	struct tty_struct *peer = tty->link;
>> +	vtty_map_t *map = tty->driver_data;
> 
> map is unused in this function.

thanks

> 
>> +
>> +	/*
>> +	 * Some other file is still using us.
>> +	 */
>> +	if (tty->count > 2)
>> +		return;
>> +
>> +	if (test_bit(TTY_IO_ERROR, &tty->flags))
>> +		return;
> 
> Why?
> 
>> +
>> +	if (tty->driver == vttys_driver) {
>> +		if (peer->count == 1) {
>> +			clear_bit(TTY_EXTRA_REF, &peer->flags);
>> +			peer->count--;
> 
> Here you read and modify peer->count. Why is it safe? You hold neither
> peer->legacy_mutex nor tty_mutex here.
> 
> Another question regarding synchronization is what prevents the
> following race, which results in leaking a vtty pair?
> 
> CPU0				CPU1
> ----				----
> opened slave			opened slave
> 
> [ master->count = 1, slave->count = 3 ]
> 
> closing slave			closing slave
>  tty_release
>   ->close (vtty_close)
>    tty->count > 2 => return
>    				 tty_release
> 				  ->close (vtty_close)
> 				   tty->count > 2 => return
>   lock tty_mutex
>   tty->count--;
>   unlock tty_mutex
> 				  lock tty_mutex
> 				  tty->count--;
> 				  unlock tty_mutex
> 
> [ master->count = 1, slave->count = 1 ]
> 
> If this race can happen, is it relevant to pty too?
>

Letme think on this and TTY_IO_ERROR comment, I'll reply separately.

>> +			tty->count--;
>> +		}
>> +	} else {
>> +		if (peer->count == 1) {
>> +			clear_bit(TTY_EXTRA_REF, &tty->flags);
>> +			tty->count--;
>> +		} else
>> +			peer->count++;
> 
> It is not obvious why we need to increment peer->count here. Please add
> a comment.

Sure, thanks!

> 
>> +	}
>> +
>> +	if (tty->count == 1) {
>> +		set_bit(TTY_IO_ERROR, &tty->flags);
>> +		set_bit(TTY_OTHER_CLOSED, &peer->flags);
>> +	}
>> +
>> +	tty->packet = 0;
> 
> You don't implement TIOCPKT ioctl so tty->packet is always 0 and this is
> redundant.

OK

> 
>> +	wake_up_interruptible(&tty->read_wait);
>> +	wake_up_interruptible(&tty->write_wait);
>> +
>> +	peer->packet = 0;
> 
> Ditto.

OK

>>  
>>  #ifdef CONFIG_VE
>> -	extern struct tty_driver *vz_vt_device(struct ve_struct *ve, dev_t dev, int *index);
>> -	if (!ve_is_super(get_exec_env()) &&
>> -	    (MAJOR(device) == TTY_MAJOR && MINOR(device) < VZ_VT_MAX_DEVS)) {
>> -		driver = tty_driver_kref_get(vz_vt_device(get_exec_env(), device, index));
>> +	struct ve_struct *ve = get_exec_env();
>> +
>> +	if (!ve_is_super(ve) && vtty_match(device)) {
> 
> This is the only place where vtty_match() is used. Wouldn't it be better
> to fold it in vtty_driver()? I mean
> 
> struct tty_driver *vtty_driver(dev_t dev, int *index)
> {
> 	if (MAJOR(device) == TTY_MAJOR &&
> 	    vtty_match_index(MINOR(device))) {
> 		*index = MINOR(device);
> 		return vttys_driver;
> 	}
> 	return NULL;
> }
>

tty_driver_kref_get can't be called for NULL driver.
But sure I'll rework to not use vtty_match_index and
check for explicit @driver != NULL after vtty_driver.

>>  
>> +#define MAX_NR_VTTY_CONSOLES		(12)
>> +
>> +#ifdef CONFIG_TTY
>> +extern int vtty_match(dev_t device);
>> +extern struct tty_driver *vtty_driver(dev_t dev, int *index);
>> +extern struct tty_driver *vtty_console_driver(int *index);
>> +extern int vtty_open_master(envid_t veid, int idx);
>> +#else /* CONFIG_TTY */
>> +static inline int vtty_match(dev_t device) { return 0; }
>> +static inline struct tty_driver *vtty_driver(dev_t dev, int *index) { return NULL; }
>> +static inline struct tty_driver *vtty_console_driver(int *index) { return NULL; }
> 
> If !CONFIG_TTY, these functions will never be used so remove the stubs
> please.
> 

ok. Thanks a huge (!) for comments

-- 
    Cyrill