Thomas Zimmermann <[email protected]> writes: > Hi > > Am 17.11.25 um 11:32 schrieb Javier Martinez Canillas: >> Thomas Zimmermann <[email protected]> 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 <[email protected]> >>> 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 <[email protected]> >> >> 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
