Received-SPF: pass (google.com: domain of linux-kernel+bounces-96297-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 7 Mar 2024 22:01:40 +0000
From: Greg KH <gregkh@linuxfoundation.org>
To: John Garry <john.g.garry@oracle.com>
Cc: mcgrof@kernel.org, russ.weight@linux.dev, rafael@kernel.org,
	linux-kernel@vger.kernel.org, masahiroy@kernel.org
Subject: Re: [PATCH v2] firmware_loader: Use init_utsname()->release
Message-ID: <2024030757-trickily-tuesday-bfcc@gregkh>
References: <20240223153121.440763-1-john.g.garry@oracle.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240223153121.440763-1-john.g.garry@oracle.com>

On Fri, Feb 23, 2024 at 03:31:21PM +0000, John Garry wrote:
> Instead of using UTS_RELEASE, use init_utsname()->release, which means
> that we don't need to rebuild the code just for the git head commit
> changing.

But you are now exchanging build "convience" with code complexity and
runtime checking.  Which is better, and which is "provable"?

> Note: As mentioned by Masahiro in [0], when CONFIG_MODVERSIONS=y it
> could be possible for a driver to be built as a module with a different
> kernel baseline and so use a different UTS_RELEASE from that baseline. So
> now using init_utsname()->release could lead to a change in behaviour
> in this driver. However, considering the nature of this driver and how it
> would not make sense to build it as an external module against a different
> tree, this change should not have any effect on users.

This is not a "driver", it's the firmware core so this does not make
sense.


> 
> [0] https://lore.kernel.org/lkml/CAK7LNAQ_r5yUjNpOppLkDBQ12sDxBYQTvRZGn1ng8D1POfZr_A@mail.gmail.com/
> 
> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
> Changes in v2:
> - moved note into commit log and tweaked slightly
> - add Luis' RB tags, thanks
> 
> Also verified against fw loader selftest - it seems to show no regression
> from baseline, however the baeline sometimes hangs (and also does with
> this patch).
> 
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index 3c67f24785fc..9a3671659134 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -38,7 +38,7 @@
>  #include <linux/zstd.h>
>  #include <linux/xz.h>
>  
> -#include <generated/utsrelease.h>
> +#include <linux/utsname.h>
>  
>  #include "../base.h"
>  #include "firmware.h"
> @@ -471,9 +471,9 @@ static int fw_decompress_xz(struct device *dev, struct fw_priv *fw_priv,
>  static char fw_path_para[256];
>  static const char * const fw_path[] = {
>  	fw_path_para,
> -	"/lib/firmware/updates/" UTS_RELEASE,
> +	"/lib/firmware/updates/", /* UTS_RELEASE is appended later */
>  	"/lib/firmware/updates",
> -	"/lib/firmware/" UTS_RELEASE,
> +	"/lib/firmware/", /* UTS_RELEASE is appended later */
>  	"/lib/firmware"
>  };
>  
> @@ -496,7 +496,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>  	size_t size;
>  	int i, len, maxlen = 0;
>  	int rc = -ENOENT;
> -	char *path, *nt = NULL;
> +	char *path, *fw_path_string, *nt = NULL;
>  	size_t msize = INT_MAX;
>  	void *buffer = NULL;
>  	dev_err(device, "%s suffix=%s\n", __func__, suffix);
> @@ -511,6 +511,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>  	if (!path)
>  		return -ENOMEM;
>  
> +	fw_path_string = __getname();
> +	if (!fw_path_string) {
> +		__putname(path);
> +		return -ENOMEM;
> +	}
> +
>  	wait_for_initramfs();
>  	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
>  		size_t file_size = 0;
> @@ -521,16 +527,32 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>  		if (!fw_path[i][0])
>  			continue;
>  
> +		len = snprintf(fw_path_string, PATH_MAX, "%s", fw_path[i]);
> +		if (len >= PATH_MAX) {
> +			rc = -ENAMETOOLONG;
> +			break;
> +		}
> +
> +		/* Special handling to append UTS_RELEASE */

You don't really document why you want to do that here, and ick:

> +		if ((fw_path[i] != fw_path_para) && (fw_path[i][len - 1] == '/')) {
> +			len = snprintf(fw_path_string, PATH_MAX, "%s%s",
> +					fw_path[i], init_utsname()->release);

You now have a "rule" where a trailing / means we add the UTS_RELEASE to
it, how is anyone going to remember that if they want to add a new path
to the array above?

this is going to be a maintenance nightmare, sorry.

greg k-h