linux-kernel - Re: [PATCH 1/5] firmware: share fw fallback killing on reboot/suspend

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: <20170427015632.GG28800@wotan.suse.de>
Date:   Thu, 27 Apr 2017 03:56:32 +0200
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     "Coelho, Luciano" <luciano.coelho@...el.com>
Cc:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "mcgrof@...nel.org" <mcgrof@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "pjones@...hat.com" <pjones@...hat.com>,
        "moritz.fischer@...us.com" <moritz.fischer@...us.com>,
        "takahiro.akashi@...aro.org" <takahiro.akashi@...aro.org>,
        "dhowells@...hat.com" <dhowells@...hat.com>,
        "pmladek@...e.com" <pmladek@...e.com>,
        "Berg, Johannes" <johannes.berg@...el.com>,
        "rjw@...ysocki.net" <rjw@...ysocki.net>,
        "yi1.li@...ux.intel.com" <yi1.li@...ux.intel.com>,
        "kvalo@...eaurora.org" <kvalo@...eaurora.org>,
        "luto@...nel.org" <luto@...nel.org>,
        "arend.vanspriel@...adcom.com" <arend.vanspriel@...adcom.com>,
        "rafal@...ecki.pl" <rafal@...ecki.pl>,
        "dwmw2@...radead.org" <dwmw2@...radead.org>,
        "wagi@...om.org" <wagi@...om.org>,
        "atull@...nsource.altera.com" <atull@...nsource.altera.com>,
        "Grumbach, Emmanuel" <emmanuel.grumbach@...el.com>
Subject: Re: [PATCH 1/5] firmware: share fw fallback killing on reboot/suspend

On Thu, Apr 06, 2017 at 06:38:47AM +0000, Coelho, Luciano wrote:
> Hi Luis,
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index ac350c518e0c..d2e2d83aaf26 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -562,16 +562,15 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
> >  
> >  static LIST_HEAD(pending_fw_head);
> >  
> > -/* reboot notifier for avoid deadlock with usermode_lock */
> >  static int fw_shutdown_notify(struct notifier_block *unused1,
> >  			      unsigned long unused2, void *unused3)
> >  {
> > -	mutex_lock(&fw_lock);
> > -	while (!list_empty(&pending_fw_head))
> > -		__fw_load_abort(list_first_entry(&pending_fw_head,
> > -					       struct firmware_buf,
> > -					       pending_list));
> > -	mutex_unlock(&fw_lock);
> > +	/*
> > +	 * Kill all pending fallback requests to avoid both stalling shutdown,
> > +	 * and avoid a deadlock with the usermode_lock.
> > +	 */
> > +	kill_pending_fw_fallback_reqs(false);
> 
> You are calling this function before you declare it, this won't compile
> when CONFIG_FW_LOADER_USER_HELPER is not set.

Actually allnoconfig compiles fine given the code in question is needed only when
CONFIG_FW_LOADER_USER_HELPER is loaded, however it does fail to compile when
CONFIG_FW_LOADER_USER_HELPER is set, and its odd that 0-day did not pick that up.

I've fixed this by moving kill_pending_fw_fallback_reqs() up above in a
separate patch first.

> > @@ -1048,21 +1047,20 @@ static int fw_load_from_user_helper(struct firmware *firmware,
> >  	return _request_firmware_load(fw_priv, opt_flags, timeout);
> >  }
> >  
> > -#ifdef CONFIG_PM_SLEEP
> > -/* kill pending requests without uevent to avoid blocking suspend */
> > -static void kill_requests_without_uevent(void)
> > +static void kill_pending_fw_fallback_reqs(bool only_kill_custom)
> >  {
> >  	struct firmware_buf *buf;
> >  	struct firmware_buf *next;
> >  
> >  	mutex_lock(&fw_lock);
> >  	list_for_each_entry_safe(buf, next, &pending_fw_head, pending_list) {
> > -		if (!buf->need_uevent)
> > +		if (!only_kill_custom)
> > +			 __fw_load_abort(buf);
> > +		else if (!buf->need_uevent)
> >  			 __fw_load_abort(buf);
> Why not use this?
> 
> 	if (!only_kill_custom || !buf->need_uevent)
> 		__fw_load_abort(buf);

Sure thing! Thanks for the review !

  Luis