On Fri, May 14, 2021 at 1:25 PM Maciej W. Rozycki <[email protected]> wrote:
>
> 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,
It might make sense, but only if we call the function to update the
low-level data.
Not calling it, and then starting to randomly use the (wrong)
geometry, and just limiting it so that it's all within the buffer -
THAT does not make sense.
So I think your patch is fundamentally wrong. It basically says "let's
use random stale incorrect data, but just make sure that the end
result is still within the allocated buffer".
My patch is at least conceptually sane.
An alternative would be to just remove the "vcmode != KD_GRAPHICS"
check entirely, and always call con_resize() to update the low-level
data, but honestly, that seems very likelty to break something very
fundamentally, since it's not how any of fbcon has ever been tested,
Another alternative would be to just delay the resize to when vcmode
is put back to text mode again. That sounds somewhat reasonable to me,
but it's a pretty big thing.
But no, your patch to just "knowingly use entirely wrong values, then
add a limit check because we know the values are possibly garbage and
not consistent with reality" is simply not acceptable.
Linus
On Fri, May 14, 2021 at 1:32 PM Linus Torvalds
<[email protected]> wrote:
>
> Another alternative would be to just delay the resize to when vcmode
> is put back to text mode again. That sounds somewhat reasonable to me,
> but it's a pretty big thing.
Actually thinking more about that option, it sounds horrible. It would
mean that we'd continue to use the old geometry for the actual VC
buffers for a random time, and then change it to the new geometry at
some arbitrary point.
So I think the only reasonable approach (apart from just my "don't do
that then") might be to just always call ->con_resize().
There are only actually three cases of "->con_resize()", so it might
not be too bad.
Looking at it, both sisusbcon_resize() and vgacon_resize() seem to be
trivially fine in KD_GRAPHICS mode.
vgacon already seems to have that "!vga_is_gfx" test, and does
vgacon_doresize() at vgacon_switch(). It might need to add a
vgacon_doresize() to the vgacon_blank() case 0 code so that it
actually does the right thing when going back to KD_TEXT mode.
And fbcon_resize() looks like it might be mostly ok with it too.
Again, there is a con_is_visible() test, and I suspect that might need
to be changed to
if (con_is_visible(vc) && vc->vc_mode == KD_TEXT)
instead, but it doesn't look _too_ bad.
So I think just removing the "vc->vc_mode != KD_GRAPHICS" test from
resize_screen() might be the way to go. That way, the low-level data
structures actually are in sync with the resize, and the "out of
bounds" bug should never happen.
Would you mind testing that?
Linus
On Fri, 14 May 2021, Linus Torvalds wrote:
> > 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,
>
> It might make sense, but only if we call the function to update the
> low-level data.
>
> Not calling it, and then starting to randomly use the (wrong)
> geometry, and just limiting it so that it's all within the buffer -
> THAT does not make sense.
>
> So I think your patch is fundamentally wrong. It basically says "let's
> use random stale incorrect data, but just make sure that the end
> result is still within the allocated buffer".
I guess you mean Tetsuo-san's patch, right? I haven't sent any in this
discussion.
> My patch is at least conceptually sane.
>
> An alternative would be to just remove the "vcmode != KD_GRAPHICS"
> check entirely, and always call con_resize() to update the low-level
> data, but honestly, that seems very likelty to break something very
> fundamentally, since it's not how any of fbcon has ever been tested,
Umm, there isn't much to change as far as console data structures are
concerned with a resize: obviously the width and the height, which affect
the size of the character/attribute buffer, and maybe some cursor data
such as the size and screen coordinates.
For vgacon we have:
if (con_is_visible(c) && !vga_is_gfx) /* who knows */
vgacon_doresize(c, width, height);
in `vgacon_resize' already, following all the sanity checks, so the CRTC
isn't poked at if `vga_is_gfx', exactly as we want.
I can see fbcon does not have equivalent code and instead has relied on
the KD_GRAPHICS check made by the caller. Which I think has been a bug
since fbcon's inception. Instead I think `fbcon_resize' ought to make all
the sanity checks I can see it does and only then check for KD_GRAPHICS
and if so, then exit without poking at hardware. Then upon exit from the
gfx mode the `fb_set_var' call made from `fbcon_blank' will DTRT.
I can try verifying the latter hypothesis, though my framebuffer setups
(with DECstation hardware) have always been somewhat incomplete. I do
believe I have a MIPS fbdev X server binary somewhere to fiddle with,
which should work with that TGA/SFB+ video adapter I mentioned before.
> Another alternative would be to just delay the resize to when vcmode
> is put back to text mode again. That sounds somewhat reasonable to me,
> but it's a pretty big thing.
Methinks it works exactly like that already. On exit from the graphics
mode (a VT switch or gfx program termination) hardware is reprogrammed
according to the console geometry previously set. We just must not break
it.
Maciej
On Fri, May 14, 2021 at 10:33 PM Linus Torvalds
<[email protected]> wrote:
>
> On Fri, May 14, 2021 at 1:25 PM Maciej W. Rozycki <[email protected]> wrote:
> >
> > 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,
>
> It might make sense, but only if we call the function to update the
> low-level data.
>
> Not calling it, and then starting to randomly use the (wrong)
> geometry, and just limiting it so that it's all within the buffer -
> THAT does not make sense.
>
> So I think your patch is fundamentally wrong. It basically says "let's
> use random stale incorrect data, but just make sure that the end
> result is still within the allocated buffer".
>
> My patch is at least conceptually sane.
>
> An alternative would be to just remove the "vcmode != KD_GRAPHICS"
> check entirely, and always call con_resize() to update the low-level
> data, but honestly, that seems very likelty to break something very
> fundamentally, since it's not how any of fbcon has ever been tested,
Just an aside: I think with fbdev drivers this would go boom, because
you'd have fbcon interferring with a direct /dev/fb/* user.
But if your fbdev driver is actually a drm modeset driver, then we
have additional limitations: If the userspace accesses the display
through /dev/dri/card0, then the kernel blocks all access through
/dev/fb/* (including fbcon) to the actual display (it only goes into
the buffer used for fbdev emulation). And everything would be fine.
Also generally you'd get away with this even in problematic cases,
since usually you resize your console when looking at it, not when X
or something else is using your fbdev direct access.
The one thing that's left out here a bit in the cold is userspace
modeset drivers in X. Those would get hosed. But also, we stopped
supporting those in at least i915/amd/radeon/nouveau drivers,
automatically falling back to the fbdev stuff in most cases (with or
without the drm drivers underneath that), and no one screamed. So
probably not many users left.
So I /think/ we could wager this, if it's the least intrusive fix from
the kernel pov. But it has some risks that we need to revert again if
we break some of the really old use-cases here.
Cheers, Daniel
> Another alternative would be to just delay the resize to when vcmode
> is put back to text mode again. That sounds somewhat reasonable to me,
> but it's a pretty big thing.
>
> But no, your patch to just "knowingly use entirely wrong values, then
> add a limit check because we know the values are possibly garbage and
> not consistent with reality" is simply not acceptable.
>
> Linus
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch