Hi Helge, On Fri, Jul 1, 2022 at 11:30 AM Helge Deller <deller@xxxxxx> wrote: > On 7/1/22 09:28, Geert Uytterhoeven wrote: > > On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller@xxxxxx> wrote: > >> On 6/30/22 22:00, Geert Uytterhoeven wrote: > >>> On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@xxxxxx> wrote: > >>>> On 6/30/22 21:36, Geert Uytterhoeven wrote: > >>>>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@xxxxxx> wrote: > >>>>>> On 6/30/22 21:00, Geert Uytterhoeven wrote: > >>>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@xxxxxx> 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@xxxxxx> > >>>>>>>> Cc: stable@xxxxxxxxxxxxxxx # 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@xxxxxxxxxxxxxx). > > 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? The right fix is to fix the broken .fb_check_var() implementation. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
