linux-kernel - Re: [PATCH v3 3/5] fbdev: Restart conflicting fb removal loop when unregistering devices

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]

Message-ID: <44804419-0e83-b584-96d9-a35939b715bf@suse.de>
Date:   Mon, 25 Apr 2022 10:27:00 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        linux-kernel@...r.kernel.org
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        dri-devel@...ts.freedesktop.org,
        Alex Deucher <alexander.deucher@....com>,
        Changcheng Deng <deng.changcheng@....com.cn>,
        Daniel Vetter <daniel@...ll.ch>, Helge Deller <deller@....de>,
        Sam Ravnborg <sam@...nborg.org>,
        Zhen Lei <thunder.leizhen@...wei.com>,
        linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v3 3/5] fbdev: Restart conflicting fb removal loop when
 unregistering devices

Hi

Am 20.04.22 um 10:53 schrieb Javier Martinez Canillas:
> Drivers that want to remove registered conflicting framebuffers prior to
> register their own framebuffer, calls remove_conflicting_framebuffers().
> 
> This function takes the registration_lock mutex, to prevent a races when
> drivers register framebuffer devices. But if a conflicting framebuffer
> device is found, the underlaying platform device is unregistered and this
> will lead to the platform driver .remove callback to be called, which in
> turn will call to the unregister_framebuffer() that takes the same lock.
> 
> To prevent this, a struct fb_info.forced_out field was used as indication
> to unregister_framebuffer() whether the mutex has to be grabbed or not.
> 
> A cleaner solution is to drop the lock before platform_device_unregister()
> so unregister_framebuffer() can take it when called from the fbdev driver,
> and just grab the lock again after the device has been registered and do
> a removal loop restart.

I don't see how this patch improves the situation. So far, 
do_remove_conflicting_framebuffers() had no business in maintaining 
locks. And now it's doing this in in a goto-loop where it keeps 
getting/dropping locks.  That's asking for bugs IMHO.

Best regards
Thomas

> 
> Since the framebuffer devices will already be removed, the loop would just
> finish when no more conflicting framebuffers are found.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@...ll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@...ll.ch>
> ---
> 
> (no changes since v1)
> 
>   drivers/video/fbdev/core/fbmem.c | 22 +++++++++++++++-------
>   include/linux/fb.h               |  1 -
>   2 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 84427470367b..0bb459258df3 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1553,6 +1553,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
>   {
>   	int i;
>   
> +restart_removal:
>   	/* check all firmware fbs and kick off if the base addr overlaps */
>   	for_each_registered_fb(i) {
>   		struct apertures_struct *gen_aper;
> @@ -1585,12 +1586,23 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
>   				pr_warn("fb%d: no device set\n", i);
>   				do_unregister_framebuffer(registered_fb[i]);
>   			} else if (dev_is_platform(device)) {
> -				registered_fb[i]->forced_out = true;
> +				/*
> +				 * Drop the lock because if the device is unregistered, its
> +				 * driver will call to unregister_framebuffer(), that takes
> +				 * this lock.
> +				 */
> +				mutex_unlock(&registration_lock);
>   				platform_device_unregister(to_platform_device(device));
> +				mutex_lock(&registration_lock);
>   			} else {
>   				pr_warn("fb%d: cannot remove device\n", i);
>   				do_unregister_framebuffer(registered_fb[i]);
>   			}
> +			/*
> +			 * Restart the removal loop now that the device has been
> +			 * unregistered and its associated framebuffer gone.
> +			 */
> +			goto restart_removal;
>   		}
>   	}
>   }
> @@ -1897,13 +1909,9 @@ EXPORT_SYMBOL(register_framebuffer);
>   void
>   unregister_framebuffer(struct fb_info *fb_info)
>   {
> -	bool forced_out = fb_info->forced_out;
> -
> -	if (!forced_out)
> -		mutex_lock(&registration_lock);
> +	mutex_lock(&registration_lock);
>   	do_unregister_framebuffer(fb_info);
> -	if (!forced_out)
> -		mutex_unlock(&registration_lock);
> +	mutex_unlock(&registration_lock);
>   }
>   EXPORT_SYMBOL(unregister_framebuffer);
>   
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index f95da1af9ff6..b781bc721113 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -502,7 +502,6 @@ struct fb_info {
>   	} *apertures;
>   
>   	bool skip_vt_switch; /* no VT switch on suspend/resume required */
> -	bool forced_out; /* set when being removed by another driver */
>   };
>   
>   static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {

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