[PATCH 2/5] fbcon: Fix up user-provided virtual screen size

[Helge Deller]

On 7/1/22 16:52, Geert Uytterhoeven wrote:
> Hi Helge,
>
> On Fri, Jul 1, 2022 at 11:30 AM Helge Deller <deller at gmx.de> wrote:
>> On 7/1/22 09:28, Geert Uytterhoeven wrote:
>>> On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller at gmx.de> wrote:
>>>> On 6/30/22 22:00, Geert Uytterhoeven wrote:
>>>>> On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller at gmx.de> wrote:
>>>>>> On 6/30/22 21:36, Geert Uytterhoeven wrote:
>>>>>>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller at gmx.de> wrote:
>>>>>>>> On 6/30/22 21:00, Geert Uytterhoeven wrote:
>>>>>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller at gmx.de> wrote:
>>>>>>>>>> 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 at gmx.de>
>>>>>>>>>> Cc: stable at vger.kernel.org # v5.4+
>>>>>>>>>
>>>>>>>>> Thanks for your patch!
>>>>>>>>>
>>>>>>>>>> --- 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).
>>
>> 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