netdev - Re: [PATCH v3 2/2] net/ncsi: fix state race during channel probe completion

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: <CAGfYmwVH1A+imBF5TLenLaSM0Zf0C5wgfgocYix9Ye_7siR_xQ@mail.gmail.com>
Date: Thu, 16 Jan 2025 08:38:15 +0800
From: Potin Lai <potin.lai.pt@...il.com>
To: Paul Fertser <fercerpav@...il.com>
Cc: Samuel Mendoza-Jonas <sam@...dozajonas.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Simon Horman <horms@...nel.org>, Ivan Mikhaylov <fr0st61te@...il.com>, 
	Patrick Williams <patrick@...cx.xyz>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Cosmo Chou <cosmo.chou@...ntatw.com>, Potin Lai <potin.lai@...ntatw.com>, 
	Cosmo Chou <chou.cosmo@...il.com>
Subject: Re: [PATCH v3 2/2] net/ncsi: fix state race during channel probe completion

On Tue, Jan 14, 2025 at 8:55 PM Paul Fertser <fercerpav@...il.com> wrote:
>
> Hello,
>
> On Mon, Jan 13, 2025 at 10:34:48AM +0800, Potin Lai wrote:
> > During channel probing, the last NCSI_PKT_CMD_DP command can trigger
> > an unnecessary schedule_work() via ncsi_free_request(). We observed
> > that subsequent config states were triggered before the scheduled
> > work completed, causing potential state handling issues.
>
> Please let's not make this whole NC-SI story even less comprehensible
> than it already is. From this commit message I was unable to
> understand what exactly is racing with what and under which
> conditions. "Can trigger" would imply that it does not always trigger
> that wrong state transition but that would also mean there's a set of
> conditions that is necessary for bug to happen.
>
> > Fix this by clearing req_flags when processing the last package.
>
> After reading the code for a few hours I can probably see how lack of
> proper processing of the response to the last "Package Deselect" call
> can mix up the states.
>
> How about this diff instead (tested on Tioga Pass but there we didn't
> have any issues in the first place)?
>
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index bf276eaf9330..7891a537bddd 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -1385,6 +1385,12 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
>                 nd->state = ncsi_dev_state_probe_package;
>                 break;
>         case ncsi_dev_state_probe_package:
> +               if (ndp->package_probe_id >= 8) {
> +                       /* Last package probed, finishing */
> +                       ndp->flags |= NCSI_DEV_PROBED;
> +                       break;
> +               }
> +
>                 ndp->pending_req_num = 1;
>
>                 nca.type = NCSI_PKT_CMD_SP;
> @@ -1501,13 +1507,8 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
>                 if (ret)
>                         goto error;
>
> -               /* Probe next package */
> +               /* Probe next package after receiving response */
>                 ndp->package_probe_id++;
> -               if (ndp->package_probe_id >= 8) {
> -                       /* Probe finished */
> -                       ndp->flags |= NCSI_DEV_PROBED;
> -                       break;
> -               }
>                 nd->state = ncsi_dev_state_probe_package;
>                 ndp->active_package = NULL;
>                 break;
>
> --
> Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
> mailto:fercerpav@...il.com

Hi Paul,

Thank you for your suggestion! I tested the patch, and it works perfectly.
Could you please help to send it out? I appreciate your assistance.

Best regards,
Potin