Re: [PATCH] fbcon: Fix delayed takeover locking

From: Daniel Vetter <daniel@ffwll.ch>
To: Javier Martinez Canillas <javierm@redhat.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Du Cheng <ducheng2@gmail.com>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Zheyu Ma <zheyuma97@gmail.com>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Claudio Suarez <cssk@net-c.es>,
	Matthew Wilcox <willy@infradead.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Sam Ravnborg <sam@ravnborg.org>, Helge Deller <deller@gmx.de>,
	Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH] fbcon: Fix delayed takeover locking
Date: Wed, 13 Apr 2022 11:21:23 +0200	[thread overview]
Message-ID: <YlaWE+/YBtcrnLHY@phenom.ffwll.local> (raw)
In-Reply-To: <955400a5-a785-57af-92d4-42710c55ce99@redhat.com>

On Wed, Apr 13, 2022 at 11:16:08AM +0200, Javier Martinez Canillas wrote:
> Hello Daniel,
> 
> On 4/13/22 10:21, Daniel Vetter wrote:
> > I messed up the delayed takover path in the locking conversion in
> > 6e7da3af008b ("fbcon: Move console_lock for register/unlink/unregister").
> >
> 
> Maybe a few more words of what the issue is ? Something like the following:
> 
> If CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER is enabled, fbcon take-over
> doesn't take place when calling fbcon_fb_registered(). Instead, is deferred
> using a workqueue and its fbcon_register_existing_fbs() function calls to
> fbcon_fb_registered() again for each registered fbcon fb.
> 
> This leads to the console_lock tried to be held twice, causing a deadlock.

Will add.
>  
> > Fix it by re-extracting the lockless function and using it in the
> > delayed takeover path, where we need to hold the lock already to
> > iterate over the list of already registered fb. Well the current code
> > still is broken in there (since the list is protected by a
> > registration_lock, which we can't take here because it nests the other
> > way round with console_lock), but in the future this will be a list
> > protected by console_lock when this is all sorted out.
> > 
> 
> [snip]
> 
> >  
> > -/* called with console_lock held */
> >  void fbcon_fb_unbind(struct fb_info *info)
> >  {
> >  	int i, new_idx = -1;
> > @@ -2822,7 +2821,6 @@ void fbcon_fb_unbind(struct fb_info *info)
> >  	console_unlock();
> >  }
> >  
> > -/* called with console_lock held */
> >  void fbcon_fb_unregistered(struct fb_info *info)
> >  {
> 
> Removing these comments feels like should be in a separate patch or at least
> mention in the patch description that should had been removed in the commit
> 6e7da3af008b ("fbcon: Move console_lock for register/unlink/unregister"),
> that made these functions to be called without the console_lock being held.

Yeah I forgot to mention that in the commit message - while reviewing all
the locking to figure out the bug I also noticed that I didn't update the
comments, and since it's all issues in the same patch I figured I'll do it
all in one go.

Ok if I explain that and then keep the comment removals?
-Daniel
> 
> The changes themselves look good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch