Re: [PATCH v2] firmware_loader: Use init_utsname()->release

[John Garry]

On 07/03/2024 22:01, Greg KH wrote:
> 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"?

Well I did want something that did not add too much complexity, so it 
would be an obvious win.

> > 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.

Understood

> > [0]https://urldefense.com/v3/__https://lore.kernel.org/lkml/CAK7LNAQ_r5yUjNpOppLkDBQ12sDxBYQTvRZGn1ng8D1POfZr_A@xxxxxxxxxxxxxx/__;!!ACWV5N9M2RV99hQ!I5-MVUh-jmCxwUFtX_eLsjXZpF9BBk6KeBWJ-6mlrfjJjomRDUWQ0_nXpixUddcj_Gz6H_FiBu8lUys6u5kzcqsAyg$  
> >
> > Reviewed-by: Luis Chamberlain<mcgrof@xxxxxxxxxx>
> > Signed-off-by: John Garry<john.g.garry@xxxxxxxxxx>
> > ---
> > 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:

FWIW, I did leave comments for current members of fw_path[]

> > +		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?

I didn't think that those path ever really changed, so would not be a 
problem, but I see what you mean.

> this is going to be a maintenance nightmare, sorry.

Understood, back to the drawing board ...

Thanks,
John