On 7/1/22 16:52, Geert Uytterhoeven wrote:

Hi Helge,

On Fri, Jul 1, 2022 at 11:30 AM Helge Deller [off-list ref] wrote:

quoted

On 7/1/22 09:28, Geert Uytterhoeven wrote:

quoted

On Thu, Jun 30, 2022 at 10:10 PM Helge Deller [off-list ref] wrote:

quoted

On 6/30/22 22:00, Geert Uytterhoeven wrote:

quoted

On Thu, Jun 30, 2022 at 9:46 PM Helge Deller [off-list ref] wrote:

quoted

On 6/30/22 21:36, Geert Uytterhoeven wrote:

quoted

On Thu, Jun 30, 2022 at 9:31 PM Helge Deller [off-list ref] wrote:

quoted

On 6/30/22 21:00, Geert Uytterhoeven wrote:

quoted

On Wed, Jun 29, 2022 at 10:00 PM Helge Deller [off-list ref] wrote:

quoted

The virtual screen size can't be smaller than the physical screen size.
Based on the general rule that we round up user-provided input if
neccessary, adjust the virtual screen size as well if needed.

Signed-off-by: Helge Deller <deller@gmx.de>
Cc: stable@vger.kernel.org # v5.4+

Thanks for your patch!

quoted

--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c

@@ -1106,6 +1106,11 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
                        return -EFAULT;
                console_lock();
                lock_fb_info(info);
+               /* adjust virtual screen size if user missed it */
+               if (var.xres_virtual < var.xres)
+                       var.xres_virtual = var.xres;
+               if (var.yres_virtual < var.yres)
+                       var.yres_virtual = var.yres;
                ret = fb_set_var(info, &var);
                if (!ret)
                        fbcon_update_vcs(info, var.activate & FB_ACTIVATE_ALL);

Given "[PATCH 4/5] fbmem: Prevent invalid virtual screen sizes in
fb_set_var", I don't think we need this patch.

We do.

Why? It will be caught by [PATCH 4/5].

Right, it will be caught by patch #4.
But if you drop this part, then everytime a user runs
        fbset -xres 800 -yres 600 -xvres 200
users will get the KERNEL BUG WARNING (from patch #4) including
a kernel backtrace in their syslogs.

No, they will only see that warning if they are using a broken fbdev
driver that implements .fb_check_var(), but fails to validate or
update the passed geometry.

IMHO this argument is mood.
That way you put pressure on and need such simple code in
each single driver to fix it up, instead of cleaning it up at a central
place.

Most hardware has restrictions on resolution (e.g. xres must be a
multiple of N), so the driver has to round up the resolution to make
it fit.  And after that the driver has to validate and update the
virtual resolution again anyway...

If a driver does not support changing the video mode, it can leave
out the .fb_check_var() and .fb_set_par() callbacks, so the fbdev
core will ignore the userspace-supplied parameters, and reinstate
the single supported mode. See e.g. "[PATCH] drm/fb-helper:
Remove helpers to change frame buffer config"
(https://lore.kernel.org/all/20220629105658.1373770-1-geert@linux-m68k.org (local)).

I implemented all of your suggested changes - from this mail and the others.
I've committed a new testing tree to the fbcon-fix-testing branch at:
https://github.com/hdeller/linux/tree/fbcon-fix-testing
The diff is here:
https://github.com/torvalds/linux/compare/master...hdeller:linux:fbcon-fix-testing

Although your idea is good since we now would find issues in the drivers,
I don't think we want to commit it, since the testcase from
the bug report then immediately crashes the kernel - see below.

That is expected behavior with panic_on_warn?

Oh well. You're right!
The kernel config had panic_on_warn enabled, which I didn't noticed.
I disabled that option and now it works:

[  147.430332][ T3171] WARNING: CPU: 0 PID: 3171 at drivers/video/fbdev/core/fbmem.c:1025 fb_set_var.cold+0x83/0x1bc
....
[  147.431126][ T3171] ---[ end trace 0000000000000000 ]---
[  147.431132][ T3171] fbcon: Fix up invalid yres 0 for bochs-drmdrmfb

The right fix is to fix the broken .fb_check_var() implementation.

Yep.
I'll send this patch series now.

Helge