Hi Geert, 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. 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..
