linux-kernel - RE: [PATCH net-next] i40e: i40e_reset_vf should return false if reset vf 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] [day] [month] [year] [list]

Message-ID: <DM6PR11MB311352D9E711CD02F740600087869@DM6PR11MB3113.namprd11.prod.outlook.com>
Date:   Tue, 12 Jul 2022 00:03:27 +0000
From:   "Laba, SlawomirX" <slawomirx.laba@...el.com>
To:     Zhengchao Shao <shaozhengchao@...wei.com>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "hawk@...nel.org" <hawk@...nel.org>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>
CC:     "weiyongjun1@...wei.com" <weiyongjun1@...wei.com>,
        "yuehaibing@...wei.com" <yuehaibing@...wei.com>
Subject: RE: [PATCH net-next] i40e: i40e_reset_vf should return false if reset
 vf timeout

> -----Original Message-----
> From: Zhengchao Shao <shaozhengchao@...wei.com>
> Sent: Tuesday, July 5, 2022 3:13 AM
> To: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; linux-
> kernel@...r.kernel.org; bpf@...r.kernel.org; davem@...emloft.net;
> edumazet@...gle.com; kuba@...nel.org; pabeni@...hat.com; Brandeburg,
> Jesse <jesse.brandeburg@...el.com>; Nguyen, Anthony L
> <anthony.l.nguyen@...el.com>; ast@...nel.org; daniel@...earbox.net;
> hawk@...nel.org; john.fastabend@...il.com
> Cc: weiyongjun1@...wei.com; yuehaibing@...wei.com;
> shaozhengchao@...wei.com
> Subject: [PATCH net-next] i40e: i40e_reset_vf should return false if reset vf
> timeout
> 
> when trigger vf reset, but vf status is not ready, i40e_reset_vf should not do
> other cleanup action. The current logic is always return true. But it can't cover

Are you sure the queues are stopped/disabled when the VF timeouts?
This could lead to DMA corruption.
It is hard for me to elaborate on the code path because you haven't provided a test case.
Could you please provide reproduction steps along with the description of the issue?

> timeout scenary, and the looping in function i40e_vc_reset_vf is useless.
> Waiting for 120ms will cover most normal scenary. And the caller function
> should try again when timeout or accept that resetting vf failed.
> 
> Signed-off-by: Zhengchao Shao <shaozhengchao@...wei.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index d01fb592778c..42262009a00c 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -1564,11 +1564,17 @@ bool i40e_reset_vf(struct i40e_vf *vf, bool flr)
>  	if (flr)
>  		usleep_range(10000, 20000);
> 
> -	if (!rsd)
> -		dev_err(&pf->pdev->dev, "VF reset check timeout on VF %d\n",
> -			vf->vf_id);
>  	usleep_range(10000, 20000);
> 
> +	if (!rsd) {
> +		reg = rd32(hw, I40E_VPGEN_VFRSTAT(vf->vf_id));
> +		if (!(reg & I40E_VPGEN_VFRSTAT_VFRD_MASK)) {
> +			dev_err(&pf->pdev->dev, "VF reset check timeout on VF
> %d\n",
> +				vf->vf_id);
> +			return false;

I think this could introduce the bug since you are never calling:
clear_bit(__I40E_VF_DISABLE, pf->state);

So the next time you enter this function the bit is still set and you'll return.

This solution could work if you move the cleanups to the i40e_vc_reset_vf when the timeout happens.
Still I'd like to see a test case and what is not working.

> +		}
> +	}
> +
>  	/* On initial reset, we don't have any queues to disable */
>  	if (vf->lan_vsi_idx != 0)
>  		i40e_vsi_stop_rings(pf->vsi[vf->lan_vsi_idx]);
> --
> 2.17.1