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

From: John Garry
Date: Tue May 07 2024 - 05:48:03 EST

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


Hi Greg,

Do you think that the change below is more acceptable?

Now firmware_loader is the only module in drivers/ which needs to be rebuilt for git head commit changing under a x86_64 or arm64 defconfig - I didn't test other architectures. It would be nice to improve that situation.

Thanks,
John

---- 8< -----


diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index da8ca01d011c..162fe0c4fd51 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"
@@ -469,12 +469,16 @@ static int fw_decompress_xz(struct device *dev, struct fw_priv *fw_priv,

/* direct firmware loading support */
static char fw_path_para[256];
-static const char * const fw_path[] = {
- fw_path_para,
- "/lib/firmware/updates/" UTS_RELEASE,
- "/lib/firmware/updates",
- "/lib/firmware/" UTS_RELEASE,
- "/lib/firmware"
+
+static const struct fw_path_s {
+ const char *path;
+ bool append; /* Append UTS_RELEASE, path must end in '/' */
+} fw_paths[] = {
+ { fw_path_para, false},
+ { "/lib/firmware/updates/", true},
+ { "/lib/firmware/updates", false},
+ { "/lib/firmware/", true},
+ { "/lib/firmware", false},
};

/*
@@ -496,7 +500,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;

@@ -510,25 +514,40 @@ 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++) {
+ for (i = 0; i < ARRAY_SIZE(fw_paths); i++) {
+ const struct fw_path_s *fw_path = &fw_paths[i];
size_t file_size = 0;
size_t *file_size_ptr = NULL;

/* skip the unset customized path */
- if (!fw_path[i][0])
+ if (!fw_path->path[0])
continue;

+ len = snprintf(fw_path_string, PATH_MAX, "%s%s",
+ fw_path->path, fw_path->append ?
+ init_utsname()->release : "");
+ if (len >= PATH_MAX) {
+ rc = -ENAMETOOLONG;
+ break;
+ }
+
/* strip off \n from customized path */
- maxlen = strlen(fw_path[i]);
+ maxlen = strlen(fw_path_string);
if (i == 0) {
- nt = strchr(fw_path[i], '\n');
+ nt = strchr(fw_path_string, '\n');
if (nt)
- maxlen = nt - fw_path[i];
+ maxlen = nt - fw_path_string;
}

len = snprintf(path, PATH_MAX, "%.*s/%s%s",
- maxlen, fw_path[i],
+ maxlen, fw_path_string,
fw_priv->fw_name, suffix);
if (len >= PATH_MAX) {
rc = -ENAMETOOLONG;
@@ -589,6 +608,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
break;
}
__putname(path);
+ __putname(fw_path_string);

return rc;
}
--
2.34.1

---- 8< -------


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://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:

+ 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