| 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: <b03abadc-eb9a-4025-afed-a4eb7c4a5e2c@intel.com>
Date: Mon, 11 Mar 2024 13:34:03 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Aleksandr Loktionov <aleksandr.loktionov@...el.com>,
<intel-wired-lan@...ts.osuosl.org>, <anthony.l.nguyen@...el.com>
CC: <netdev@...r.kernel.org>
Subject: Re: [PATCH iwl-net] i40e: fix vf may be used uninitialized in this
function warning
On 3/11/24 12:25, Aleksandr Loktionov wrote:
> To fix the regression introduced by 52424f974bc5 commit, wchich causes
> servers hang in very hard to reproduce conditions with resets races.
>
> Remove redundant "v" variable and iterate via single VF pointer across
> whole function instead to guarantee VF pointer validity.
>
> Fixes: 52424f974bc5 ("i40e: Fix VF hang when reset is triggered on another VF")
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
> ---
> .../ethernet/intel/i40e/i40e_virtchnl_pf.c | 34 +++++++++----------
> 1 file changed, 16 insertions(+), 18 deletions(-)
>
While reading carefully I could tell that you are indeed only changing
int v; &vf = pf->vf[v]; into pointer, and the scope is limited to just
one function, and commit message makes it clear what the error was.
With that:
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index b34c717..f7c4654 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -1628,105 +1628,103 @@ bool i40e_reset_all_vfs(struct i40e_pf *pf, bool flr)
> {
> struct i40e_hw *hw = &pf->hw;
> struct i40e_vf *vf;
> - int i, v;
> u32 reg;
> + int i;
>
> /* If we don't have any VFs, then there is nothing to reset */
> if (!pf->num_alloc_vfs)
> return false;
>
> /* If VFs have been disabled, there is no need to reset */
> if (test_and_set_bit(__I40E_VF_DISABLE, pf->state))
> return false;
>
> /* Begin reset on all VFs at once */
> - for (v = 0; v < pf->num_alloc_vfs; v++) {
> - vf = &pf->vf[v];
> + for (vf = &pf->vf[0]; vf < &pf->vf[pf->num_alloc_vfs]; ++vf) {
> /* If VF is being reset no need to trigger reset again */
> if (!test_bit(I40E_VF_STATE_RESETTING, &vf->vf_states))
> - i40e_trigger_vf_reset(&pf->vf[v], flr);
> + i40e_trigger_vf_reset(vf, flr);
> }
>
> /* HW requires some time to make sure it can flush the FIFO for a VF
> * when it resets it. Poll the VPGEN_VFRSTAT register for each VF in
> * sequence to make sure that it has completed. We'll keep track of
> * the VFs using a simple iterator that increments once that VF has
> * finished resetting.
> */
> - for (i = 0, v = 0; i < 10 && v < pf->num_alloc_vfs; i++) {
> + for (i = 0, vf = &pf->vf[0]; i < 10 && vf < &pf->vf[pf->num_alloc_vfs]; ++i) {
> usleep_range(10000, 20000);
>
> /* Check each VF in sequence, beginning with the VF to fail
> * the previous check.
> */
> - while (v < pf->num_alloc_vfs) {
> - vf = &pf->vf[v];
> + while (vf < &pf->vf[pf->num_alloc_vfs]) {
> if (!test_bit(I40E_VF_STATE_RESETTING, &vf->vf_states)) {
> reg = rd32(hw, I40E_VPGEN_VFRSTAT(vf->vf_id));
> if (!(reg & I40E_VPGEN_VFRSTAT_VFRD_MASK))
> break;
> }
>
> /* If the current VF has finished resetting, move on
> * to the next VF in sequence.
> */
> - v++;
> + ++vf;
> }
> }
>
> if (flr)
> usleep_range(10000, 20000);
>
> /* Display a warning if at least one VF didn't manage to reset in
> * time, but continue on with the operation.
> */
> - if (v < pf->num_alloc_vfs)
> + if (vf < &pf->vf[pf->num_alloc_vfs])
> dev_err(&pf->pdev->dev, "VF reset check timeout on VF %d\n",
> - pf->vf[v].vf_id);
> + vf->vf_id);
> usleep_range(10000, 20000);
>
> /* Begin disabling all the rings associated with VFs, but do not wait
> * between each VF.
> */
> - for (v = 0; v < pf->num_alloc_vfs; v++) {
> + for (vf = &pf->vf[0]; vf < &pf->vf[pf->num_alloc_vfs]; ++vf) {
> /* On initial reset, we don't have any queues to disable */
> - if (pf->vf[v].lan_vsi_idx == 0)
> + if (vf->lan_vsi_idx == 0)
> continue;
>
> /* If VF is reset in another thread just continue */
> if (test_bit(I40E_VF_STATE_RESETTING, &vf->vf_states))
> continue;
>
> - i40e_vsi_stop_rings_no_wait(pf->vsi[pf->vf[v].lan_vsi_idx]);
> + i40e_vsi_stop_rings_no_wait(pf->vsi[vf->lan_vsi_idx]);
> }
>
> /* Now that we've notified HW to disable all of the VF rings, wait
> * until they finish.
> */
> - for (v = 0; v < pf->num_alloc_vfs; v++) {
> + for (vf = &pf->vf[0]; vf < &pf->vf[pf->num_alloc_vfs]; ++vf) {
> /* On initial reset, we don't have any queues to disable */
> - if (pf->vf[v].lan_vsi_idx == 0)
> + if (vf->lan_vsi_idx == 0)
> continue;
>
> /* If VF is reset in another thread just continue */
> if (test_bit(I40E_VF_STATE_RESETTING, &vf->vf_states))
> continue;
>
> - i40e_vsi_wait_queues_disabled(pf->vsi[pf->vf[v].lan_vsi_idx]);
> + i40e_vsi_wait_queues_disabled(pf->vsi[vf->lan_vsi_idx]);
> }
>
> /* Hw may need up to 50ms to finish disabling the RX queues. We
> * minimize the wait by delaying only once for all VFs.
> */
> mdelay(50);
>
> /* Finish the reset on each VF */
> - for (v = 0; v < pf->num_alloc_vfs; v++) {
> + for (vf = &pf->vf[0]; vf < &pf->vf[pf->num_alloc_vfs]; ++vf) {
> /* If VF is reset in another thread just continue */
> if (test_bit(I40E_VF_STATE_RESETTING, &vf->vf_states))
> continue;
>
> - i40e_cleanup_reset_vf(&pf->vf[v]);
> + i40e_cleanup_reset_vf(vf);
> }
>
> i40e_flush(hw);
Powered by blists - more mailing lists