| 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
| ||
|
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