netdev - Re: [PATCH net-next 0/6] net/ncsi: Allow enabling multiple packages & channels

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: <7f3744217190fe94bb1df879fa107593e911d7b5.camel@mendozajonas.com>
Date:   Fri, 19 Oct 2018 10:05:09 +1100
From:   Samuel Mendoza-Jonas <sam@...dozajonas.com>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, Justin.Lee1@...l.com,
        linux-kernel@...r.kernel.org, openbmc@...ts.ozlabs.org
Subject: Re: [PATCH net-next 0/6] net/ncsi: Allow enabling multiple packages
 & channels

On Thu, 2018-10-18 at 15:56 -0700, David Miller wrote:
> From: Samuel Mendoza-Jonas <sam@...dozajonas.com>
> Date: Thu, 18 Oct 2018 14:59:11 +1100
> 
> > This series extends the NCSI driver to configure multiple packages
> > and/or channels simultaneously. Since the RFC series this includes a few
> > extra changes to fix areas in the driver that either made this harder or
> > were roadblocks due to deviations from the NCSI specification.
> > 
> > Patches 1 & 2 fix two issues where the driver made assumptions about the
> > capabilities of the NCSI topology.
> > Patches 3 & 4 change some internal semantics slightly to make multi-mode
> > easier.
> > Patch 5 introduces a cleaner way of reconfiguring the NCSI configuration
> > and keeping track of channel states.
> > Patch 6 implements the main multi-package/multi-channel configuration,
> > configured via the Netlink interface.
> > 
> > Readers who have an interesting NCSI setup - especially multi-package
> > with HWA - please test! I think I've covered all permutations but I
> > don't have infinite hardware to test on.
> 
> This doesn't apply cleanly to net-next.  Does it depend upon changes
> applied elsewhere?  You must always make that explicit.

Ah, my mistake; I hadn't updated my net-next branch recently enough and
missed Vijay's OEM command patch. Will rebase.

> 
> Also, please explain this locking in ncsi_reset_dev():
> 
> +       NCSI_FOR_EACH_PACKAGE(ndp, np) {
> +               NCSI_FOR_EACH_CHANNEL(np, nc) {
> +                       spin_lock_irqsave(&nc->lock, flags);
> +                       enabled = nc->monitor.enabled;
> +                       state = nc->state;
> +                       spin_unlock_irqrestore(&nc->lock, flags);
> +
> +                       if (enabled)
> +                               ncsi_stop_channel_monitor(nc);
> +                       if (state == NCSI_CHANNEL_ACTIVE) {
> +                               active = nc;
> +                               break;
> +                       }
> 
> Is that really protecting anything?
> 
> Right after you drop np->lock those two values can change, the state
> of the 'nc' can change such that it isn't NCSI_CHANNEL_ACTIVE anymore
> etc.
> 
> At best this locking makes sure thatn enabled and state are consistent
> with respect to eachother, only.  It doesn't guarantee anything about
> the stability of the state of the object at all, and it can change
> right from under you.

And you've caught this correctly, I'll fix this up in the rebase to
protect actually checking the channel/monitor state.

Thanks,
Sam