netdev - Re: [for-next V2 03/13] net/mlx5e: Wait for FPGA command responses with a timeout

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: <1520452915.24565.48.camel@mellanox.com>
Date:   Wed, 7 Mar 2018 20:01:57 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "ktkhai@...tuozzo.com" <ktkhai@...tuozzo.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "dledford@...hat.com" <dledford@...hat.com>
CC:     Jason Gunthorpe <jgg@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Matan Barak <matanb@...lanox.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        Leon Romanovsky <leonro@...lanox.com>,
        Aviad Yehezkel <aviadye@...lanox.com>,
        Boris Pismenny <borisp@...lanox.com>
Subject: Re: [for-next V2 03/13] net/mlx5e: Wait for FPGA command responses
 with a timeout

On Wed, 2018-03-07 at 13:25 +0300, Kirill Tkhai wrote:
> On 07.03.2018 09:35, Saeed Mahameed wrote:
> > From: Aviad Yehezkel <aviadye@...lanox.com>
> > 
> > Generally, FPGA IPSec commands must always complete.
> > We want to wait for one minute for them to complete gracefully also
> > when killing a process.
> > 
> > Signed-off-by: Aviad Yehezkel <aviadye@...lanox.com>
> > Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
> > ---
> >  drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
> > index 35d0e33381ca..95f9c5a8619b 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/ipsec.c
> > @@ -39,6 +39,7 @@
> >  #include "fpga/core.h"
> >  
> >  #define SBU_QP_QUEUE_SIZE 8
> > +#define MLX5_FPGA_IPSEC_CMD_TIMEOUT_MSEC	(60 * 1000)
> >  
> >  enum mlx5_ipsec_response_syndrome {
> >  	MLX5_IPSEC_RESPONSE_SUCCESS = 0,
> > @@ -217,12 +218,14 @@ void *mlx5_fpga_ipsec_sa_cmd_exec(struct
> > mlx5_core_dev *mdev,
> >  int mlx5_fpga_ipsec_sa_cmd_wait(void *ctx)
> >  {
> >  	struct mlx5_ipsec_command_context *context = ctx;
> > +	unsigned long timeout =
> > +		msecs_to_jiffies(MLX5_FPGA_IPSEC_CMD_TIMEOUT_MSEC)
> > ;
> >  	int res;
> >  
> > -	res = wait_for_completion_killable(&context->complete);
> > -	if (res) {
> > +	res = wait_for_completion_timeout(&context->complete,
> > timeout);
> 
> You had a possibility to kill the waiting task before the patch, but
> it gets lost after the patch.
> Why not wait_for_completion_killable_timeout()?
> 

Actually allowing the process to kill while an FPGA command execution
is in progress is a bug, see commit message:
"We want to wait for one minute for them to complete gracefully also
when killing a process."

Abandoning the command execution from this function is wrong, which is
exactly what this patch is for.

> > +	if (!res) {
> >  		mlx5_fpga_warn(context->dev, "Failure waiting for
> > IPSec command response\n");
> > -		return -EINTR;
> > +		return -ETIMEDOUT;
> >  	}
> >  
> >  	if (context->status == MLX5_FPGA_IPSEC_SACMD_COMPLETE)
> 
> Thanks,
> Kirill