[PATCH 0/3] firmware loader: cleanup and introduce search paths option

[PATCH 0/3] firmware loader: cleanup and introduce search paths option

[Ming Lei]

Hi,

The 1st patch cancels exporting of cache_firmware and uncache_firmware.

The 2nd one simplifies holding module for request_firmware().

The 3rd one introduces one kernel option to allow distributions or users
to set their specific firmware search paths.

 drivers/base/Kconfig          |   12 ++++++
 drivers/base/firmware_class.c |   95 +++++++++++++++++++++++++++++++++--------
 include/linux/firmware.h      |   11 -----
 3 files changed, 89 insertions(+), 29 deletions(-)



Thanks,
--
Ming Lei

[PATCH 1/3] firmware loader: don't export cache_firmware and uncache_firmware

[Ming Lei]

Looks no drivers have the explict requirement for the two, just don't
export them anymore.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |    6 ++----
 include/linux/firmware.h      |   11 -----------
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index c31fc29..34d47e3 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -1240,7 +1240,7 @@ EXPORT_SYMBOL(request_firmware_nowait);
  * Return !0 otherwise
  *
  */
-int cache_firmware(const char *fw_name)
+static int cache_firmware(const char *fw_name)
 {
 	int ret;
 	const struct firmware *fw;
@@ -1255,7 +1255,6 @@ int cache_firmware(const char *fw_name)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(cache_firmware);
 
 /**
  * uncache_firmware - remove one cached firmware image
@@ -1268,7 +1267,7 @@ EXPORT_SYMBOL_GPL(cache_firmware);
  * Return !0 otherwise
  *
  */
-int uncache_firmware(const char *fw_name)
+static int uncache_firmware(const char *fw_name)
 {
 	struct firmware_buf *buf;
 	struct firmware fw;
@@ -1286,7 +1285,6 @@ int uncache_firmware(const char *fw_name)
 
 	return -EINVAL;
 }
-EXPORT_SYMBOL_GPL(uncache_firmware);
 
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index e4279fe..e154c10 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -47,8 +47,6 @@ int request_firmware_nowait(
 	void (*cont)(const struct firmware *fw, void *context));
 
 void release_firmware(const struct firmware *fw);
-int cache_firmware(const char *name);
-int uncache_firmware(const char *name);
 #else
 static inline int request_firmware(const struct firmware **fw,
 				   const char *name,
@@ -68,15 +66,6 @@ static inline void release_firmware(const struct firmware *fw)
 {
 }
 
-static inline int cache_firmware(const char *name)
-{
-	return -ENOENT;
-}
-
-static inline int uncache_firmware(const char *name)
-{
-	return -EINVAL;
-}
 #endif
 
 #endif
-- 
1.7.9.5

[PATCH 2/3] firmware loader: simplify holding module for request_firmware

[Ming Lei]

module reference doesn't cover direct loading path, so this patch
simply holds the module in the whole life time of request_firmware()
to fix the problem.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/firmware_class.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 34d47e3..c743409 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -523,8 +523,6 @@ static void fw_dev_release(struct device *dev)
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 
 	kfree(fw_priv);
-
-	module_put(THIS_MODULE);
 }
 
 static int firmware_uevent(struct device *dev, struct kobj_uevent_env *env)
@@ -852,9 +850,6 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, bool uevent,
 
 	dev_set_uevent_suppress(f_dev, true);
 
-	/* Need to pin this module until class device is destroyed */
-	__module_get(THIS_MODULE);
-
 	retval = device_add(f_dev);
 	if (retval) {
 		dev_err(f_dev, "%s: device_register failed\n", __func__);
@@ -1127,7 +1122,13 @@ int
 request_firmware(const struct firmware **firmware_p, const char *name,
                  struct device *device)
 {
-	return _request_firmware(firmware_p, name, device, true, false);
+	int ret;
+
+	/* Need to pin this module until return */
+	__module_get(THIS_MODULE);
+	ret = _request_firmware(firmware_p, name, device, true, false);
+	module_put(THIS_MODULE);
+	return ret;
 }
 EXPORT_SYMBOL(request_firmware);
 
-- 
1.7.9.5

[PATCH 3/3] firmware loader: allow distribution to choose default search paths

[Ming Lei]

For some distributions(e.g. android), firmware images aren't put
under kernel built-in search paths, so introduce one Kconfig
option to allow distributions or users to choose its specific default
search paths, which are always tried before searching from kernel
built-in paths in direct loading.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/Kconfig          |   12 +++++++
 drivers/base/firmware_class.c |   76 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 80 insertions(+), 8 deletions(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 07abd9d..5b0c909 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -156,6 +156,18 @@ config FW_LOADER_USER_HELPER
 	  no longer required unless you have a special firmware file that
 	  resides in a non-standard path.
 
+config FW_CUSTOMIZED_PATH
+	string "default firmware search paths for direct loading"
+	help
+	  On some distribution(e.g. android), firmware images aren't
+	  put under kernel built-in search paths, so provide this option
+	  for distributions to choose a distribution specific firmware
+	  search path. The option allows to choose more than one path,
+	  and paths are seperated with semicolon(e.g. on android, the
+	  option might look as "/etc/firmware;/vendor/firmware").
+
+	  If you are unsure about this, don't choose here.
+
 config DEBUG_DRIVER
 	bool "Driver Core verbose debug messages"
 	depends on DEBUG_KERNEL
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index c743409..50b5913 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -267,6 +267,9 @@ static void fw_free_buf(struct firmware_buf *buf)
 static char fw_path_para[256];
 static const char * const fw_path[] = {
 	fw_path_para,
+#ifdef CONFIG_FW_CUSTOMIZED_PATH
+	CONFIG_FW_CUSTOMIZED_PATH,
+#endif
 	"/lib/firmware/updates/" UTS_RELEASE,
 	"/lib/firmware/updates",
 	"/lib/firmware/" UTS_RELEASE,
@@ -314,6 +317,59 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
 	return true;
 }
 
+static bool fw_get_file_firmware(const char *path,
+				 struct firmware_buf *buf)
+{
+	struct file *file;
+	bool success;
+
+	file = filp_open(path, O_RDONLY, 0);
+	if (IS_ERR(file))
+		return false;
+	success = fw_read_file_contents(file, buf);
+	fput(file);
+
+	return success;
+}
+
+#ifdef CONFIG_FW_CUSTOMIZED_PATH
+/* The path in @paths is seperated by ';' */
+static bool fw_get_fw_file_from_paths(const char *paths, char *path,
+				      struct firmware_buf *buf)
+{
+	int len, start, end;
+	char *pos;
+
+	end = -1;
+	do {
+		start = end + 1;
+		pos = strchr(&paths[start], ';');
+		if (pos) {
+			end = (int)(pos - paths);
+			len = end - start;
+		} else {
+			len = strlen(&paths[start]);
+		}
+
+		if (PATH_MAX < len + strlen(buf->fw_id))
+			continue;
+		strncpy(path, &paths[start], len);
+		snprintf(&path[len], PATH_MAX - len, "/%s", buf->fw_id);
+
+		if (fw_get_file_firmware(path, buf))
+			return true;
+	} while (pos && end < strlen(paths) - 1);
+
+	return false;
+}
+#else
+static bool fw_get_fw_file_from_paths(const char *paths, char *path,
+				      struct firmware_buf *buf)
+{
+	return false;
+}
+#endif
+
 static bool fw_get_filesystem_firmware(struct device *device,
 				       struct firmware_buf *buf)
 {
@@ -322,19 +378,23 @@ static bool fw_get_filesystem_firmware(struct device *device,
 	char *path = __getname();
 
 	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
-		struct file *file;
 
 		/* skip the unset customized path */
 		if (!fw_path[i][0])
 			continue;
 
-		snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
-
-		file = filp_open(path, O_RDONLY, 0);
-		if (IS_ERR(file))
-			continue;
-		success = fw_read_file_contents(file, buf);
-		fput(file);
+		/*
+		 * If CONFIG_FW_CUSTOMIZED_PATH is set, search from
+		 * these paths first
+		 */
+		if (i == 1 && ARRAY_SIZE(fw_path) > 5) {
+			success = fw_get_fw_file_from_paths(fw_path[1],
+							    path, buf);
+		} else {
+			snprintf(path, PATH_MAX, "%s/%s", fw_path[i],
+				 buf->fw_id);
+			success = fw_get_file_firmware(path, buf);
+		}
 		if (success)
 			break;
 	}
-- 
1.7.9.5

Re: [PATCH 3/3] firmware loader: allow distribution to choose default search paths

[Takashi Iwai]

At Wed,  5 Jun 2013 13:42:49 +0800,
Ming Lei wrote:
> 
> For some distributions(e.g. android), firmware images aren't put
> under kernel built-in search paths, so introduce one Kconfig
> option to allow distributions or users to choose its specific default
> search paths, which are always tried before searching from kernel
> built-in paths in direct loading.
> 
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/base/Kconfig          |   12 +++++++
>  drivers/base/firmware_class.c |   76 ++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 80 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 07abd9d..5b0c909 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -156,6 +156,18 @@ config FW_LOADER_USER_HELPER
>  	  no longer required unless you have a special firmware file that
>  	  resides in a non-standard path.
>  
> +config FW_CUSTOMIZED_PATH
> +	string "default firmware search paths for direct loading"
> +	help
> +	  On some distribution(e.g. android), firmware images aren't
> +	  put under kernel built-in search paths, so provide this option
> +	  for distributions to choose a distribution specific firmware
> +	  search path. The option allows to choose more than one path,
> +	  and paths are seperated with semicolon(e.g. on android, the
> +	  option might look as "/etc/firmware;/vendor/firmware").
> +
> +	  If you are unsure about this, don't choose here.
> +
>  config DEBUG_DRIVER
>  	bool "Driver Core verbose debug messages"
>  	depends on DEBUG_KERNEL
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index c743409..50b5913 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -267,6 +267,9 @@ static void fw_free_buf(struct firmware_buf *buf)
>  static char fw_path_para[256];
>  static const char * const fw_path[] = {
>  	fw_path_para,
> +#ifdef CONFIG_FW_CUSTOMIZED_PATH
> +	CONFIG_FW_CUSTOMIZED_PATH,
> +#endif
>  	"/lib/firmware/updates/" UTS_RELEASE,
>  	"/lib/firmware/updates",
>  	"/lib/firmware/" UTS_RELEASE,
> @@ -314,6 +317,59 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
>  	return true;
>  }
>  
> +static bool fw_get_file_firmware(const char *path,
> +				 struct firmware_buf *buf)
> +{
> +	struct file *file;
> +	bool success;
> +
> +	file = filp_open(path, O_RDONLY, 0);
> +	if (IS_ERR(file))
> +		return false;
> +	success = fw_read_file_contents(file, buf);
> +	fput(file);
> +
> +	return success;
> +}
> +
> +#ifdef CONFIG_FW_CUSTOMIZED_PATH
> +/* The path in @paths is seperated by ';' */
> +static bool fw_get_fw_file_from_paths(const char *paths, char *path,
> +				      struct firmware_buf *buf)
> +{
> +	int len, start, end;
> +	char *pos;
> +
> +	end = -1;
> +	do {
> +		start = end + 1;
> +		pos = strchr(&paths[start], ';');
> +		if (pos) {
> +			end = (int)(pos - paths);
> +			len = end - start;
> +		} else {
> +			len = strlen(&paths[start]);
> +		}
> +
> +		if (PATH_MAX < len + strlen(buf->fw_id))
> +			continue;
> +		strncpy(path, &paths[start], len);
> +		snprintf(&path[len], PATH_MAX - len, "/%s", buf->fw_id);
> +
> +		if (fw_get_file_firmware(path, buf))
> +			return true;
> +	} while (pos && end < strlen(paths) - 1);
> +
> +	return false;
> +}
> +#else
> +static bool fw_get_fw_file_from_paths(const char *paths, char *path,
> +				      struct firmware_buf *buf)
> +{
> +	return false;
> +}
> +#endif
> +
>  static bool fw_get_filesystem_firmware(struct device *device,
>  				       struct firmware_buf *buf)
>  {
> @@ -322,19 +378,23 @@ static bool fw_get_filesystem_firmware(struct device *device,
>  	char *path = __getname();
>  
>  	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
> -		struct file *file;
>  
>  		/* skip the unset customized path */
>  		if (!fw_path[i][0])
>  			continue;
>  
> -		snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
> -
> -		file = filp_open(path, O_RDONLY, 0);
> -		if (IS_ERR(file))
> -			continue;
> -		success = fw_read_file_contents(file, buf);
> -		fput(file);
> +		/*
> +		 * If CONFIG_FW_CUSTOMIZED_PATH is set, search from
> +		 * these paths first
> +		 */
> +		if (i == 1 && ARRAY_SIZE(fw_path) > 5) {
> +			success = fw_get_fw_file_from_paths(fw_path[1],
> +							    path, buf);
> +		} else {
> +			snprintf(path, PATH_MAX, "%s/%s", fw_path[i],
> +				 buf->fw_id);
> +			success = fw_get_file_firmware(path, buf);

Shouldn't fw_get_fw_file_from_paths() be applied unconditionally?
It'll be benefit for the customized path passed via module option,
too.  The only drawback is a slight code growth, but the code can be
reduced a bit with strcspn() or such.

BTW, I now wonder what happens if you pass a relative path.
Did you already test it?


thanks,

Takashi

Re: [PATCH 3/3] firmware loader: allow distribution to choose default search paths

[Ming Lei]

On Wed, Jun 5, 2013 at 3:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Wed,  5 Jun 2013 13:42:49 +0800,
> Ming Lei wrote:
>>
>> For some distributions(e.g. android), firmware images aren't put
>> under kernel built-in search paths, so introduce one Kconfig
>> option to allow distributions or users to choose its specific default
>> search paths, which are always tried before searching from kernel
>> built-in paths in direct loading.
>>
>> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> Cc: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  drivers/base/Kconfig          |   12 +++++++
>>  drivers/base/firmware_class.c |   76 ++++++++++++++++++++++++++++++++++++-----
>>  2 files changed, 80 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
>> index 07abd9d..5b0c909 100644
>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -156,6 +156,18 @@ config FW_LOADER_USER_HELPER
>>         no longer required unless you have a special firmware file that
>>         resides in a non-standard path.
>>
>> +config FW_CUSTOMIZED_PATH
>> +     string "default firmware search paths for direct loading"
>> +     help
>> +       On some distribution(e.g. android), firmware images aren't
>> +       put under kernel built-in search paths, so provide this option
>> +       for distributions to choose a distribution specific firmware
>> +       search path. The option allows to choose more than one path,
>> +       and paths are seperated with semicolon(e.g. on android, the
>> +       option might look as "/etc/firmware;/vendor/firmware").
>> +
>> +       If you are unsure about this, don't choose here.
>> +
>>  config DEBUG_DRIVER
>>       bool "Driver Core verbose debug messages"
>>       depends on DEBUG_KERNEL
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index c743409..50b5913 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -267,6 +267,9 @@ static void fw_free_buf(struct firmware_buf *buf)
>>  static char fw_path_para[256];
>>  static const char * const fw_path[] = {
>>       fw_path_para,
>> +#ifdef CONFIG_FW_CUSTOMIZED_PATH
>> +     CONFIG_FW_CUSTOMIZED_PATH,
>> +#endif
>>       "/lib/firmware/updates/" UTS_RELEASE,
>>       "/lib/firmware/updates",
>>       "/lib/firmware/" UTS_RELEASE,
>> @@ -314,6 +317,59 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
>>       return true;
>>  }
>>
>> +static bool fw_get_file_firmware(const char *path,
>> +                              struct firmware_buf *buf)
>> +{
>> +     struct file *file;
>> +     bool success;
>> +
>> +     file = filp_open(path, O_RDONLY, 0);
>> +     if (IS_ERR(file))
>> +             return false;
>> +     success = fw_read_file_contents(file, buf);
>> +     fput(file);
>> +
>> +     return success;
>> +}
>> +
>> +#ifdef CONFIG_FW_CUSTOMIZED_PATH
>> +/* The path in @paths is seperated by ';' */
>> +static bool fw_get_fw_file_from_paths(const char *paths, char *path,
>> +                                   struct firmware_buf *buf)
>> +{
>> +     int len, start, end;
>> +     char *pos;
>> +
>> +     end = -1;
>> +     do {
>> +             start = end + 1;
>> +             pos = strchr(&paths[start], ';');
>> +             if (pos) {
>> +                     end = (int)(pos - paths);
>> +                     len = end - start;
>> +             } else {
>> +                     len = strlen(&paths[start]);
>> +             }
>> +
>> +             if (PATH_MAX < len + strlen(buf->fw_id))
>> +                     continue;
>> +             strncpy(path, &paths[start], len);
>> +             snprintf(&path[len], PATH_MAX - len, "/%s", buf->fw_id);
>> +
>> +             if (fw_get_file_firmware(path, buf))
>> +                     return true;
>> +     } while (pos && end < strlen(paths) - 1);
>> +
>> +     return false;
>> +}
>> +#else
>> +static bool fw_get_fw_file_from_paths(const char *paths, char *path,
>> +                                   struct firmware_buf *buf)
>> +{
>> +     return false;
>> +}
>> +#endif
>> +
>>  static bool fw_get_filesystem_firmware(struct device *device,
>>                                      struct firmware_buf *buf)
>>  {
>> @@ -322,19 +378,23 @@ static bool fw_get_filesystem_firmware(struct device *device,
>>       char *path = __getname();
>>
>>       for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
>> -             struct file *file;
>>
>>               /* skip the unset customized path */
>>               if (!fw_path[i][0])
>>                       continue;
>>
>> -             snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
>> -
>> -             file = filp_open(path, O_RDONLY, 0);
>> -             if (IS_ERR(file))
>> -                     continue;
>> -             success = fw_read_file_contents(file, buf);
>> -             fput(file);
>> +             /*
>> +              * If CONFIG_FW_CUSTOMIZED_PATH is set, search from
>> +              * these paths first
>> +              */
>> +             if (i == 1 && ARRAY_SIZE(fw_path) > 5) {
>> +                     success = fw_get_fw_file_from_paths(fw_path[1],
>> +                                                         path, buf);
>> +             } else {
>> +                     snprintf(path, PATH_MAX, "%s/%s", fw_path[i],
>> +                              buf->fw_id);
>> +                     success = fw_get_file_firmware(path, buf);
>
> Shouldn't fw_get_fw_file_from_paths() be applied unconditionally?
> It'll be benefit for the customized path passed via module option,
> too.  The only drawback is a slight code growth, but the code can be
> reduced a bit with strcspn() or such.

You mean that both the 1st two items should be covered by
fw_get_fw_file_from_paths()? If so, the function may become
a bit ugly since two strings are required to pass in, and we can't
merge one runtime string and one ro string created in compiling.

Looks we can let fw_get_fw_file_from_paths() handle all
predefined paths(CONFIG_FW_CUSTOMIZED_PATH and
kernel built-in paths), then fw_get_filesystem_firmware()
may become simple, just check fw_path_para and all
other paths by fw_get_fw_file_from_paths(). How about the
idea?

>
> BTW, I now wonder what happens if you pass a relative path.
> Did you already test it?

I tested absolute paths, and not test relative paths. Do you mean
it may cause security problem? If so, we can check and ignore them,
but it should be OK since the paths are provided by kernel builder.
>From view of function, I don't think there are much difference with
absolute paths.


Thanks,
--
Ming Lei

Re: [PATCH 3/3] firmware loader: allow distribution to choose default search paths

[Takashi Iwai]

At Wed, 5 Jun 2013 17:35:26 +0800,
Ming Lei wrote:
> 
> On Wed, Jun 5, 2013 at 3:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Wed,  5 Jun 2013 13:42:49 +0800,
> > Ming Lei wrote:
> >>
> >> For some distributions(e.g. android), firmware images aren't put
> >> under kernel built-in search paths, so introduce one Kconfig
> >> option to allow distributions or users to choose its specific default
> >> search paths, which are always tried before searching from kernel
> >> built-in paths in direct loading.
> >>
> >> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> >> Cc: Takashi Iwai <tiwai@suse.de>
> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> >> ---
> >>  drivers/base/Kconfig          |   12 +++++++
> >>  drivers/base/firmware_class.c |   76 ++++++++++++++++++++++++++++++++++++-----
> >>  2 files changed, 80 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> >> index 07abd9d..5b0c909 100644
> >> --- a/drivers/base/Kconfig
> >> +++ b/drivers/base/Kconfig
> >> @@ -156,6 +156,18 @@ config FW_LOADER_USER_HELPER
> >>         no longer required unless you have a special firmware file that
> >>         resides in a non-standard path.
> >>
> >> +config FW_CUSTOMIZED_PATH
> >> +     string "default firmware search paths for direct loading"
> >> +     help
> >> +       On some distribution(e.g. android), firmware images aren't
> >> +       put under kernel built-in search paths, so provide this option
> >> +       for distributions to choose a distribution specific firmware
> >> +       search path. The option allows to choose more than one path,
> >> +       and paths are seperated with semicolon(e.g. on android, the
> >> +       option might look as "/etc/firmware;/vendor/firmware").
> >> +
> >> +       If you are unsure about this, don't choose here.
> >> +
> >>  config DEBUG_DRIVER
> >>       bool "Driver Core verbose debug messages"
> >>       depends on DEBUG_KERNEL
> >> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> >> index c743409..50b5913 100644
> >> --- a/drivers/base/firmware_class.c
> >> +++ b/drivers/base/firmware_class.c
> >> @@ -267,6 +267,9 @@ static void fw_free_buf(struct firmware_buf *buf)
> >>  static char fw_path_para[256];
> >>  static const char * const fw_path[] = {
> >>       fw_path_para,
> >> +#ifdef CONFIG_FW_CUSTOMIZED_PATH
> >> +     CONFIG_FW_CUSTOMIZED_PATH,
> >> +#endif
> >>       "/lib/firmware/updates/" UTS_RELEASE,
> >>       "/lib/firmware/updates",
> >>       "/lib/firmware/" UTS_RELEASE,
> >> @@ -314,6 +317,59 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
> >>       return true;
> >>  }
> >>
> >> +static bool fw_get_file_firmware(const char *path,
> >> +                              struct firmware_buf *buf)
> >> +{
> >> +     struct file *file;
> >> +     bool success;
> >> +
> >> +     file = filp_open(path, O_RDONLY, 0);
> >> +     if (IS_ERR(file))
> >> +             return false;
> >> +     success = fw_read_file_contents(file, buf);
> >> +     fput(file);
> >> +
> >> +     return success;
> >> +}
> >> +
> >> +#ifdef CONFIG_FW_CUSTOMIZED_PATH
> >> +/* The path in @paths is seperated by ';' */
> >> +static bool fw_get_fw_file_from_paths(const char *paths, char *path,
> >> +                                   struct firmware_buf *buf)
> >> +{
> >> +     int len, start, end;
> >> +     char *pos;
> >> +
> >> +     end = -1;
> >> +     do {
> >> +             start = end + 1;
> >> +             pos = strchr(&paths[start], ';');
> >> +             if (pos) {
> >> +                     end = (int)(pos - paths);
> >> +                     len = end - start;
> >> +             } else {
> >> +                     len = strlen(&paths[start]);
> >> +             }
> >> +
> >> +             if (PATH_MAX < len + strlen(buf->fw_id))
> >> +                     continue;
> >> +             strncpy(path, &paths[start], len);
> >> +             snprintf(&path[len], PATH_MAX - len, "/%s", buf->fw_id);
> >> +
> >> +             if (fw_get_file_firmware(path, buf))
> >> +                     return true;
> >> +     } while (pos && end < strlen(paths) - 1);
> >> +
> >> +     return false;
> >> +}
> >> +#else
> >> +static bool fw_get_fw_file_from_paths(const char *paths, char *path,
> >> +                                   struct firmware_buf *buf)
> >> +{
> >> +     return false;
> >> +}
> >> +#endif
> >> +
> >>  static bool fw_get_filesystem_firmware(struct device *device,
> >>                                      struct firmware_buf *buf)
> >>  {
> >> @@ -322,19 +378,23 @@ static bool fw_get_filesystem_firmware(struct device *device,
> >>       char *path = __getname();
> >>
> >>       for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
> >> -             struct file *file;
> >>
> >>               /* skip the unset customized path */
> >>               if (!fw_path[i][0])
> >>                       continue;
> >>
> >> -             snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
> >> -
> >> -             file = filp_open(path, O_RDONLY, 0);
> >> -             if (IS_ERR(file))
> >> -                     continue;
> >> -             success = fw_read_file_contents(file, buf);
> >> -             fput(file);
> >> +             /*
> >> +              * If CONFIG_FW_CUSTOMIZED_PATH is set, search from
> >> +              * these paths first
> >> +              */
> >> +             if (i == 1 && ARRAY_SIZE(fw_path) > 5) {
> >> +                     success = fw_get_fw_file_from_paths(fw_path[1],
> >> +                                                         path, buf);
> >> +             } else {
> >> +                     snprintf(path, PATH_MAX, "%s/%s", fw_path[i],
> >> +                              buf->fw_id);
> >> +                     success = fw_get_file_firmware(path, buf);
> >
> > Shouldn't fw_get_fw_file_from_paths() be applied unconditionally?
> > It'll be benefit for the customized path passed via module option,
> > too.  The only drawback is a slight code growth, but the code can be
> > reduced a bit with strcspn() or such.
> 
> You mean that both the 1st two items should be covered by
> fw_get_fw_file_from_paths()? If so, the function may become
> a bit ugly since two strings are required to pass in, and we can't
> merge one runtime string and one ro string created in compiling.

I meant to simply call fw_get_fw_file_from_paths() for all fw_path[]
entries.  So far, the module option can pass only a single path.
But if it's handled through fw_get_file_from_paths(), you can pass
multiple paths there, too.

> Looks we can let fw_get_fw_file_from_paths() handle all
> predefined paths(CONFIG_FW_CUSTOMIZED_PATH and
> kernel built-in paths), then fw_get_filesystem_firmware()
> may become simple, just check fw_path_para and all
> other paths by fw_get_fw_file_from_paths(). How about the
> idea?
> 
> > BTW, I now wonder what happens if you pass a relative path.
> > Did you already test it?
> 
> I tested absolute paths, and not test relative paths. Do you mean
> it may cause security problem?

Yes.  It just came to my mind while reviewing your patch.

> If so, we can check and ignore them,
> but it should be OK since the paths are provided by kernel builder.
> >From view of function, I don't think there are much difference with
> absolute paths.

The path can be provided via module option, too, so we need to check
it in anyway.


thanks,

Takashi

Re: [PATCH 3/3] firmware loader: allow distribution to choose default search paths

[Ming Lei]

On Wed, Jun 5, 2013 at 5:50 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Wed, 5 Jun 2013 17:35:26 +0800,
> Ming Lei wrote:
>>
>> On Wed, Jun 5, 2013 at 3:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > At Wed,  5 Jun 2013 13:42:49 +0800,
>> > Ming Lei wrote:
>> >>
>> >> For some distributions(e.g. android), firmware images aren't put
>> >> under kernel built-in search paths, so introduce one Kconfig
>> >> option to allow distributions or users to choose its specific default
>> >> search paths, which are always tried before searching from kernel
>> >> built-in paths in direct loading.
>> >>
>> >> Cc: Linus Torvalds <torvalds@linux-foundation.org>
>> >> Cc: Takashi Iwai <tiwai@suse.de>
>> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> >> ---
>> >>  drivers/base/Kconfig          |   12 +++++++
>> >>  drivers/base/firmware_class.c |   76 ++++++++++++++++++++++++++++++++++++-----
>> >>  2 files changed, 80 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
>> >> index 07abd9d..5b0c909 100644
>> >> --- a/drivers/base/Kconfig
>> >> +++ b/drivers/base/Kconfig
>> >> @@ -156,6 +156,18 @@ config FW_LOADER_USER_HELPER
>> >>         no longer required unless you have a special firmware file that
>> >>         resides in a non-standard path.
>> >>
>> >> +config FW_CUSTOMIZED_PATH
>> >> +     string "default firmware search paths for direct loading"
>> >> +     help
>> >> +       On some distribution(e.g. android), firmware images aren't
>> >> +       put under kernel built-in search paths, so provide this option
>> >> +       for distributions to choose a distribution specific firmware
>> >> +       search path. The option allows to choose more than one path,
>> >> +       and paths are seperated with semicolon(e.g. on android, the
>> >> +       option might look as "/etc/firmware;/vendor/firmware").
>> >> +
>> >> +       If you are unsure about this, don't choose here.
>> >> +
>> >>  config DEBUG_DRIVER
>> >>       bool "Driver Core verbose debug messages"
>> >>       depends on DEBUG_KERNEL
>> >> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> >> index c743409..50b5913 100644
>> >> --- a/drivers/base/firmware_class.c
>> >> +++ b/drivers/base/firmware_class.c
>> >> @@ -267,6 +267,9 @@ static void fw_free_buf(struct firmware_buf *buf)
>> >>  static char fw_path_para[256];
>> >>  static const char * const fw_path[] = {
>> >>       fw_path_para,
>> >> +#ifdef CONFIG_FW_CUSTOMIZED_PATH
>> >> +     CONFIG_FW_CUSTOMIZED_PATH,
>> >> +#endif
>> >>       "/lib/firmware/updates/" UTS_RELEASE,
>> >>       "/lib/firmware/updates",
>> >>       "/lib/firmware/" UTS_RELEASE,
>> >> @@ -314,6 +317,59 @@ static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf
>> >>       return true;
>> >>  }
>> >>
>> >> +static bool fw_get_file_firmware(const char *path,
>> >> +                              struct firmware_buf *buf)
>> >> +{
>> >> +     struct file *file;
>> >> +     bool success;
>> >> +
>> >> +     file = filp_open(path, O_RDONLY, 0);
>> >> +     if (IS_ERR(file))
>> >> +             return false;
>> >> +     success = fw_read_file_contents(file, buf);
>> >> +     fput(file);
>> >> +
>> >> +     return success;
>> >> +}
>> >> +
>> >> +#ifdef CONFIG_FW_CUSTOMIZED_PATH
>> >> +/* The path in @paths is seperated by ';' */
>> >> +static bool fw_get_fw_file_from_paths(const char *paths, char *path,
>> >> +                                   struct firmware_buf *buf)
>> >> +{
>> >> +     int len, start, end;
>> >> +     char *pos;
>> >> +
>> >> +     end = -1;
>> >> +     do {
>> >> +             start = end + 1;
>> >> +             pos = strchr(&paths[start], ';');
>> >> +             if (pos) {
>> >> +                     end = (int)(pos - paths);
>> >> +                     len = end - start;
>> >> +             } else {
>> >> +                     len = strlen(&paths[start]);
>> >> +             }
>> >> +
>> >> +             if (PATH_MAX < len + strlen(buf->fw_id))
>> >> +                     continue;
>> >> +             strncpy(path, &paths[start], len);
>> >> +             snprintf(&path[len], PATH_MAX - len, "/%s", buf->fw_id);
>> >> +
>> >> +             if (fw_get_file_firmware(path, buf))
>> >> +                     return true;
>> >> +     } while (pos && end < strlen(paths) - 1);
>> >> +
>> >> +     return false;
>> >> +}
>> >> +#else
>> >> +static bool fw_get_fw_file_from_paths(const char *paths, char *path,
>> >> +                                   struct firmware_buf *buf)
>> >> +{
>> >> +     return false;
>> >> +}
>> >> +#endif
>> >> +
>> >>  static bool fw_get_filesystem_firmware(struct device *device,
>> >>                                      struct firmware_buf *buf)
>> >>  {
>> >> @@ -322,19 +378,23 @@ static bool fw_get_filesystem_firmware(struct device *device,
>> >>       char *path = __getname();
>> >>
>> >>       for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
>> >> -             struct file *file;
>> >>
>> >>               /* skip the unset customized path */
>> >>               if (!fw_path[i][0])
>> >>                       continue;
>> >>
>> >> -             snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
>> >> -
>> >> -             file = filp_open(path, O_RDONLY, 0);
>> >> -             if (IS_ERR(file))
>> >> -                     continue;
>> >> -             success = fw_read_file_contents(file, buf);
>> >> -             fput(file);
>> >> +             /*
>> >> +              * If CONFIG_FW_CUSTOMIZED_PATH is set, search from
>> >> +              * these paths first
>> >> +              */
>> >> +             if (i == 1 && ARRAY_SIZE(fw_path) > 5) {
>> >> +                     success = fw_get_fw_file_from_paths(fw_path[1],
>> >> +                                                         path, buf);
>> >> +             } else {
>> >> +                     snprintf(path, PATH_MAX, "%s/%s", fw_path[i],
>> >> +                              buf->fw_id);
>> >> +                     success = fw_get_file_firmware(path, buf);
>> >
>> > Shouldn't fw_get_fw_file_from_paths() be applied unconditionally?
>> > It'll be benefit for the customized path passed via module option,
>> > too.  The only drawback is a slight code growth, but the code can be
>> > reduced a bit with strcspn() or such.
>>
>> You mean that both the 1st two items should be covered by
>> fw_get_fw_file_from_paths()? If so, the function may become
>> a bit ugly since two strings are required to pass in, and we can't
>> merge one runtime string and one ro string created in compiling.
>
> I meant to simply call fw_get_fw_file_from_paths() for all fw_path[]
> entries.  So far, the module option can pass only a single path.
> But if it's handled through fw_get_file_from_paths(), you can pass
> multiple paths there, too.
>
>> Looks we can let fw_get_fw_file_from_paths() handle all
>> predefined paths(CONFIG_FW_CUSTOMIZED_PATH and
>> kernel built-in paths), then fw_get_filesystem_firmware()
>> may become simple, just check fw_path_para and all
>> other paths by fw_get_fw_file_from_paths(). How about the
>> idea?
>>
>> > BTW, I now wonder what happens if you pass a relative path.
>> > Did you already test it?
>>
>> I tested absolute paths, and not test relative paths. Do you mean
>> it may cause security problem?
>
> Yes.  It just came to my mind while reviewing your patch.
>
>> If so, we can check and ignore them,
>> but it should be OK since the paths are provided by kernel builder.
>> >From view of function, I don't think there are much difference with
>> absolute paths.
>
> The path can be provided via module option, too, so we need to check
> it in anyway.

Yes, even the firmware name may includes "..".


Thanks,
--
Ming Lei

Re: [PATCH 3/3] firmware loader: allow distribution to choose default search paths

[Ming Lei]

Hi,

On Wed, Jun 5, 2013 at 5:35 PM, Ming Lei <ming.lei@canonical.com> wrote:
>
> Looks we can let fw_get_fw_file_from_paths() handle all
> predefined paths(CONFIG_FW_CUSTOMIZED_PATH and
> kernel built-in paths), then fw_get_filesystem_firmware()
> may become simple, just check fw_path_para and all
> other paths by fw_get_fw_file_from_paths(). How about the
> idea?

So follows the revised patch for review, which should be cleaner
than before, :-)

Any comments?

---
 drivers/base/Kconfig          |   14 +++++++
 drivers/base/firmware_class.c |   87 +++++++++++++++++++++++++++++------------
 2 files changed, 76 insertions(+), 25 deletions(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 07abd9d..b66e63e 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -156,6 +156,20 @@ config FW_LOADER_USER_HELPER
 	  no longer required unless you have a special firmware file that
 	  resides in a non-standard path.

+config FW_CUSTOMIZED_PATH
+	string "default firmware search paths for direct loading"
+	help
+	  On some distribution(e.g. android), firmware images aren't
+	  put under kernel built-in search paths, so provide this option
+	  for distributions to choose a distribution specific firmware
+	  search path. The option allows to choose more than one path,
+	  and paths are seperated with semicolon(e.g. on android, the
+	  option might look as "/etc/firmware;/vendor/firmware"). Each
+	  path should be a absolute path, and relative path will be
+	  ignored.
+
+	  If you are unsure about this, don't choose here.
+
 config DEBUG_DRIVER
 	bool "Driver Core verbose debug messages"
 	depends on DEBUG_KERNEL
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index c743409..218dc1b 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -265,13 +265,14 @@ static void fw_free_buf(struct firmware_buf *buf)

 /* 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 char * const fw_paths =
+#ifdef CONFIG_FW_CUSTOMIZED_PATH
+	CONFIG_FW_CUSTOMIZED_PATH ";"
+#endif
+	"/lib/firmware/updates/" UTS_RELEASE ";"
+	"/lib/firmware/updates;"
+	"/lib/firmware/" UTS_RELEASE ";"
+	"/lib/firmware";

 /*
  * Typical usage is that passing 'firmware_class.path=$CUSTOMIZED_PATH'
@@ -314,30 +315,66 @@ static bool fw_read_file_contents(struct file
*file, struct firmware_buf *fw_buf
 	return true;
 }

-static bool fw_get_filesystem_firmware(struct device *device,
-				       struct firmware_buf *buf)
+static bool fw_get_file_firmware(const char *path,
+				 struct firmware_buf *buf)
 {
-	int i;
-	bool success = false;
-	char *path = __getname();
+	struct file *file;
+	bool success;
+
+	file = filp_open(path, O_RDONLY, 0);
+	if (IS_ERR(file))
+		return false;
+	success = fw_read_file_contents(file, buf);
+	fput(file);

-	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
-		struct file *file;
+	return success;
+}

-		/* skip the unset customized path */
-		if (!fw_path[i][0])
-			continue;
+/* The path in @paths is seperated by ';' */
+static bool fw_get_fw_file_from_paths(const char *paths, char *path,
+				      struct firmware_buf *buf)
+{
+	int len, start, end = -1;
+	char *pos;
+
+	if (!paths[0])
+		return false;

-		snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
+	do {
+		start = end + 1;
+		pos = strchr(&paths[start], ';');
+		if (pos) {
+			end = (int)(pos - paths);
+			len = end - start;
+		} else {
+			len = strlen(&paths[start]);
+		}

-		file = filp_open(path, O_RDONLY, 0);
-		if (IS_ERR(file))
+		if (len <= 0 || PATH_MAX < len + strlen(buf->fw_id))
 			continue;
-		success = fw_read_file_contents(file, buf);
-		fput(file);
-		if (success)
-			break;
-	}
+		strncpy(path, &paths[start], len);
+		snprintf(&path[len], PATH_MAX - len, "/%s", buf->fw_id);
+		/* don't search relative path */
+		if (strstr(path, ".."))
+			continue;
+
+		if (fw_get_file_firmware(path, buf))
+			return true;
+	} while (pos && end < strlen(paths) - 1);
+
+	return false;
+}
+
+static bool fw_get_filesystem_firmware(struct device *device,
+				       struct firmware_buf *buf)
+{
+	bool success = false;
+	char *path = __getname();
+
+	/* search runtime paths first, then static pre-defined paths */
+	success = fw_get_fw_file_from_paths(fw_path_para, path, buf);
+	if (!success)
+		success = fw_get_fw_file_from_paths(fw_paths, path, buf);
 	__putname(path);

 	if (success) {
-- 
1.7.9.5


Thanks,
--
Ming Lei

Re: [PATCH 3/3] firmware loader: allow distribution to choose default search paths

[Takashi Iwai]

At Wed, 5 Jun 2013 18:28:46 +0800,
Ming Lei wrote:
> 
> Hi,
> 
> On Wed, Jun 5, 2013 at 5:35 PM, Ming Lei <ming.lei@canonical.com> wrote:
> >
> > Looks we can let fw_get_fw_file_from_paths() handle all
> > predefined paths(CONFIG_FW_CUSTOMIZED_PATH and
> > kernel built-in paths), then fw_get_filesystem_firmware()
> > may become simple, just check fw_path_para and all
> > other paths by fw_get_fw_file_from_paths(). How about the
> > idea?
> 
> So follows the revised patch for review, which should be cleaner
> than before, :-)

Hmm, separating with semicolons isn't so common, but we can live with
it.  I thought the previous loop can be kept but just pass each member
to fw_get_file_firmware() unconditionally.  This will reduce the
changes, I guess.  But it's just a matter of taste, here is no
critically hot path, after all.

> 
> Any comments?
> 
> ---
>  drivers/base/Kconfig          |   14 +++++++
>  drivers/base/firmware_class.c |   87 +++++++++++++++++++++++++++++------------
>  2 files changed, 76 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 07abd9d..b66e63e 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -156,6 +156,20 @@ config FW_LOADER_USER_HELPER
>  	  no longer required unless you have a special firmware file that
>  	  resides in a non-standard path.
> 
> +config FW_CUSTOMIZED_PATH
> +	string "default firmware search paths for direct loading"
> +	help
> +	  On some distribution(e.g. android), firmware images aren't
> +	  put under kernel built-in search paths, so provide this option
> +	  for distributions to choose a distribution specific firmware
> +	  search path. The option allows to choose more than one path,
> +	  and paths are seperated with semicolon(e.g. on android, the
> +	  option might look as "/etc/firmware;/vendor/firmware"). Each
> +	  path should be a absolute path, and relative path will be
> +	  ignored.
> +
> +	  If you are unsure about this, don't choose here.
> +
>  config DEBUG_DRIVER
>  	bool "Driver Core verbose debug messages"
>  	depends on DEBUG_KERNEL
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index c743409..218dc1b 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -265,13 +265,14 @@ static void fw_free_buf(struct firmware_buf *buf)
> 
>  /* 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 char * const fw_paths =
> +#ifdef CONFIG_FW_CUSTOMIZED_PATH
> +	CONFIG_FW_CUSTOMIZED_PATH ";"
> +#endif
> +	"/lib/firmware/updates/" UTS_RELEASE ";"
> +	"/lib/firmware/updates;"
> +	"/lib/firmware/" UTS_RELEASE ";"
> +	"/lib/firmware";
> 
>  /*
>   * Typical usage is that passing 'firmware_class.path=$CUSTOMIZED_PATH'
> @@ -314,30 +315,66 @@ static bool fw_read_file_contents(struct file
> *file, struct firmware_buf *fw_buf
>  	return true;
>  }
> 
> -static bool fw_get_filesystem_firmware(struct device *device,
> -				       struct firmware_buf *buf)
> +static bool fw_get_file_firmware(const char *path,
> +				 struct firmware_buf *buf)
>  {
> -	int i;
> -	bool success = false;
> -	char *path = __getname();
> +	struct file *file;
> +	bool success;
> +
> +	file = filp_open(path, O_RDONLY, 0);
> +	if (IS_ERR(file))
> +		return false;
> +	success = fw_read_file_contents(file, buf);
> +	fput(file);
> 
> -	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
> -		struct file *file;
> +	return success;
> +}
> 
> -		/* skip the unset customized path */
> -		if (!fw_path[i][0])
> -			continue;
> +/* The path in @paths is seperated by ';' */
> +static bool fw_get_fw_file_from_paths(const char *paths, char *path,
> +				      struct firmware_buf *buf)
> +{
> +	int len, start, end = -1;
> +	char *pos;
> +
> +	if (!paths[0])
> +		return false;
> 
> -		snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
> +	do {
> +		start = end + 1;
> +		pos = strchr(&paths[start], ';');
> +		if (pos) {
> +			end = (int)(pos - paths);
> +			len = end - start;
> +		} else {
> +			len = strlen(&paths[start]);
> +		}
> 
> -		file = filp_open(path, O_RDONLY, 0);
> -		if (IS_ERR(file))
> +		if (len <= 0 || PATH_MAX < len + strlen(buf->fw_id))
>  			continue;
> -		success = fw_read_file_contents(file, buf);
> -		fput(file);
> -		if (success)
> -			break;
> -	}
> +		strncpy(path, &paths[start], len);
> +		snprintf(&path[len], PATH_MAX - len, "/%s", buf->fw_id);
> +		/* don't search relative path */
> +		if (strstr(path, ".."))
> +			continue;

I expect you'll prepare this path sanity check part in a separate
patch, right?  It's basically independent from the new kconfig.


thanks,

Takashi


> +
> +		if (fw_get_file_firmware(path, buf))
> +			return true;
> +	} while (pos && end < strlen(paths) - 1);
> +
> +	return false;
> +}
> +
> +static bool fw_get_filesystem_firmware(struct device *device,
> +				       struct firmware_buf *buf)
> +{
> +	bool success = false;
> +	char *path = __getname();
> +
> +	/* search runtime paths first, then static pre-defined paths */
> +	success = fw_get_fw_file_from_paths(fw_path_para, path, buf);
> +	if (!success)
> +		success = fw_get_fw_file_from_paths(fw_paths, path, buf);
>  	__putname(path);
> 
>  	if (success) {
> -- 
> 1.7.9.5
> 
> 
> Thanks,
> --
> Ming Lei
> 

Re: [PATCH 3/3] firmware loader: allow distribution to choose default search paths

[Ming Lei]

On Wed, Jun 5, 2013 at 7:57 PM, Takashi Iwai <tiwai@suse.de> wrote:
>
> Hmm, separating with semicolons isn't so common, but we can live with

Looks there are seldom such usage patterns in kernel, so could you let me
know which separator is more common?

> it.  I thought the previous loop can be kept but just pass each member
> to fw_get_file_firmware() unconditionally.  This will reduce the
> changes, I guess.  But it's just a matter of taste, here is no
> critically hot path, after all.

Yes, aother way is to move 'fw_path_para' out of fw_path[], and only
let  fw_get_fw_file_from_paths() deal with 'fw_path_para' and
CONFIG_FW_CUSTOMIZED_PATH. Looks it is still clean.


>> +             if (strstr(path, ".."))
>> +                     continue;
>
> I expect you'll prepare this path sanity check part in a separate
> patch, right?  It's basically independent from the new kconfig.

Yes, it will be submitted as another patch, also I plan to add
the check on firmware_name in _request_firmware_prepare()
so that the similar problem can be found earlier.

Thanks,
--
Ming Lei

Re: [PATCH 3/3] firmware loader: allow distribution to choose default search paths

[Takashi Iwai]

At Wed, 5 Jun 2013 20:36:19 +0800,
Ming Lei wrote:
> 
> On Wed, Jun 5, 2013 at 7:57 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >
> > Hmm, separating with semicolons isn't so common, but we can live with
> 
> Looks there are seldom such usage patterns in kernel, so could you let me
> know which separator is more common?

$PATH uses colon, but I don't know of many others.

> > it.  I thought the previous loop can be kept but just pass each member
> > to fw_get_file_firmware() unconditionally.  This will reduce the
> > changes, I guess.  But it's just a matter of taste, here is no
> > critically hot path, after all.
> 
> Yes, aother way is to move 'fw_path_para' out of fw_path[], and only
> let  fw_get_fw_file_from_paths() deal with 'fw_path_para' and
> CONFIG_FW_CUSTOMIZED_PATH. Looks it is still clean.

Why not just pass all strings to fw_get_fw_file_from_paths(), no
matter whether it's a single or multiple path strings?

static const char * const fw_path[] = {
	fw_path_para,
#ifdef CONFIG_FW_CUSTOMIZED_PATH
	CONFIG_FW_CUSTOMIZED_PATH,
#endif
	"/lib/firmware/updates/" UTS_RELEASE,
	"/lib/firmware/updates",
	"/lib/firmware/" UTS_RELEASE,
};

....

	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
		/* skip the unset customized path */
		if (!fw_path[i][0])
			continue;
		success = fw_get_fw_file_from_paths(fw_path[i], path, buf);
		if (success)
			break;
	}

> 
> 
> >> +             if (strstr(path, ".."))
> >> +                     continue;
> >
> > I expect you'll prepare this path sanity check part in a separate
> > patch, right?  It's basically independent from the new kconfig.
> 
> Yes, it will be submitted as another patch, also I plan to add
> the check on firmware_name in _request_firmware_prepare()
> so that the similar problem can be found earlier.

Sounds good.


Takashi

Re: [PATCH 3/3] firmware loader: allow distribution to choose default search paths

[Ming Lei]

On Wed, Jun 5, 2013 at 8:45 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Wed, 5 Jun 2013 20:36:19 +0800,
> Ming Lei wrote:
>>
>> On Wed, Jun 5, 2013 at 7:57 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> >
>> > Hmm, separating with semicolons isn't so common, but we can live with
>>
>> Looks there are seldom such usage patterns in kernel, so could you let me
>> know which separator is more common?
>
> $PATH uses colon, but I don't know of many others.

OK, will change to colon to keep consistent with $PATH.

>
>> > it.  I thought the previous loop can be kept but just pass each member
>> > to fw_get_file_firmware() unconditionally.  This will reduce the
>> > changes, I guess.  But it's just a matter of taste, here is no
>> > critically hot path, after all.
>>
>> Yes, aother way is to move 'fw_path_para' out of fw_path[], and only
>> let  fw_get_fw_file_from_paths() deal with 'fw_path_para' and
>> CONFIG_FW_CUSTOMIZED_PATH. Looks it is still clean.
>
> Why not just pass all strings to fw_get_fw_file_from_paths(), no
> matter whether it's a single or multiple path strings?
>
> static const char * const fw_path[] = {
>         fw_path_para,
> #ifdef CONFIG_FW_CUSTOMIZED_PATH
>         CONFIG_FW_CUSTOMIZED_PATH,
> #endif
>         "/lib/firmware/updates/" UTS_RELEASE,
>         "/lib/firmware/updates",
>         "/lib/firmware/" UTS_RELEASE,
> };
>
> ....
>
>         for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
>                 /* skip the unset customized path */
>                 if (!fw_path[i][0])
>                         continue;
>                 success = fw_get_fw_file_from_paths(fw_path[i], path, buf);
>                 if (success)
>                         break;
>         }

Good, this approach is very clean, and I will take it, :-)

Thanks,
--
Ming Lei