| Message ID | 20240112025853.123048-1-chentao@kylinos.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] igb: Fix string truncation warnings in igb_set_fw_version | expand |
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); > } > > /**
Hi Paul, Thanks for your reply. On 2024/1/12 21:46, Paul Menzel wrote: > 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* Thanks i'll rewirte the commit msg and add some warnings. > >> kasprintf() returns a pointer to dynamically allocated memory >> which can be NULL upon failure. > > Maybe paste one warning message. Thanks again. > >> 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); >> } >> /**
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); } /**
'commit 1978d3ead82c ("intel: fix string truncation warnings")' fix '-Wformat-truncation=' warnings in igb_main.c by using kasprintf. kasprintf() returns a pointer to dynamically allocated memory which can be NULL upon failure. 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> --- 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(-)