Received-SPF: pass (google.com: domain of linux-kernel+bounces-171012-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <17aad327-ffdc-4cee-8b7d-174409327a7f@oracle.com>
Date: Tue, 7 May 2024 10:40:07 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] firmware_loader: Use init_utsname()->release
To: Greg KH <gregkh@linuxfoundation.org>
Cc: mcgrof@kernel.org, russ.weight@linux.dev, rafael@kernel.org,
        linux-kernel@vger.kernel.org, masahiroy@kernel.org
References: <20240223153121.440763-1-john.g.garry@oracle.com>
 <2024030757-trickily-tuesday-bfcc@gregkh>
Content-Language: en-US
From: John Garry <john.g.garry@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <2024030757-trickily-tuesday-bfcc@gregkh>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?L1RqTDRaZjNEUzRObXBvK0tCak9qWWllbFdoY29YblJFTzRSWHpFUW9XS0tR?=
 =?utf-8?B?QXdEVnpBSDlzVmllSXREekhjWDZFbThIdjMvZUl5YUZyNFVZMWN6WU9kZjc0?=
 =?utf-8?B?NGFZOFFtZS8yNWU5cWcwd2VTZ0twb1UvVnR1a09sSnFNWi96d0lFWkhCajVO?=
 =?utf-8?B?c2hPOG0wV0Q5M1RaRHN5M2RGZ1Q3dlVaYXc1TmloN2VLRUppcnl2VmJEakJs?=
 =?utf-8?B?cU53UVNEUDM1OHhYaFZ4MlRLYzBzbGVJdU5aTlEyRm1LQlhXTWlPQlVPOW4w?=
 =?utf-8?B?WVozWWt6dDV1Qy9scndHRGFJdkFsb1BDYmN2TDd1bFVKL0dNaG52ejFnY1Fs?=
 =?utf-8?B?RE1lcVM4NzFDQnNkUU9TNTUwRzdDWnpFVy9SUDhZRE1uZUdBektnVVc2WXln?=
 =?utf-8?B?b0U1bE9ORElnL05vRHZFMjB0bE1GOXhVTU5GaE1ndEoyMU15ZysveHFlYXEw?=
 =?utf-8?B?MFBtcmlOREgzVk16RGtwT3BLcTZmbUJFd0tmVHpkekNveExwODlnQllxQjlj?=
 =?utf-8?B?d3hhczRSRmg2UVZ5K0wwZisyWm41VUJhbUtVS1lmandocUVqTzdYOFViS3ZO?=
 =?utf-8?B?YlpoZEhPTEIyN2NCRDFrTjBndERSQlpJZWRtQW1xcmZueXh4elFrbkxqYll1?=
 =?utf-8?B?OE9PbFVxK3phNmtlZ2c2STRUbDlzQ1FOU0IvZ2plTWM1MGxuOUtQSUVnbStx?=
 =?utf-8?B?MWxMVDZKemNueG1rQjVaMUF2alBVakY0cXpTSytHLzJtcU14bXhsc3hOTUNq?=
 =?utf-8?B?VTBYbXVFNUJ5NU5DRmhjd2YyZzVSNFZITWYra1RlMUk0VnFibHh4ak1EVzNj?=
 =?utf-8?B?azNncjR6b3JEQmhXVmRrUWo2UDNYbkhMNmpNdjZCM2MzT0Ywci8yYWVSaVd2?=
 =?utf-8?B?TC9ud0ZQZllBOFVxK2MvRkQ0WnYrbGxoVnhVT3VVQUhaL3I4ZG1sVkdrVGxq?=
 =?utf-8?B?Ny96OENZQ2ZyLzNVRG9aMFFmWjJYZ05sV09OckJMZE9teUFJS1dwbFg4K1M4?=
 =?utf-8?B?R1ErS2FjNm1mQ2FhWkUyNXU5b3ZWeFJyc1FsVVI5UitJMXFjcHJ0di9nZUF2?=
 =?utf-8?B?L3RJVDEyNFdjeHlyVGs1U3gvWFp3azZMbDhNaEREYnZKeDFzOWlRSnFtaXFF?=
 =?utf-8?B?RE42UkxvcWdpNE5MSDRXSWlNa2RkV1BRN1ZmWkdSWGh6dGFTSzlua1hFaUlM?=
 =?utf-8?B?MEhPandDT0w4cWd3NHB3Q3BYa1pJdUFMbkZhOE1CalhqS0REYWwxTFpsUGhp?=
 =?utf-8?B?K1o3TkE1YlZYSzVHMDF2V1JqVnJ1K3UrbWd3ZTh4elc1VDN4RHN0Y2F0UXJh?=
 =?utf-8?B?MkpGb3U4U3hJUTJXeHVXVFFiTEZIWnF2K25UcnhiL0RUbTNpN3ZpMy90L1F1?=
 =?utf-8?B?VWZBUXJaaFlJMlYxb2REWEhFaEMvdlhOQWtWWVd2a3NYQUxRR3lPOWpQMlZK?=
 =?utf-8?B?S0dyR1h4QkVVd2xmbWl3N3JzL2NPL293L0dPaFNKUlIycWxLdm45aHRJWnhT?=
 =?utf-8?B?TGo2M0xlTnZJVWx4QUdUd3JmdDFXcktOK2UwRTJWdzRHNlowaU5aNlJsUzdB?=
 =?utf-8?B?YTNOeEJoTGlXVVUva3VlclRUdDVwOWwwNFAwZy9WSVZiWGpvczBmRWJCSXla?=
 =?utf-8?B?UStuMUVRcUlwd2dxbW5kT2t3enZMOUFBY2xneFg0cm9wR3ZZT2QxeDkxU1pz?=
 =?utf-8?B?eWlqY3FJTmJIdEc5MFhqWXFvSThpelJHbnZ3SEZhU1hjeHdGWXk1YzcyYnI5?=
 =?utf-8?B?WGxoTXhkaTIwelk3UEg3aWl4VGJrcTlMQVovZHl6b3NrOWRySGl5dGt0RDd5?=
 =?utf-8?B?R2FWYXl2Tll1WXlGczUvaGs2T01ac092RzNZTDhwRDVvT2FYcStOckFKOHdh?=
 =?utf-8?B?RUN5WWY0S0RISXNWQmR4ZzY4djUxS1ZuNUNtdy9sNU5YSU9vVUNCb2Zacm5r?=
 =?utf-8?B?VkNQRzdwOGFVRTk5clFwRDVvREtNZEpBWE4wWWJ5UEJoN3JCb0E0MnJ4eFk3?=
 =?utf-8?B?QXBTTENqalRrZUdTTll6d2l5UHFLMnliOEUxT2c2NGkvZDZnUEllNXFFSVEy?=
 =?utf-8?B?d0M5U0RNYm1TclBhSUxqV3Y4RVBySUhWRWVyREhsUXljY3l6ekZIelFVVjNw?=
 =?utf-8?B?V1g4ZmVGVTY3TjVwS2hrazh1SklHRFFRT3RDcFRaTzNIQUdGazZaUDRNeFhR?=
 =?utf-8?B?REE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	BxQg42IYJ6E1h+l0J1wMLjwRf35lRASC6CuvJBt9vZXCyui953DGxXRmM08deiVE4N7Qc0mdzGa35cWqqHkOhQ6duCIqUqEu7Pq4CMBHQ1hDUecCXDimY7NOMn6Quo59XXtgrN9LQsK0kxthIWwDMeyfwCbsoLEKAiNRv0IDyzwU4EAW3ml/AOTZSohlECxBqfNQx1LDZljxNtVLk6WqU5eQOOcaDVlDKFh2YWRQDZ6uzlm3HdQM+/d9FpQwmxdjP3NTh8oSHCRX4+diLLOAfQKo397++EBcNv0odFJf0q+ZyVX1oaU6DFQzkkH5yPdoZJ4CcoH3xqgDhE+ipNUavCSZVnVLnnMQnaxuI0o+vWOXzB57E8WEjoZsWGf+P9DcguPQyDc9tpSGqmDM4Wr8FAKcQ1bm751AgUeoytLcNjk0+1vzjndmUKDpeTql7jOPESaKzWnEQUbPzAk/F/nhiXQhumm2AKRR8ARJTimtTXL7i1+klpc5cgLpgm6hCIa4F5RC73cjwHxdkexsqBvet6C90I+CaekZYeP2wzxi5q27C24LIeiHxsflefMuufnbmc3dwwSBDkyAXJRgWtiY7qdI90N6+w6nmaHTgrrnRwU=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: add788e7-a7d0-4b86-ab91-08dc6e79b0ca
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4313.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 May 2024 09:40:11.7822
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Kcp8VSu2IAleluChFThPPwHc6zYXLVsk71d1UWPXnR/3vZzKNK4eb0EDm9LAbyOcJB7B3ux1+jQGSbzvrOeX1Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4638
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-07_04,2024-05-06_02,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxlogscore=999
 bulkscore=0 suspectscore=0 spamscore=0 malwarescore=0 mlxscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2405070065
X-Proofpoint-GUID: Ns4Imhk8ZrMmntR9IEIn9TpdCNeDbKWU
X-Proofpoint-ORIG-GUID: Ns4Imhk8ZrMmntR9IEIn9TpdCNeDbKWU

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