netdev - Re: [PATCH] net/mlx5: reduce stack usage in FW tracer

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]

Date:   Mon, 9 Sep 2019 19:39:37 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "davem@...emloft.net" <davem@...emloft.net>,
        "arnd@...db.de" <arnd@...db.de>,
        "leon@...nel.org" <leon@...nel.org>
CC:     "cai@....pw" <cai@....pw>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        Moshe Shemesh <moshe@...lanox.com>,
        Feras Daoud <ferasda@...lanox.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Erez Shitrit <erezsh@...lanox.com>
Subject: Re: [PATCH] net/mlx5: reduce stack usage in FW tracer

On Fri, 2019-09-06 at 17:11 +0200, Arnd Bergmann wrote:
> It's generally not ok to put a 512 byte buffer on the stack, as
> kernel
> stack is a scarce resource:
> 
> drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c:660:13:
> error: stack frame size of 1032 bytes in function
> 'mlx5_fw_tracer_handle_traces' [-Werror,-Wframe-larger-than=]
> 
> This is done in a context that is allowed to sleep, so using
> dynamic allocation is ok as well. I'm not too worried about
> runtime overhead, as this already contains an snprintf() and
> other expensive functions.
> 
> Fixes: 70dd6fdb8987 ("net/mlx5: FW tracer, parse traces and kernel
> tracing support")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
>  .../mellanox/mlx5/core/diag/fw_tracer.c       | 21 ++++++++++-------
> --
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> index 2011eaf15cc5..d81e78060f9f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> @@ -557,16 +557,16 @@ static void mlx5_tracer_print_trace(struct
> tracer_string_format *str_frmt,
>  				    struct mlx5_core_dev *dev,
>  				    u64 trace_timestamp)
>  {
> -	char	tmp[512];
> -

Hi Arnd, thanks for the patch, 
this function is very perfomance critical when fw traces are activated
to pull some fw content on error situations, using kmalloc here might
become a problem and stall the system further more if the problem was
initially due to lack of memory.

since this function only needs 512 bytes maybe we should mark it as
noinline to avoid any extra stack usages on the caller function
mlx5_fw_tracer_handle_traces ?

> -	snprintf(tmp, sizeof(tmp), str_frmt->string,
> -		 str_frmt->params[0],
> -		 str_frmt->params[1],
> -		 str_frmt->params[2],
> -		 str_frmt->params[3],
> -		 str_frmt->params[4],
> -		 str_frmt->params[5],
> -		 str_frmt->params[6]);
> +	char *tmp = kasprintf(GFP_KERNEL, str_frmt->string,
> +			      str_frmt->params[0],
> +			      str_frmt->params[1],
> +			      str_frmt->params[2],
> +			      str_frmt->params[3],
> +			      str_frmt->params[4],
> +			      str_frmt->params[5],
> +			      str_frmt->params[6]);
> +	if (!tmp)
> +		return;
>  
>  	trace_mlx5_fw(dev->tracer, trace_timestamp, str_frmt->lost,
>  		      str_frmt->event_id, tmp);
> @@ -576,6 +576,7 @@ static void mlx5_tracer_print_trace(struct
> tracer_string_format *str_frmt,
>  
>  	/* remove it from hash */
>  	mlx5_tracer_clean_message(str_frmt);
> +	kfree(tmp);
>  }
>  
>  static int mlx5_tracer_handle_string_trace(struct mlx5_fw_tracer
> *tracer,