On 18. 03. 20, 23:38, Eric Biggers wrote:
> --- a/drivers/tty/vt/vt_ioctl.c
> +++ b/drivers/tty/vt/vt_ioctl.c
> @@ -43,9 +43,11 @@ bool vt_dont_switch;
>
> static inline bool vt_in_use(unsigned int i)
> {
> - extern struct tty_driver *console_driver;
> + const struct vc_data *vc = vc_cons[i].d;
>
> - return console_driver->ttys[i] && console_driver->ttys[i]->count;
> + WARN_CONSOLE_UNLOCKED();
> +
> + return vc && kref_read(&vc->port.kref) > 1;
> }
>
> static inline bool vt_busy(int i)
> @@ -643,15 +645,16 @@ int vt_ioctl(struct tty_struct *tty,
> struct vt_stat __user *vtstat = up;
> unsigned short state, mask;
>
> - /* Review: FIXME: Console lock ? */
> if (put_user(fg_console + 1, &vtstat->v_active))
> ret = -EFAULT;
> else {
> state = 1; /* /dev/tty0 is always open */
> + console_lock();
Could you comment on this one and the lock below why you added it here?
To me, it seems, we should rather introduce a vt alloc/dealloc lock
protecting cases like this, not console lock. But not now, some time
later. So a comment would help when/once we/I get into it...
The interface (ie. the ioctls) also look weird and racy. Both of them.
Like the "OK, I give you this number, but it might not be correct by
now." kind of thing.
This let me think, who could use this? The answer is many 8-/. openpt,
systemd, sysvinit, didn't check others.
Perhaps we should provide openvt -- analogy of openpty and deprecate
VT_OPENQRY?
With VT_GETSTATE, the situation is more complicated:
sysvinit uses VT_GETSTATE only if TIOCGDEV is not available, so
VT_GETSTATE is actually unneeded there.
systemd uses it to find the current console (vtstat->v_active) and
systemd-logind uses it for spawning autovt on free consoles. That sort
of makes sense...
> for (i = 0, mask = 2; i < MAX_NR_CONSOLES && mask;
> ++i, mask <<= 1)
> if (vt_in_use(i))
> state |= mask;
> + console_unlock();
> ret = put_user(state, &vtstat->v_state);
> }
> break;
> @@ -661,10 +664,11 @@ int vt_ioctl(struct tty_struct *tty,
> * Returns the first available (non-opened) console.
> */
> case VT_OPENQRY:
> - /* FIXME: locking ? - but then this is a stupid API */
> + console_lock();
> for (i = 0; i < MAX_NR_CONSOLES; ++i)
> if (!vt_in_use(i))
> break;
> + console_unlock();
> uival = i < MAX_NR_CONSOLES ? (i+1) : -1;
> goto setint;
>
>
thanks,
--
js
suse labs
