[PATCH 0/6] assorted console/vt/vcs fixes

[PATCH 0/6] assorted console/vt/vcs fixes

[Nicolas Pitre]

Hello Greg,

Here's a few patches fixing various issues. The first 2 are most
certainly stable material. I don't know if the other issues are serious enough to be stable worthy though.
Please feel free to tag them as such if so.


Nicolas

[PATCH 1/6] vt: make vt_console_print() compatible with the unicode screen buffer

[Nicolas Pitre]

When kernel messages are printed to the console, they appear blank on
the unicode screen. This is because vt_console_print() is lacking a call
to vc_uniscr_putc(). However the later function assumes vc->vc_x is
always up to date when called, which is not the case here as
vt_console_print() uses it to mark the beginning of the display update.

This patch reworks (and simplifies) vt_console_print() so that vc->vc_x
is always valid and keeps the start of display update in a local variable
instead, which finally allows for adding the missing vc_uniscr_putc()
call.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
Cc: stable@vger.kernel.org # v4.19+
---
 drivers/tty/vt/vt.c | 47 +++++++++++++++------------------------------
 1 file changed, 15 insertions(+), 32 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 41ec8e5010..b9004bd087 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2884,8 +2884,7 @@ static void vt_console_print(struct console *co, const char *b, unsigned count)
 	unsigned char c;
 	static DEFINE_SPINLOCK(printing_lock);
 	const ushort *start;
-	ushort cnt = 0;
-	ushort myx;
+	ushort start_x, cnt;
 	int kmsg_console;
 
 	/* console busy or not yet initialized */
@@ -2898,10 +2897,6 @@ static void vt_console_print(struct console *co, const char *b, unsigned count)
 	if (kmsg_console && vc_cons_allocated(kmsg_console - 1))
 		vc = vc_cons[kmsg_console - 1].d;
 
-	/* read `x' only after setting currcons properly (otherwise
-	   the `x' macro will read the x of the foreground console). */
-	myx = vc->vc_x;
-
 	if (!vc_cons_allocated(fg_console)) {
 		/* impossible */
 		/* printk("vt_console_print: tty %d not allocated ??\n", currcons+1); */
@@ -2916,53 +2911,41 @@ static void vt_console_print(struct console *co, const char *b, unsigned count)
 		hide_cursor(vc);
 
 	start = (ushort *)vc->vc_pos;
-
-	/* Contrived structure to try to emulate original need_wrap behaviour
-	 * Problems caused when we have need_wrap set on '\n' character */
+	start_x = vc->vc_x;
+	cnt = 0;
 	while (count--) {
 		c = *b++;
 		if (c == 10 || c == 13 || c == 8 || vc->vc_need_wrap) {
-			if (cnt > 0) {
-				if (con_is_visible(vc))
-					vc->vc_sw->con_putcs(vc, start, cnt, vc->vc_y, vc->vc_x);
-				vc->vc_x += cnt;
-				if (vc->vc_need_wrap)
-					vc->vc_x--;
-				cnt = 0;
-			}
+			if (cnt && con_is_visible(vc))
+				vc->vc_sw->con_putcs(vc, start, cnt, vc->vc_y, start_x);
+			cnt = 0;
 			if (c == 8) {		/* backspace */
 				bs(vc);
 				start = (ushort *)vc->vc_pos;
-				myx = vc->vc_x;
+				start_x = vc->vc_x;
 				continue;
 			}
 			if (c != 13)
 				lf(vc);
 			cr(vc);
 			start = (ushort *)vc->vc_pos;
-			myx = vc->vc_x;
+			start_x = vc->vc_x;
 			if (c == 10 || c == 13)
 				continue;
 		}
+		vc_uniscr_putc(vc, c);
 		scr_writew((vc->vc_attr << 8) + c, (unsigned short *)vc->vc_pos);
 		notify_write(vc, c);
 		cnt++;
-		if (myx == vc->vc_cols - 1) {
-			vc->vc_need_wrap = 1;
-			continue;
-		}
-		vc->vc_pos += 2;
-		myx++;
-	}
-	if (cnt > 0) {
-		if (con_is_visible(vc))
-			vc->vc_sw->con_putcs(vc, start, cnt, vc->vc_y, vc->vc_x);
-		vc->vc_x += cnt;
-		if (vc->vc_x == vc->vc_cols) {
-			vc->vc_x--;
+		if (vc->vc_x == vc->vc_cols - 1) {
 			vc->vc_need_wrap = 1;
+		} else {
+			vc->vc_pos += 2;
+			vc->vc_x++;
 		}
 	}
+	if (cnt && con_is_visible(vc))
+		vc->vc_sw->con_putcs(vc, start, cnt, vc->vc_y, start_x);
 	set_cursor(vc);
 	notify_update(vc);
 
-- 
2.20.1

[PATCH 2/6] vt: always call notifier with the console lock held

[Nicolas Pitre]

Every invocation of notify_write() and notify_update() is performed
under the console lock, except for one case. Let's fix that.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
Cc: stable@vger.kernel.org
---
 drivers/tty/vt/vt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index b9004bd087..5b8b0e33f9 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2764,8 +2764,8 @@ static int do_con_write(struct tty_struct *tty, const unsigned char *buf, int co
 	con_flush(vc, draw_from, draw_to, &draw_x);
 	vc_uniscr_debug_check(vc);
 	console_conditional_schedule();
-	console_unlock();
 	notify_update(vc);
+	console_unlock();
 	return n;
 }
 
-- 
2.20.1

[PATCH 3/6] vt: invoke notifier on screen size change

[Nicolas Pitre]

User space using poll() on /dev/vcs devices are not awaken when a
screen size change occurs. Let's fix that.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 drivers/tty/vt/vt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 5b8b0e33f9..bba75560d1 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1272,6 +1272,7 @@ static int vc_do_resize(struct tty_struct *tty, struct vc_data *vc,
 	if (con_is_visible(vc))
 		update_screen(vc);
 	vt_event_post(VT_EVENT_RESIZE, vc->vc_num, vc->vc_num);
+	notify_update(vc);
 	return err;
 }
 
-- 
2.20.1

[PATCH 4/6] vcsa: clamp header values when they don't fit

[Nicolas Pitre]

The /dev/vcsa* devices have a fixed char-sized header that stores the
screen geometry and cursor location. Let's make sure it doesn't contain
random garbage when those values exceed 255. If ever it becomes necessary
to convey larger screen info to user space then a larger header in the
not-yet-implemented /dev/vcsua* devices should be considered.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 drivers/tty/vt/vc_screen.c | 5 +++--
 drivers/tty/vt/vt.c        | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index 2384ea85ff..3dba60825c 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -335,8 +335,9 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 			if (p < HEADER_SIZE) {
 				size_t tmp_count;
 
-				con_buf0[0] = (char)vc->vc_rows;
-				con_buf0[1] = (char)vc->vc_cols;
+				/* clamp header values if they don't fit */
+				con_buf0[0] = min(vc->vc_rows, 0xFFu);
+				con_buf0[1] = min(vc->vc_cols, 0xFFu);
 				getconsxy(vc, con_buf0 + 2);
 
 				con_buf_start += p;
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index bba75560d1..f519c22e70 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -4591,8 +4591,9 @@ EXPORT_SYMBOL_GPL(screen_pos);
 
 void getconsxy(struct vc_data *vc, unsigned char *p)
 {
-	p[0] = vc->vc_x;
-	p[1] = vc->vc_y;
+	/* clamp values if they don't fit */
+	p[0] = min(vc->vc_x, 0xFFu);
+	p[1] = min(vc->vc_y, 0xFFu);
 }
 
 void putconsxy(struct vc_data *vc, unsigned char *p)
-- 
2.20.1

[PATCH 5/6] vcs: poll(): cope with a deallocated vt

[Nicolas Pitre]

When VT_DISALLOCATE is used on a vt, user space waiting with poll() on
the corresponding /dev/vcs device is not awakened. This is now fixed by
returning POLLHUP|POLLERR to user space.

Also, in the normal screen update case, we don't set POLLERR anymore as
POLLPRI alone is a much more logical response in a non-error situation,
saving some confusion on the user space side. The only known user app
making use of poll() on /dev/vcs* is BRLTTY which is known to cope with
that change already, so the risk of breakage is pretty much nonexistent.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 drivers/tty/vt/vc_screen.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index 3dba60825c..1bbe2a30cd 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -80,7 +80,7 @@
 struct vcs_poll_data {
 	struct notifier_block notifier;
 	unsigned int cons_num;
-	bool seen_last_update;
+	int event;
 	wait_queue_head_t waitq;
 	struct fasync_struct *fasync;
 };
@@ -94,7 +94,7 @@ vcs_notifier(struct notifier_block *nb, unsigned long code, void *_param)
 		container_of(nb, struct vcs_poll_data, notifier);
 	int currcons = poll->cons_num;
 
-	if (code != VT_UPDATE)
+	if (code != VT_UPDATE && code != VT_DEALLOCATE)
 		return NOTIFY_DONE;
 
 	if (currcons == 0)
@@ -104,7 +104,7 @@ vcs_notifier(struct notifier_block *nb, unsigned long code, void *_param)
 	if (currcons != vc->vc_num)
 		return NOTIFY_DONE;
 
-	poll->seen_last_update = false;
+	poll->event = code;
 	wake_up_interruptible(&poll->waitq);
 	kill_fasync(&poll->fasync, SIGIO, POLL_IN);
 	return NOTIFY_OK;
@@ -261,7 +261,7 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 
 	poll = file->private_data;
 	if (count && poll)
-		poll->seen_last_update = true;
+		poll->event = 0;
 	read = 0;
 	ret = 0;
 	while (count) {
@@ -616,12 +616,21 @@ static __poll_t
 vcs_poll(struct file *file, poll_table *wait)
 {
 	struct vcs_poll_data *poll = vcs_poll_data_get(file);
-	__poll_t ret = DEFAULT_POLLMASK|EPOLLERR|EPOLLPRI;
+	__poll_t ret = DEFAULT_POLLMASK|EPOLLERR;
 
 	if (poll) {
 		poll_wait(file, &poll->waitq, wait);
-		if (poll->seen_last_update)
+		switch (poll->event) {
+		case VT_UPDATE:
+			ret = DEFAULT_POLLMASK|EPOLLPRI;
+			break;
+		case VT_DEALLOCATE:
+			ret = DEFAULT_POLLMASK|EPOLLHUP|EPOLLERR;
+			break;
+		case 0:
 			ret = DEFAULT_POLLMASK;
+			break;
+		}
 	}
 	return ret;
 }
-- 
2.20.1

[PATCH 6/6] vcs: fasync(): make it consistent with poll()

[Nicolas Pitre]

We use POLLPRI not POLLIN to wait for data with poll() as there is
never any incoming data stream per se. Let's use the same semantic
with fasync() for consistency, including the fact that a vt may go away.

No known user space ever relied on the SIGIO reason so far, let alone
FASYNC, so the risk of breakage is pretty much nonexistent.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 drivers/tty/vt/vc_screen.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index 1bbe2a30cd..1d887113ff 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -93,9 +93,18 @@ vcs_notifier(struct notifier_block *nb, unsigned long code, void *_param)
 	struct vcs_poll_data *poll =
 		container_of(nb, struct vcs_poll_data, notifier);
 	int currcons = poll->cons_num;
-
-	if (code != VT_UPDATE && code != VT_DEALLOCATE)
+	int fa_band;
+
+	switch (code) {
+	case VT_UPDATE:
+		fa_band = POLL_PRI;
+		break;
+	case VT_DEALLOCATE:
+		fa_band = POLL_HUP;
+		break;
+	default:
 		return NOTIFY_DONE;
+	}
 
 	if (currcons == 0)
 		currcons = fg_console;
@@ -106,7 +115,7 @@ vcs_notifier(struct notifier_block *nb, unsigned long code, void *_param)
 
 	poll->event = code;
 	wake_up_interruptible(&poll->waitq);
-	kill_fasync(&poll->fasync, SIGIO, POLL_IN);
+	kill_fasync(&poll->fasync, SIGIO, fa_band);
 	return NOTIFY_OK;
 }
 
-- 
2.20.1

Re: [PATCH 5/6] vcs: poll(): cope with a deallocated vt

[Nicolas Pitre]

On Tue, 8 Jan 2019, Nicolas Pitre wrote:

> When VT_DISALLOCATE is used on a vt, user space waiting with poll() on
> the corresponding /dev/vcs device is not awakened. This is now fixed by
> returning POLLHUP|POLLERR to user space.
> 
> Also, in the normal screen update case, we don't set POLLERR anymore as
> POLLPRI alone is a much more logical response in a non-error situation,
> saving some confusion on the user space side. The only known user app
> making use of poll() on /dev/vcs* is BRLTTY which is known to cope with
> that change already, so the risk of breakage is pretty much nonexistent.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>

That patch introduced a small unwanted behavior change that I intend to 
fix in a follow-up patch (it will be tagged [PATCH 7/6]). I prefer to go 
with a separate patch rather than respinning this one as this gives me 
the opportunity to separately document said behavior.

Nicolas

Re: [PATCH 0/6] assorted console/vt/vcs fixes

[Greg Kroah-Hartman]

On Tue, Jan 08, 2019 at 10:54:58PM -0500, Nicolas Pitre wrote:
> Hello Greg,
> 
> Here's a few patches fixing various issues. The first 2 are most
> certainly stable material. I don't know if the other issues are serious enough to be stable worthy though.
> Please feel free to tag them as such if so.

Thanks for all of these, now queued up.

greg k-h