linux-kernel - Re: [PATCH] firmware_loader: move CONFIG_FW_LOADER_USER

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] [day] [month] [year] [list]

Message-ID: <20190205002708.GU11489@garbanzo.do-not-panic.com>
Date:   Mon, 4 Feb 2019 16:27:08 -0800
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>
Subject: Re: [PATCH] firmware_loader: move CONFIG_FW_LOADER_USER_HELPER
 switch to Makefile

On Tue, Dec 25, 2018 at 12:14:57AM +0900, Masahiro Yamada wrote:
> The whole code of fallback_table.c is surrounded by #ifdef of
> CONFIG_FW_LOADER_USER_HELPER.
> 
> Move the CONFIG_FW_LOADER_USER_HELPER switch to Makefile so that
> it is not compiled at all when this CONFIG is turned off.
> 
> I also removed the confusing comment, "Module or buit-in [sic]".
> CONFIG_FW_LOADER_USER_HELPER is a boolean option.
> (If it were a module, CONFIG_FW_LOADER_USER_HELPER_MODULE would
> be defined instead.)
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>
> ---
> 
>  drivers/base/firmware_loader/Makefile         | 2 +-
>  drivers/base/firmware_loader/fallback_table.c | 5 -----
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/base/firmware_loader/Makefile b/drivers/base/firmware_loader/Makefile
> index a97eeb0..5c7f422 100644
> --- a/drivers/base/firmware_loader/Makefile
> +++ b/drivers/base/firmware_loader/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Makefile for the Linux firmware loader
>  
> -obj-y			:= fallback_table.o
> +obj-$(CONFIG_FW_LOADER_USER_HELPER) += fallback_table.o

Phew ... CONFIG_FW_LOADER_USER_HELPER is bool.

So no new modules. You had me scared for a bit.

The reason this was built-in was that we *need* this table for built-in
code which *can* request firmware *pre* module loading. The table thing
was designed to be built-in if the firmware interface was enabled in
any way.

Fortunately I don't see this regressing as CONFIG_FW_LOADER_USER_HELPER
is a bool. It is hard to enforce other than with careful review.

>  obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
>  firmware_class-objs := main.o
>  firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
> diff --git a/drivers/base/firmware_loader/fallback_table.c b/drivers/base/firmware_loader/fallback_table.c
> index 7428659..776dd69 100644
> --- a/drivers/base/firmware_loader/fallback_table.c
> +++ b/drivers/base/firmware_loader/fallback_table.c
> @@ -16,9 +16,6 @@
>   * firmware fallback configuration table
>   */
>  
> -/* Module or buit-in */
> -#ifdef CONFIG_FW_LOADER_USER_HELPER
> -
>  static unsigned int zero;
>  static unsigned int one = 1;
>  
> @@ -51,5 +48,3 @@ struct ctl_table firmware_config_table[] = {
>  	{ }
>  };
>  EXPORT_SYMBOL_GPL(firmware_config_table);
> -
> -#endif

Just a random early spring cleaning note for the future:

Although we cannot rename the firmware module to something more
sensible, we could just make the old sysfs firmware loading interface
*its own module firmware_class* and move the non-sysfs code to its own
first citizen file.

  Luis