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.
Since UTS_RELEASE is used for fw_path[] and this points to const data,
append init_utsname()->release dynamically to an intermediate string.
The check for appending uts release is if the string in fw_path[] ends
in '/'. Since fw_path[] may include a path from the kernel command line
in fw_path_para, and it would be valid for this string to end in '/',
check for fw_path_para when appending uts release.
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.
[0] https://lore.kernel.org/lkml/CAK7LNAQ_r5yUjNpOppLkDBQ12sDxBYQTvRZGn1ng8D1POfZr_A@mail.gmail.com/
Reviewed-by: Luis Chamberlain <[email protected]>
Signed-off-by: John Garry <[email protected]>
---
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 */
+ 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);
+ 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;
@@ -588,6 +610,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
break;
}
__putname(path);
+ __putname(fw_path_string);
return rc;
}
--
2.31.1
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 <[email protected]>
> Signed-off-by: John Garry <[email protected]>
> ---
> 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
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@mail.gmail.com/__;!!ACWV5N9M2RV99hQ!I5-MVUh-jmCxwUFtX_eLsjXZpF9BBk6KeBWJ-6mlrfjJjomRDUWQ0_nXpixUddcj_Gz6H_FiBu8lUys6u5kzcqsAyg$
>>
>> Reviewed-by: Luis Chamberlain<[email protected]>
>> Signed-off-by: John Garry<[email protected]>
>> ---
>> 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
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@mail.gmail.com/__;!!ACWV5N9M2RV99hQ!I5-MVUh-jmCxwUFtX_eLsjXZpF9BBk6KeBWJ-6mlrfjJjomRDUWQ0_nXpixUddcj_Gz6H_FiBu8lUys6u5kzcqsAyg$
>>
>> Reviewed-by: Luis Chamberlain <[email protected]>
>> Signed-off-by: John Garry <[email protected]>
>> ---
>> 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