lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <abf8d279-b579-4a03-9ae9-053cf5efec3d@wanadoo.fr>
Date: Tue, 3 Oct 2023 22:33:47 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Jesse Brandeburg <jesse.brandeburg@...el.com>,
 Tony Nguyen <anthony.l.nguyen@...el.com>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
 intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] iavf: Avoid a memory allocation in
 iavf_print_link_message()

Le 03/10/2023 à 19:14, Jesse Brandeburg a écrit :
> On 9/23/2023 5:17 AM, Christophe JAILLET wrote:
>> IAVF_MAX_SPEED_STRLEN is only 13 and 'speed' is allocated and freed within
>> iavf_print_link_message().
>>
>> 'speed' is only used with some snprintf() and netdev_info() calls.
>>
>> So there is no real use to kzalloc()/free() it. Use the stack instead.
>> This saves a memory allocation.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>> ---
>>   drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
>> index 8ce6389b5815..980dc69d7fbe 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
>> @@ -1389,18 +1389,14 @@ void iavf_disable_vlan_insertion_v2(struct iavf_adapter *adapter, u16 tpid)
>>   static void iavf_print_link_message(struct iavf_adapter *adapter)
>>   {
>>   	struct net_device *netdev = adapter->netdev;
>> +	char speed[IAVF_MAX_SPEED_STRLEN];
>>   	int link_speed_mbps;
>> -	char *speed;
>>   
>>   	if (!adapter->link_up) {
>>   		netdev_info(netdev, "NIC Link is Down\n");
>>   		return;
>>   	}
>>   
>> -	speed = kzalloc(IAVF_MAX_SPEED_STRLEN, GFP_KERNEL);
>> -	if (!speed)
>> -		return;
>> -
>>   	if (ADV_LINK_SUPPORT(adapter)) {
>>   		link_speed_mbps = adapter->link_speed_mbps;
>>   		goto print_link_msg;
>> @@ -1452,7 +1448,6 @@ static void iavf_print_link_message(struct iavf_adapter *adapter)
>>   	}
>>   
>>   	netdev_info(netdev, "NIC Link is Up Speed is %s Full Duplex\n", speed);
>> -	kfree(speed);
>>   }
>>   
>>   /**
> 
> Hi Christophe!
> 
> I had a slightly different proposal that gets rid of all the -Wformat=2
> warnings in this code by using kasprintf to handle the varying string
> lengths.
> 
> any thoughts about this instead and drop yours? I'm less worried about
> the "extra allocation" here in this function since it's slow path, and
> the same comment applies to your patch as well.

kasprintf() is much better.

> 
> your patch still shows these errors

I built-tested the patch before sending, so this is strange.

However, I got a similar feedback from Greg KH and the "kernel test 
robot" for another similar patch.

What version of gcc do you use?
I use 12.3.0, and I suspect that the value range algorithm or how the 
diagnostic is done has been improved in recent gcc.

The other report was from 11.3.0.

CJ

>> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c: In function ‘iavf_virtchnl_completion’:
>> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:60: warning: ‘%s’ directive output may be truncated writing 4 bytes into a region of size between 1 and 11 [-Wformat-truncation=]
>>   1446 |                 snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s",
>>        |                                                            ^~
>>   1447 |                          link_speed_mbps, "Mbps");
>>        |                                           ~~~~~~
>> In function ‘iavf_print_link_message’,
>>      inlined from ‘iavf_virtchnl_completion’ at drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1965:4:
>> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:17: note: ‘snprintf’ output between 7 and 17 bytes into a destination of size 13
>>   1446 |                 snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s",
>>        |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>   1447 |                          link_speed_mbps, "Mbps");
>>        |                          ~~~~~~~~~~~~~~~~~~~~~~~~
> 
> 
> <my iavf patch pasted as a quote so my mail client won't wrap the lines...>
> 
> 
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
>> index 8ce6389b5815..82b84a93bcc8 100644
>> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
>> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
>> @@ -1378,8 +1378,6 @@ void iavf_disable_vlan_insertion_v2(struct iavf_adapter *adapter, u16 tpid)
>>                                    VIRTCHNL_OP_DISABLE_VLAN_INSERTION_V2);
>>   }
>>
>> -#define IAVF_MAX_SPEED_STRLEN  13
>> -
>>   /**
>>    * iavf_print_link_message - print link up or down
>>    * @adapter: adapter structure
>> @@ -1397,10 +1395,6 @@ static void iavf_print_link_message(struct iavf_adapter *adapter)
>>                  return;
>>          }
>>
>> -       speed = kzalloc(IAVF_MAX_SPEED_STRLEN, GFP_KERNEL);
>> -       if (!speed)
>> -               return;
>> -
>>          if (ADV_LINK_SUPPORT(adapter)) {
>>                  link_speed_mbps = adapter->link_speed_mbps;
>>                  goto print_link_msg;
>> @@ -1438,17 +1432,17 @@ static void iavf_print_link_message(struct iavf_adapter *adapter)
>>
>>   print_link_msg:
>>          if (link_speed_mbps > SPEED_1000) {
>> -               if (link_speed_mbps == SPEED_2500)
>> -                       snprintf(speed, IAVF_MAX_SPEED_STRLEN, "2.5 Gbps");
>> -               else
>> +               if (link_speed_mbps == SPEED_2500) {
>> +                       speed = kasprintf(GFP_KERNEL, "%s", "2.5 Gbps");
>> +               } else {
>>                          /* convert to Gbps inline */
>> -                       snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s",
>> -                                link_speed_mbps / 1000, "Gbps");
>> +                       speed = kasprintf(GFP_KERNEL, "%d Gbps",
>> +                                         link_speed_mbps / 1000);
>> +               }
>>          } else if (link_speed_mbps == SPEED_UNKNOWN) {
>> -               snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%s", "Unknown Mbps");
>> +               speed = kasprintf(GFP_KERNEL, "%s", "Unknown Mbps");
>>          } else {
>> -               snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s",
>> -                        link_speed_mbps, "Mbps");
>> +               speed = kasprintf(GFP_KERNEL, "%d Mbps", link_speed_mbps);
>>          }
>>
>>          netdev_info(netdev, "NIC Link is Up Speed is %s Full Duplex\n", speed);
> 
> 
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ