linux-kernel - Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 5 May 2022 10:05:33 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        linux-kernel@...r.kernel.org
Cc:     Daniel Vetter <daniel.vetter@...ll.ch>,
        linux-fbdev@...r.kernel.org, Helge Deller <deller@....de>,
        dri-devel@...ts.freedesktop.org,
        Hans de Goede <hdegoede@...hat.com>
Subject: Re: [PATCH 2/3] fbdev/simplefb: Cleanup fb_info in .fb_destroy rather
 than .remove

Hi

Am 05.05.22 um 09:38 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 5/5/22 09:29, Thomas Zimmermann wrote:
> 
> [snip]
> 
>>>    static void simplefb_destroy(struct fb_info *info)
>>>    {
>>>    	struct simplefb_par *par = info->par;
>>> @@ -94,6 +98,8 @@ static void simplefb_destroy(struct fb_info *info)
>>>    	if (info->screen_base)
>>>    		iounmap(info->screen_base);
>>>    
>>> +	framebuffer_release(info);
>>> +
>>>    	if (mem)
>>>    		release_mem_region(mem->start, resource_size(mem));
>>
>> The original problem with fbdev hot-unplug was that vmwgfx needed the
>> framebuffer region to be released. If we release it only after userspace
>> closed it's final file descriptor, vmwgfx could have already failed.
>>
>> I still don't fully get why this code apparently works or at least
>> doesn't blow up occasionally. Any ideas?
>>
> 
> I believe that vmwgfx doesn't fail to probe (or any other DRM driver)
> only when there are not user-space processes with a fbdev node opened
> since otherwise as you said the memory wouldn't be released yet.
> 
> unregister_framebuffer() is called from the driver's .remove handler
> and that decrement the fb_info refcount, so if reaches zero it will
> call to the fb fops .destroy() handler and release the I/O memory.
> 
> In other words, in most cases (i.e: only fbcon bound to the fbdev)
> the driver's removal/ device unbind and the memory release will be
> at the same time.
> 

We're one the same page here, but it's still sort of a mystery to me why 
this works in practice.

I'm specifically talking about pci_request_regions() in vmwgfx [1]. IIRC 
this would fail if simplefb still owns the framebuffer region. Lots of 
systems run Plymouth during boot and this should result in failures 
occasionally. Still, we never heard about anything.

Of course, it's always been broken (even long before real fbdev 
hotunplugging). Switching to simpledrm resolves the problem.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v5.17.5/source/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c#L727

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)