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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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..




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux