| Message ID | 20220629200024.187187-3-deller@gmx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fbcon: Fixes for screen resolution changes - round 2 | expand |
Hi Helge, On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@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@gmx.de> > Cc: stable@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. Moreover, this patch will prevent triggering the WARN_ON()s in [PATCH 4/5] in the most common buggy case of drivers plainly ignoring var.[xy]res_virtual. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On 6/30/22 21:00, Geert Uytterhoeven wrote: > Hi Helge, > > On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@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@gmx.de> >> Cc: stable@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. > Moreover, this > patch will prevent triggering the WARN_ON()s in [PATCH 4/5] Right. > in the most common buggy case of drivers plainly ignoring var.[xy]res_virtual. In summary: This patch #2 is fixing up user-space invalid input and is not allowed to trigger any WARN_ON(). We could drop patch #4, but then we wouldn't catch bad drivers. Helge
Hi Helge, On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote: > On 6/30/22 21:00, Geert Uytterhoeven wrote: > > On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@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@gmx.de> > >> Cc: stable@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]. > > Moreover, this > > patch will prevent triggering the WARN_ON()s in [PATCH 4/5] > > Right. > > > in the most common buggy case of drivers plainly ignoring var.[xy]res_virtual. > > In summary: > This patch #2 is fixing up user-space invalid input and is not > allowed to trigger any WARN_ON(). It's the responsibility of the driver to at least look at its parameters. What other invalid values might it let pass, that we cannot catch at the generic level? > We could drop patch #4, but then we wouldn't catch bad drivers. I do want to keep patch #4. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On 6/30/22 21:36, Geert Uytterhoeven wrote: > Hi Helge, > > On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote: >> On 6/30/22 21:00, Geert Uytterhoeven wrote: >>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@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@gmx.de> >>>> Cc: stable@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. This is not what you want. If you drop the WARN() from patch #4 you'll end up with my last patch series. >>> Moreover, this >>> patch will prevent triggering the WARN_ON()s in [PATCH 4/5] >> >> Right. >> >>> in the most common buggy case of drivers plainly ignoring var.[xy]res_virtual. >> >> In summary: >> This patch #2 is fixing up user-space invalid input and is not >> allowed to trigger any WARN_ON(). > > It's the responsibility of the driver to at least look at its parameters. > What other invalid values might it let pass, that we cannot catch > at the generic level? > >> We could drop patch #4, but then we wouldn't catch bad drivers. > > I do want to keep patch #4. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > 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
Hi Helge, On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote: > On 6/30/22 21:36, Geert Uytterhoeven wrote: > > On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote: > >> On 6/30/22 21:00, Geert Uytterhoeven wrote: > >>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@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@gmx.de> > >>>> Cc: stable@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. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On 6/30/22 22:00, Geert Uytterhoeven wrote: > Hi Helge, > > On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote: >> On 6/30/22 21:36, Geert Uytterhoeven wrote: >>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote: >>>> On 6/30/22 21:00, Geert Uytterhoeven wrote: >>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@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@gmx.de> >>>>>> Cc: stable@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. Helge
Hi Helge, On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller@gmx.de> wrote: > On 6/30/22 22:00, Geert Uytterhoeven wrote: > > On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote: > >> On 6/30/22 21:36, Geert Uytterhoeven wrote: > >>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote: > >>>> On 6/30/22 21:00, Geert Uytterhoeven wrote: > >>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@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@gmx.de> > >>>>>> Cc: stable@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). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
Hi Geert, On 7/1/22 09:28, Geert Uytterhoeven wrote: > On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller@gmx.de> wrote: >> On 6/30/22 22:00, Geert Uytterhoeven wrote: >>> On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote: >>>> On 6/30/22 21:36, Geert Uytterhoeven wrote: >>>>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote: >>>>>> On 6/30/22 21:00, Geert Uytterhoeven wrote: >>>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@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@gmx.de> >>>>>>>> Cc: stable@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. I think we need to fix up earlier. Your other patch to disable DRM's set_fb_var() might fix this specific issue, but in general we may face other problems in other drivers too. Thoughts? Helge root@debian:~# ./a.out [ 44.118212][ T3081] ------------[ cut here ]------------ [ 44.118298][ T3081] WARNING: CPU: 2 PID: 3081 at drivers/video/fbdev/core/fbmem.c:1020 fb_set_var.cold+0x10d/0x1bc [ 44.118376][ T3081] Modules linked in: [ 44.118401][ T3081] CPU: 2 PID: 3081 Comm: a.out Not tainted 5.19.0-rc4-00004-g11dd75029515 #17 [ 44.118432][ T3081] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014 [ 44.118453][ T3081] RIP: 0010:fb_set_var.cold+0x10d/0x1bc [ 44.118709][ T3081] Call Trace: [ 44.118719][ T3081] <TASK> [ 44.118731][ T3081] ? fb_blank+0x190/0x190 [ 44.118784][ T3081] ? rcu_read_lock_sched_held+0x3a/0x70 [ 44.118816][ T3081] ? trace_contention_end+0xea/0x150 [ 44.118845][ T3081] ? __mutex_lock+0x259/0x1450 [ 44.118875][ T3081] ? do_fb_ioctl+0x2fd/0x6f0 [ 44.118906][ T3081] ? mutex_lock_io_nested+0x1260/0x1260 [ 44.118936][ T3081] ? lock_downgrade+0x6e0/0x6e0 [ 44.118966][ T3081] ? rwlock_bug.part.0+0x90/0x90 [ 44.118998][ T3081] ? _raw_spin_lock_irqsave+0x4e/0x50 [ 44.119031][ T3081] ? is_console_locked+0x5/0x10 [ 44.119060][ T3081] ? fbcon_info_from_console+0x61/0x190 [ 44.119087][ T3081] ? fbcon_modechange_possible+0x359/0x4c0 [ 44.119116][ T3081] do_fb_ioctl+0x63b/0x6f0 [ 44.119146][ T3081] ? putname+0xfe/0x140 [ 44.119174][ T3081] ? fb_set_suspend+0x1a0/0x1a0 [ 44.119204][ T3081] ? path_openat+0x1016/0x2810 [ 44.119234][ T3081] ? mark_lock.part.0+0xfc/0x1a00 [ 44.119266][ T3081] ? lock_chain_count+0x20/0x20 [ 44.119297][ T3081] ? lock_chain_count+0x20/0x20 [ 44.119326][ T3081] ? lock_downgrade+0x6e0/0x6e0 [ 44.119358][ T3081] ? _raw_spin_unlock_irqrestore+0x50/0x70 [ 44.119396][ T3081] fb_ioctl+0xe7/0x150 [ 44.119424][ T3081] ? do_fb_ioctl+0x6f0/0x6f0 [ 44.119454][ T3081] __x64_sys_ioctl+0x94c/0x18a0 [ 44.119487][ T3081] ? vfs_fileattr_set+0xb70/0xb70 [ 44.119520][ T3081] ? find_held_lock+0x2d/0x110 [ 44.119550][ T3081] ? __context_tracking_exit+0xb8/0xe0 [ 44.119582][ T3081] ? lock_downgrade+0x6e0/0x6e0 [ 44.119613][ T3081] ? lock_downgrade+0x6e0/0x6e0 [ 44.119645][ T3081] ? syscall_enter_from_user_mode+0x21/0x70 [ 44.119678][ T3081] ? syscall_enter_from_user_mode+0x21/0x70 [ 44.119711][ T3081] do_syscall_64+0x35/0x80 [ 44.119751][ T3081] entry_SYSCALL_64_after_hwframe+0x46/0xb0 [ 44.119786][ T3081] RIP: 0033:0x7ffb0251a9b9 [ 44.119816][ T3081] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 54 0c 00 f7 d8 64 89 01 48 [ 44.119842][ T3081] RSP: 002b:00007ffcce124e78 EFLAGS: 00000287 ORIG_RAX: 0000000000000010 [ 44.119871][ T3081] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007ffb0251a9b9 [ 44.119890][ T3081] RDX: 00000000200001c0 RSI: 0000000000004601 RDI: 0000000000000004 [ 44.119909][ T3081] RBP: 00007ffcce124e90 R08: 00007ffcce124e90 R09: 00007ffcce124e90 [ 44.119928][ T3081] R10: 00007ffcce124e90 R11: 0000000000000287 R12: 000055b9dba381d0 [ 44.119947][ T3081] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 44.119968][ T3081] </TASK> [ 44.119980][ T3081] Kernel panic - not syncing: panic_on_warn set ... [ 44.119991][ T3081] CPU: 2 PID: 3081 Comm: a.out Not tainted 5.19.0-rc4-00004-g11dd75029515 #17 [ 44.120017][ T3081] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014 [ 44.120031][ T3081] Call Trace: [ 44.120037][ T3081] <TASK> [ 44.120046][ T3081] dump_stack_lvl+0xcd/0x134 [ 44.120075][ T3081] panic+0x2d3/0x632 [ 44.120099][ T3081] ? panic_print_sys_info.part.0+0x10b/0x10b [ 44.120128][ T3081] ? __warn.cold+0x1d1/0x2c5 [ 44.120153][ T3081] ? fb_set_var.cold+0x10d/0x1bc [ 44.120176][ T3081] __warn.cold+0x1e2/0x2c5 [ 44.120200][ T3081] ? fb_set_var.cold+0x10d/0x1bc [ 44.120224][ T3081] report_bug+0x1c0/0x210 [ 44.120253][ T3081] handle_bug+0x3c/0x60 [ 44.120277][ T3081] exc_invalid_op+0x14/0x40 [ 44.120303][ T3081] asm_exc_invalid_op+0x1b/0x20 [ 44.120326][ T3081] RIP: 0010:fb_set_var.cold+0x10d/0x1bc [ 44.120352][ T3081] Code: b6 14 02 48 89 f0 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 92 00 00 00 89 4d 0c e9 41 d8 5a fc 44 89 44 24 08 e8 3f b2 3a fb <0f> 0b 8b 74 24 08 49 8d 94 24 d0 01 00 00 48 c7 c7 60 b9 d9 86 e8 [ 44.120375][ T3081] RSP: 0018:ffffc900019d7808 EFLAGS: 00010293 [ 44.120395][ T3081] RAX: 0000000000000000 RBX: 1ffff9200033af07 RCX: 0000000000000000 [ 44.120412][ T3081] RDX: ffff888043a39c00 RSI: ffffffff863b5621 RDI: 0000000000000004 [ 44.120429][ T3081] RBP: ffffc900019d7be0 R08: 0000000000000000 R09: 0000000000000000 [ 44.120445][ T3081] R10: 0000000000000340 R11: 0000000000000000 R12: ffff888041abc000 [ 44.120462][ T3081] R13: ffffc900019d7c34 R14: 0000000000000000 R15: 0000000000000080 [ 44.120480][ T3081] ? fb_set_var.cold+0x10d/0x1bc [ 44.120505][ T3081] ? fb_set_var.cold+0x10d/0x1bc [ 44.120529][ T3081] ? fb_blank+0x190/0x190 [ 44.120558][ T3081] ? rcu_read_lock_sched_held+0x3a/0x70 [ 44.120586][ T3081] ? trace_contention_end+0xea/0x150 [ 44.120612][ T3081] ? __mutex_lock+0x259/0x1450 [ 44.120639][ T3081] ? do_fb_ioctl+0x2fd/0x6f0 [ 44.120667][ T3081] ? mutex_lock_io_nested+0x1260/0x1260 [ 44.120695][ T3081] ? lock_downgrade+0x6e0/0x6e0 [ 44.120723][ T3081] ? rwlock_bug.part.0+0x90/0x90 [ 44.120777][ T3081] ? _raw_spin_lock_irqsave+0x4e/0x50 [ 44.120809][ T3081] ? is_console_locked+0x5/0x10 [ 44.120835][ T3081] ? fbcon_info_from_console+0x61/0x190 [ 44.120860][ T3081] ? fbcon_modechange_possible+0x359/0x4c0 [ 44.120887][ T3081] do_fb_ioctl+0x63b/0x6f0 [ 44.120914][ T3081] ? putname+0xfe/0x140 [ 44.120940][ T3081] ? fb_set_suspend+0x1a0/0x1a0 [ 44.120968][ T3081] ? path_openat+0x1016/0x2810 [ 44.120995][ T3081] ? mark_lock.part.0+0xfc/0x1a00 [ 44.121025][ T3081] ? lock_chain_count+0x20/0x20 [ 44.121054][ T3081] ? lock_chain_count+0x20/0x20 [ 44.121081][ T3081] ? lock_downgrade+0x6e0/0x6e0 [ 44.121110][ T3081] ? _raw_spin_unlock_irqrestore+0x50/0x70 [ 44.121146][ T3081] fb_ioctl+0xe7/0x150 [ 44.121173][ T3081] ? do_fb_ioctl+0x6f0/0x6f0 [ 44.121200][ T3081] __x64_sys_ioctl+0x94c/0x18a0 [ 44.121231][ T3081] ? vfs_fileattr_set+0xb70/0xb70 [ 44.121261][ T3081] ? find_held_lock+0x2d/0x110 [ 44.121289][ T3081] ? __context_tracking_exit+0xb8/0xe0 [ 44.121318][ T3081] ? lock_downgrade+0x6e0/0x6e0 [ 44.121347][ T3081] ? lock_downgrade+0x6e0/0x6e0 [ 44.121377][ T3081] ? syscall_enter_from_user_mode+0x21/0x70 [ 44.121407][ T3081] ? syscall_enter_from_user_mode+0x21/0x70 [ 44.121439][ T3081] do_syscall_64+0x35/0x80 [ 44.121464][ T3081] entry_SYSCALL_64_after_hwframe+0x46/0xb0 [ 44.121497][ T3081] RIP: 0033:0x7ffb0251a9b9 [ 44.121515][ T3081] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a7 54 0c 00 f7 d8 64 89 01 48 [ 44.121539][ T3081] RSP: 002b:00007ffcce124e78 EFLAGS: 00000287 ORIG_RAX: 0000000000000010 [ 44.121564][ T3081] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007ffb0251a9b9 [ 44.121581][ T3081] RDX: 00000000200001c0 RSI: 0000000000004601 RDI: 0000000000000004 [ 44.121597][ T3081] RBP: 00007ffcce124e90 R08: 00007ffcce124e90 R09: 00007ffcce124e90 [ 44.121614][ T3081] R10: 00007ffcce124e90 R11: 0000000000000287 R12: 000055b9dba381d0 [ 44.121631][ T3081] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 44.121650][ T3081] </TASK> [ 44.122149][ T3081] Kernel Offset: disabled [ 44.291225][ T3081] Rebooting in 86400 seconds..
Hi Helge, On Fri, Jul 1, 2022 at 11:30 AM Helge Deller <deller@gmx.de> wrote: > On 7/1/22 09:28, Geert Uytterhoeven wrote: > > On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller@gmx.de> wrote: > >> On 6/30/22 22:00, Geert Uytterhoeven wrote: > >>> On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote: > >>>> On 6/30/22 21:36, Geert Uytterhoeven wrote: > >>>>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote: > >>>>>> On 6/30/22 21:00, Geert Uytterhoeven wrote: > >>>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@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@gmx.de> > >>>>>>>> Cc: stable@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? 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@linux-m68k.org 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
On 7/1/22 16:52, Geert Uytterhoeven wrote: > Hi Helge, > > On Fri, Jul 1, 2022 at 11:30 AM Helge Deller <deller@gmx.de> wrote: >> On 7/1/22 09:28, Geert Uytterhoeven wrote: >>> On Thu, Jun 30, 2022 at 10:10 PM Helge Deller <deller@gmx.de> wrote: >>>> On 6/30/22 22:00, Geert Uytterhoeven wrote: >>>>> On Thu, Jun 30, 2022 at 9:46 PM Helge Deller <deller@gmx.de> wrote: >>>>>> On 6/30/22 21:36, Geert Uytterhoeven wrote: >>>>>>> On Thu, Jun 30, 2022 at 9:31 PM Helge Deller <deller@gmx.de> wrote: >>>>>>>> On 6/30/22 21:00, Geert Uytterhoeven wrote: >>>>>>>>> On Wed, Jun 29, 2022 at 10:00 PM Helge Deller <deller@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@gmx.de> >>>>>>>>>> Cc: stable@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
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index afa2863670f3..e645b96d2700 100644 --- 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);
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+ --- drivers/video/fbdev/core/fbmem.c | 5 +++++ 1 file changed, 5 insertions(+) -- 2.35.3