linux-kernel - Re: [PATCH v2 19/23] firmware: add debug facility to emulate forcing sysfs fallback

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, 30 Nov 2017 21:35:16 +0100
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>, akpm@...ux-foundation.org,
        keescook@...omium.org, mfuzzey@...keon.com,
        zohar@...ux.vnet.ibm.com, dhowells@...hat.com,
        pali.rohar@...il.com, tiwai@...e.de, arend.vanspriel@...adcom.com,
        zajec5@...il.com, nbroeking@...com, markivx@...eaurora.org,
        stephen.boyd@...aro.org, broonie@...nel.org,
        dmitry.torokhov@...il.com, dwmw2@...radead.org,
        torvalds@...ux-foundation.org, Abhay_Salunke@...l.com,
        bjorn.andersson@...aro.org, jewalt@...innovations.com,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2 19/23] firmware: add debug facility to emulate forcing
 sysfs fallback

On Wed, Nov 29, 2017 at 11:28:04AM +0100, Greg KH wrote:
> On Mon, Nov 20, 2017 at 10:24:05AM -0800, Luis R. Rodriguez wrote:
> > diff --git a/drivers/base/firmware_debug.c b/drivers/base/firmware_debug.c
> > new file mode 100644
> > index 000000000000..f2817eb6f480
> > --- /dev/null
> > +++ b/drivers/base/firmware_debug.c
> > @@ -0,0 +1,34 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Firmware dubugging interface */
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/debugfs.h>
> > +#include "firmware_debug.h"
> > +
> > +struct firmware_debug fw_debug;
> > +
> > +static struct dentry *debugfs_firmware;
> > +
> > +int __init register_fw_debugfs(void)
> > +{
> > +	debugfs_firmware = debugfs_create_dir("firmware", NULL);
> > +	if (!debugfs_firmware)
> > +		return -ENOMEM;
> 
> You never need to check the return value of a debugfs call, you should
> not care about it, nor do anything different in your code.  The value
> returned can always be passed back into any other debugfs call when
> needed.

Neat, so all uses as in the above are wrong eh?

> > +
> > +	if (!debugfs_create_bool("force_sysfs_fallback", S_IRUSR | S_IWUSR,
> > +				 debugfs_firmware,
> > +				 &fw_debug.force_sysfs_fallback))
> 
> Same here, you don't care.

OK!

> 
> > +		goto err_out;
> > +
> > +	return 0;
> > +err_out:
> > +	debugfs_remove_recursive(debugfs_firmware);
> 
> You didn't create any files, why recursive?
> Anyway, not needed.

Wouldn't this take care of removing the two bool files I add in one shot later?

> > +	debugfs_firmware = NULL;
> > +	return -ENOMEM;
> > +}
> > +
> > +void unregister_fw_debugfs(void)
> > +{
> > +	debugfs_remove_recursive(debugfs_firmware);
> > +	debugfs_firmware = NULL;
> 
> Why set this to NULL?

OK, will avoid.

> > diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
> > index 43b97a8137f7..b2b52ba9f245 100644
> > --- a/drivers/base/firmware_loader.c
> > +++ b/drivers/base/firmware_loader.c
> > @@ -36,6 +36,7 @@
> >  #include <generated/utsrelease.h>
> >  
> >  #include "base.h"
> > +#include "firmware_debug.h"
> >  
> >  MODULE_AUTHOR("Manuel Estrada Sainz");
> >  MODULE_DESCRIPTION("Multi purpose firmware loading support");
> > @@ -1158,6 +1159,9 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
> >  #else
> >  static bool fw_force_sysfs_fallback(unsigned int opt_flags)
> >  {
> > +	if (fw_debug_force_sysfs_fallback())
> > +		return true;
> > +
> >  	if (!(opt_flags & FW_OPT_USERHELPER))
> >  		return false;
> >  	return true;
> > @@ -1913,10 +1917,14 @@ static int __init firmware_class_init(void)
> >  	/* No need to unfold these on exit */
> >  	fw_cache_init();
> >  
> > -	ret = register_fw_pm_ops();
> > +	ret = register_fw_debugfs();
> >  	if (ret)
> >  		return ret;
> 
> Again you don't care about the state of debugfs.  Did you test this on a
> system without CONFIG_DEBUGFS enabled?

Hrm, nope. Will fix it up. Thanks for the review.

  Luis