Received-SPF: pass (google.com: domain of linux-kernel+bounces-24706-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <2a58fed3-1fa9-47eb-b475-3f7c3b291376@molgen.mpg.de>
Date: Fri, 12 Jan 2024 14:46:42 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [Intel-wired-lan] [PATCH v2] igb: Fix string truncation warnings
 in igb_set_fw_version
To: Kunwu Chan <chentao@kylinos.cn>
Cc: jesse.brandeburg@intel.com, anthony.l.nguyen@intel.com,
 davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
 pabeni@redhat.com, Kunwu Chan <kunwu.chan@hotmail.com>,
 przemyslaw.kitszel@intel.com, linux-kernel@vger.kernel.org,
 intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
 jacob.e.keller@intel.com
References: <20240112025853.123048-1-chentao@kylinos.cn>
Content-Language: en-US
From: Paul Menzel <pmenzel@molgen.mpg.de>
In-Reply-To: <20240112025853.123048-1-chentao@kylinos.cn>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Dear Kunwu,


Thank you for your patch. I have some minor nits.

Am 12.01.24 um 03:58 schrieb Kunwu Chan:
> 'commit 1978d3ead82c ("intel: fix string truncation warnings")'

Please don’t enclose it in '': Commit 1978d3ead82c ("intel: fix string 
truncation warnings")

> fix '-Wformat-truncation=' warnings in igb_main.c by using kasprintf.

fix*es*

> kasprintf() returns a pointer to dynamically allocated memory
> which can be NULL upon failure.

Maybe paste one warning message.

> Fix this warning by using a larger space for adapter->fw_version,
> and then fall back and continue to use snprintf.
> 
> Fixes: 1978d3ead82c ("intel: fix string truncation warnings")
> Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
> Cc: Kunwu Chan <kunwu.chan@hotmail.com>
> Suggested-by: Jakub Kicinski <kuba@kernel.org>


Kind regards,

Paul Menzel


> ---
> v2: Fall back to use snprintf and a larger space,as suggested by
> https://lore.kernel.org/all/20231212132637.1b0fb8aa@kernel.org/
> ---
>   drivers/net/ethernet/intel/igb/igb.h      |  2 +-
>   drivers/net/ethernet/intel/igb/igb_main.c | 35 ++++++++++++-----------
>   2 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index a2b759531cb7..3c2dc7bdebb5 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -637,7 +637,7 @@ struct igb_adapter {
>   		struct timespec64 period;
>   	} perout[IGB_N_PEROUT];
>   
> -	char fw_version[32];
> +	char fw_version[48];
>   #ifdef CONFIG_IGB_HWMON
>   	struct hwmon_buff *igb_hwmon_buff;
>   	bool ets;
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index b2295caa2f0a..ce762d77d2c1 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -3069,7 +3069,6 @@ void igb_set_fw_version(struct igb_adapter *adapter)
>   {
>   	struct e1000_hw *hw = &adapter->hw;
>   	struct e1000_fw_version fw;
> -	char *lbuf;
>   
>   	igb_get_fw_version(hw, &fw);
>   
> @@ -3077,34 +3076,36 @@ void igb_set_fw_version(struct igb_adapter *adapter)
>   	case e1000_i210:
>   	case e1000_i211:
>   		if (!(igb_get_flash_presence_i210(hw))) {
> -			lbuf = kasprintf(GFP_KERNEL, "%2d.%2d-%d",
> -					 fw.invm_major, fw.invm_minor,
> -					 fw.invm_img_type);
> +			snprintf(adapter->fw_version,
> +				 sizeof(adapter->fw_version),
> +				 "%2d.%2d-%d",
> +				 fw.invm_major, fw.invm_minor,
> +				 fw.invm_img_type);
>   			break;
>   		}
>   		fallthrough;
>   	default:
>   		/* if option rom is valid, display its version too */
>   		if (fw.or_valid) {
> -			lbuf = kasprintf(GFP_KERNEL, "%d.%d, 0x%08x, %d.%d.%d",
> -					 fw.eep_major, fw.eep_minor,
> -					 fw.etrack_id, fw.or_major, fw.or_build,
> -					 fw.or_patch);
> +			snprintf(adapter->fw_version,
> +				 sizeof(adapter->fw_version),
> +				 "%d.%d, 0x%08x, %d.%d.%d",
> +				 fw.eep_major, fw.eep_minor, fw.etrack_id,
> +				 fw.or_major, fw.or_build, fw.or_patch);
>   		/* no option rom */
>   		} else if (fw.etrack_id != 0X0000) {
> -			lbuf = kasprintf(GFP_KERNEL, "%d.%d, 0x%08x",
> -					 fw.eep_major, fw.eep_minor,
> -					 fw.etrack_id);
> +			snprintf(adapter->fw_version,
> +				 sizeof(adapter->fw_version),
> +				 "%d.%d, 0x%08x",
> +				 fw.eep_major, fw.eep_minor, fw.etrack_id);
>   		} else {
> -			lbuf = kasprintf(GFP_KERNEL, "%d.%d.%d", fw.eep_major,
> -					 fw.eep_minor, fw.eep_build);
> +			snprintf(adapter->fw_version,
> +				 sizeof(adapter->fw_version),
> +				 "%d.%d.%d",
> +				 fw.eep_major, fw.eep_minor, fw.eep_build);
>   		}
>   		break;
>   	}
> -
> -	/* the truncate happens here if it doesn't fit */
> -	strscpy(adapter->fw_version, lbuf, sizeof(adapter->fw_version));
> -	kfree(lbuf);
>   }
>   
>   /**