Re: [PATCH] drm, fbcon, vga_switcheroo: Avoid race condition in fbcon setup

[Javier Martinez Canillas <javierm@redhat.com>]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 17.11.25 um 11:32 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>>
>> Hello Thomas,
>>
>>> Protect vga_switcheroo_client_fb_set() with console lock. Avoids OOB
>>> access in fbcon_remap_all(). Without holding the console lock the call
>>> races with switching outputs.
>>>
>>> VGA switcheroo calls fbcon_remap_all() when switching clients. The fbcon
>>> function uses struct fb_info.node, which is set by register_framebuffer().
>>> As the fb-helper code currently sets up VGA switcheroo before registering
>>> the framebuffer, the value of node is -1 and therefore not a legal value.
>>> For example, fbcon uses the value within set_con2fb_map() [1] as an index
>>> into an array.
>>>
>>> Moving vga_switcheroo_client_fb_set() after register_framebuffer() can
>>> result in VGA switching that does not switch fbcon correctly.
>>>
>>> Therefore move vga_switcheroo_client_fb_set() under fbcon_fb_registered(),
>>> which already holds the console lock. Fbdev calls fbcon_fb_registered()
>>> from within register_framebuffer(). Serializes the helper with VGA
>>> switcheroo's call to fbcon_remap_all().
>>>
>>> Although vga_switcheroo_client_fb_set() takes an instance of struct fb_info
>>> as parameter, it really only needs the contained fbcon state. Moving the
>>> call to fbcon initialization is therefore cleaner than before. Only amdgpu,
>>> i915, nouveau and radeon support vga_switcheroo. For all other drivers,
>>> this change does nothing.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Link: https://elixir.bootlin.com/linux/v6.17/source/drivers/video/fbdev/core/fbcon.c#L2942 # [1]
>>> ---
>> I'm not that familiar with fbcon and vga_switcheroo to properly review
>> your patch but after reading the explanation in the commit message and
>> reading the diff, the change does make sense to me.
>>
>> Acked-by: Javier Martinez Canillas <javierm@redhat.com>
>>
>> But I think that would be good if you get some testing for the drivers
>> that make use of vga_switcheroo. Also, do you need a Fixes tag ?
>
> I've ran the testing on amdgpu and i915 so that nothing breaks. The bug 
> is hard to reproduce though. I've discovered it by reading the code.
>

Thanks.

I usually put that kind of information between the --- separator and the
start of the diff. Since that info can be useful for reviewers and doesn't
end in the commited patch, due tools like `git am` omitting that section.

> About Fixes, the problem has been in the code forever. So IDK what Fixes 
> would make sense. Just in case:
>

I see. Then I agree that having the tag is less useful.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat