| 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: <a5e933fe-4566-9ae6-9a5d-b3a4c186fe0b@intel.com>
Date: Tue, 3 Oct 2023 10:14:05 -0700
From: Jesse Brandeburg <jesse.brandeburg@...el.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
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()
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.
your patch still shows these errors
> 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