Re: [PATCH v2] net/ice: support customized search path for DDP package

[Bruce Richardson <bruce.richardson@intel.com>]

On Thu, Aug 29, 2024 at 11:41:58AM +0800, Zhichao Zeng wrote:
> This patch adds support for customizing firmware search path for
> DDP package like the kernel behavior, it will read the search path
> from "/sys/module/firmware_class/parameters/path",
> and try to load DDP package.
> 
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> 

See review comments inline below.

Thanks,
/Bruce

> ---
> v2: separate the patch and rewrite the log
> ---
>  doc/guides/nics/ice.rst      |  5 +++++
>  drivers/net/ice/ice_ethdev.c | 26 ++++++++++++++++++++++++++
>  drivers/net/ice/ice_ethdev.h |  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/doc/guides/nics/ice.rst b/doc/guides/nics/ice.rst
> index ae975d19ad..741cd42cb7 100644
> --- a/doc/guides/nics/ice.rst
> +++ b/doc/guides/nics/ice.rst
> @@ -108,6 +108,11 @@ Runtime Configuration
>  
>      -a 80:00.0,default-mac-disable=1
>  
> +- ``DDP Package File``
> +
> +  Support for customizing the firmware search path, will read the search path
> +  from "/sys/module/firmware_class/parameters/path" and try to load DDP package.
> +

This is no longer a configuration parameter, so this section can be
removed from the doc.

There is a section in the ice docs for "Limitations or Known Issues" where
the search paths for DDP package files are given. That could do with an
update in this patch. Also, the details of DDP searching probably should go
in its own section rather than having it as a limitation - move it further
up the doc to be more accessible to users.

>  - ``Protocol extraction for per queue``
>  
>    Configure the RX queues to do protocol extraction into mbuf for protocol
> diff --git a/drivers/net/ice/ice_ethdev.c b/drivers/net/ice/ice_ethdev.c
> index 304f959b7e..5dfb3d9c21 100644
> --- a/drivers/net/ice/ice_ethdev.c
> +++ b/drivers/net/ice/ice_ethdev.c
> @@ -1873,6 +1873,22 @@ ice_load_pkg_type(struct ice_hw *hw)
>  	return package_type;
>  }
>  
> +static int ice_read_customized_path(char *pkg_file)
> +{
> +	char buf[ICE_MAX_PKG_FILENAME_SIZE];
> +	FILE *fp = fopen(ICE_PKG_FILE_CUSTOMIZED_PATH, "r");
> +	if (fp == NULL) {
> +		PMD_INIT_LOG(ERR, "Failed to read CUSTOMIZED_PATH");

Is this actually an error? Will older kernels not be missing this path, in
which case it's not really a problem at all? I think any message should be
at a much lower logging level to avoid unnecessary warnings to users.

> +		return -EIO;
> +	}
> +	if (fscanf(fp, "%s\n", buf) > 0)

This looks unsafe to me, you are not specifying a maximum string length in
the call to fscanf.
For strings, just use fgets, or alternatively just use regular "open" and
"read" system calls.

> +		strncpy(pkg_file, buf, ICE_MAX_PKG_FILENAME_SIZE);

strncpy is insecure in many cases (such as here!) and should not be used.
This will not null-terminate the string at all. For string copies use safe
functions like "strlcpy".

However, there is no need to do a copy here at all. Remove the variable
"buf" and instead read the value directly into "pkg_file". Using a
temporary variable has no benefit.

NOTE: you also need to change the function signature to pass in the length
of pkg_file variable. When passing a string field to be completed, you
always need to pass in the length value too.

> +	else
> +		return -EIO;
> +
> +	return 0;
> +}
> +
>  int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  {
>  	struct ice_hw *hw = &adapter->hw;
> @@ -1888,6 +1904,12 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  	memset(opt_ddp_filename, 0, ICE_MAX_PKG_FILENAME_SIZE);
>  	snprintf(opt_ddp_filename, ICE_MAX_PKG_FILENAME_SIZE,
>  		"ice-%016" PRIx64 ".pkg", dsn);
> +
> +	ice_read_customized_path(pkg_file);
> +	strcat(pkg_file, opt_ddp_filename);

This is also at risk of buffer overflows. Use strlcat and check for errors
that the paths are not too long.

> +	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
> +		goto load_fw;
> +
>  	strncpy(pkg_file, ICE_PKG_FILE_SEARCH_PATH_UPDATES,
>  		ICE_MAX_PKG_FILENAME_SIZE);
>  	strcat(pkg_file, opt_ddp_filename);
> @@ -1901,6 +1923,10 @@ int ice_load_pkg(struct ice_adapter *adapter, bool use_dsn, uint64_t dsn)
>  		goto load_fw;
>  
>  no_dsn:
> +	ice_read_customized_path(pkg_file);

Minor nit, but why read sysfs twice using system calls. Use a local
variable to save the value from the first read rather than repeating it.

> +	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
> +		goto load_fw;
> +
>  	strncpy(pkg_file, ICE_PKG_FILE_UPDATES, ICE_MAX_PKG_FILENAME_SIZE);
>  	if (rte_firmware_read(pkg_file, &buf, &bufsz) == 0)
>  		goto load_fw;
> diff --git a/drivers/net/ice/ice_ethdev.h b/drivers/net/ice/ice_ethdev.h
> index 3ea9f37dc8..8b644ed700 100644
> --- a/drivers/net/ice/ice_ethdev.h
> +++ b/drivers/net/ice/ice_ethdev.h
> @@ -51,6 +51,7 @@
>  #define ICE_PKG_FILE_UPDATES "/lib/firmware/updates/intel/ice/ddp/ice.pkg"
>  #define ICE_PKG_FILE_SEARCH_PATH_DEFAULT "/lib/firmware/intel/ice/ddp/"
>  #define ICE_PKG_FILE_SEARCH_PATH_UPDATES "/lib/firmware/updates/intel/ice/ddp/"
> +#define ICE_PKG_FILE_CUSTOMIZED_PATH "/sys/module/firmware_class/parameters/path"
>  #define ICE_MAX_PKG_FILENAME_SIZE   256
>  
>  #define MAX_ACL_NORMAL_ENTRIES    256
> -- 
> 2.34.1
>