On Fri, May 14, 2021 at 9:20 AM Tetsuo Handa
<[email protected]> wrote:
>
> Currently it is impossible to control upper limit of rows/columns values
> based on amount of memory reserved for the graphical screen, for
> resize_screen() calls vc->vc_sw->con_resize() only if vc->vc_mode is not
> already KD_GRAPHICS
Honestly, the saner approach would seem to be to simply error out if
vc_mode is KD_GRAPHICS.
Doing VT_RESIZE while in KD_GRAPHICS mode seems _very_ questionable,
and is clearly currently very buggy.
So why not just say "that clearly already doesn't work, so make it
explicitly not permitted"?
Linus
On Fri, May 14, 2021 at 10:29 AM Linus Torvalds
<[email protected]> wrote:
>
> So why not just say "that clearly already doesn't work, so make it
> explicitly not permitted"?
IOW, something like this would seem fairly simple and straightforward:
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 01645e87b3d5..f24e627b7402 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -1171,8 +1171,13 @@ static inline int resize_screen(struct
vc_data *vc, int width, int height,
/* Resizes the resolution of the display adapater */
int err = 0;
- if (vc->vc_mode != KD_GRAPHICS && vc->vc_sw->con_resize)
+ if (vc->vc_sw->con_resize) {
+ // If we have a resize function but are in KD_GRAPHICS mode,
+ // we can't actually do a resize and need to error out.
+ if (vc->vc_mode == KD_GRAPHICS)
+ return -EINVAL;
err = vc->vc_sw->con_resize(vc, width, height, user);
+ }
return err;
}
not tested, but it looks ObviouslyCorrect(tm), and since we know the
old case didn't work right, it seems very safe to do.
Linus
On Fri, 14 May 2021, Linus Torvalds wrote:
> > Currently it is impossible to control upper limit of rows/columns values
> > based on amount of memory reserved for the graphical screen, for
> > resize_screen() calls vc->vc_sw->con_resize() only if vc->vc_mode is not
> > already KD_GRAPHICS
>
> Honestly, the saner approach would seem to be to simply error out if
> vc_mode is KD_GRAPHICS.
>
> Doing VT_RESIZE while in KD_GRAPHICS mode seems _very_ questionable,
> and is clearly currently very buggy.
I haven't looked into it any further beyond tracking down (again, using
the LMO tree) the originating change as the other fix took precedence. It
came with:
commit 094e0a9cdbdf1e11a28dd756a6cbd750b6303d10
Author: Ralf Baechle <[email protected]>
Date: Sun Jun 1 12:07:37 2003 +0000
Merge with Linux 2.5.51
along with framebuffer console support:
+inline int resize_screen(int currcons, int width, int height)
+{
+ /* Resizes the resolution of the display adapater */
+ int err = 0;
+
+ if (vcmode != KD_GRAPHICS && sw->con_resize)
+ err = sw->con_resize(vc_cons[currcons].d, width, height);
+ return err;
+}
+
A handler for fbcon was added shortly afterwards with:
commit bab384bdbe279efd7acc2146ef13b0b0395b2a42
Author: Ralf Baechle <[email protected]>
Date: Tue Jun 3 17:04:10 2003 +0000
Merge with Linux 2.5.59.
however vgacon didn't have a handler for it until commit 28254d439b8c
("[PATCH] vga text console and stty cols/rows") two years later only.
Overall I think it does make sense to resize the text console at any
time, even if the visible console (VT) chosen is in the graphics mode, as
my understanding (and experience at least with vgacon) is that resizing
the console applies globally across all the VTs. So the intent of the
original change appears valid to me, and the choice not to reprogram the
visible console and only store the settings for a future use if it's in
the graphics mode correct.
Which means any bug triggered here needs to be fixed elsewhere rather
than by making the request fail.
NB for fbcon the usual ioctl to resize the console is FBIOPUT_VSCREENINFO
rather than VT_RESIZEX; fbset(8) uses it, and I actually experimented with
it and a TGA-like (SFB+) framebuffer when at my lab last time, as Linux is
kind enough to know how to fiddle with its clockchip. It works just fine.
Maciej
On 2021/05/15 5:25, Maciej W. Rozycki wrote:
> NB for fbcon the usual ioctl to resize the console is FBIOPUT_VSCREENINFO
> rather than VT_RESIZEX; fbset(8) uses it, and I actually experimented with
> it and a TGA-like (SFB+) framebuffer when at my lab last time, as Linux is
> kind enough to know how to fiddle with its clockchip. It works just fine.
fbcon_update_vcs() from FBIOPUT_VSCREENINFO is no-op if vc->vc_mode != KD_TEXT
(which is equivalent to "if vc->vc_mode == KD_GRAPHICS" because KD_TEXT0/KD_TEXT1
are treated as KD_TEXT). Then, maybe it is OK to let resize_screen() return -EINVAL
in order to make vc_do_resize() request fail if vc->vc_mode == KD_GRAPHICS.
> Overall I think it does make sense to resize the text console at any
> time, even if the visible console (VT) chosen is in the graphics mode, as
> my understanding (and experience at least with vgacon) is that resizing
> the console applies globally across all the VTs. So the intent of the
> original change appears valid to me, and the choice not to reprogram the
> visible console and only store the settings for a future use if it's in
> the graphics mode correct.
>
> Which means any bug triggered here needs to be fixed elsewhere rather
> than by making the request fail.
Since syzbot does not trigger this problem with Linus's patch, I think we can
try Linus's patch with
pr_info_once("Resizing text console while in graphical mode is ignored. Please report if you need this.\n");
added in order to see if somebody wants "only store the settings for a future use".