linux-kernel - RE: [PATCH v1 3/7] platform/mellanox: mlxreg-hotplug: add extra cycle for hotplug work queue

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]

Date:   Fri, 13 Apr 2018 19:39:58 +0000
From:   Vadim Pasternak <vadimp@...lanox.com>
To:     Darren Hart <dvhart@...radead.org>
CC:     "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        Michael Shych <michaelsh@...lanox.com>,
        "ivecera@...hat.com" <ivecera@...hat.com>
Subject: RE: [PATCH v1 3/7] platform/mellanox: mlxreg-hotplug: add extra cycle
 for hotplug work queue



> -----Original Message-----
> From: Darren Hart [mailto:dvhart@...radead.org]
> Sent: Friday, April 13, 2018 7:47 PM
> To: Vadim Pasternak <vadimp@...lanox.com>
> Cc: andy.shevchenko@...il.com; gregkh@...uxfoundation.org; linux-
> kernel@...r.kernel.org; platform-driver-x86@...r.kernel.org; jiri@...nulli.us;
> Michael Shych <michaelsh@...lanox.com>; ivecera@...hat.com
> Subject: Re: [PATCH v1 3/7] platform/mellanox: mlxreg-hotplug: add extra cycle
> for hotplug work queue
> 
> On Tue, Mar 27, 2018 at 10:02:03AM +0000, Vadim Pasternak wrote:
> > It adds missed logic for signal acknowledge, by adding an extra run
> > for work queue in case a signal is received, but no specific signal
> > assertion is detected. Such case theoretically can happen for example
> > in case several units are removed or inserted at the same time. In
> > this situation acknowledge for some signal can be missed at signal top
> > aggreagation status level.
> 
> Why can they be missed? What does "signal top aggregation status level"
> mean? I'm asking to confirm that we are fixing this at the right place, and not
> just applying a suboptimal bandaid by running the workqueue more.
> 

Hi Darren,

Thank for review.

It could happen within the next flow:

The signal routing flow is as following (f.e. for of FANi removing):
 - FAN status and event registers related bit is changed;
 -- intermediate aggregation status register is changed;
 --- top aggregation status register is changed;
 ---- interrupt routed to CPU and interrupt handler is invoked.

When interrupt handler is invoked it follows the next simple logic (f.e
FAN3 is removed):
 (a1) mask top aggregation interrupt mask register;
 (a2) read top aggregation interrupt status register and test to which
      underling group belongs a signal (FANs in this case and is changed
	  from 0xff to 0xfb and 0xfb is saved as a last status value);
   (b1) mask FANs interrupt mask register;
   (b2) read FANs status register and test which FAN has been changed (FAN3 in
        this example);
     (c1) perform relevant action;
              <--------------- (FAN2 is removed at this point)
   (b3) clear FANs interrupt event register to acknowledge FAN3 signal;
   (b4) unmask FANs interrupt mask register
 (a3) unmask top aggregation interrupt mask register;
 
 An interrupt handler is invoked, since FAN2 interrupt is not acknowledge.
 It should set top aggregation interrupt status register bit 6 (0xfb).
 In step (a2)
 (a2) read top aggregation interrupt and comparing it with saved value doesn't
      show change (same 0xfb) and after (a2) execution jumps to (a3) and
	  signal leaved unhandled.

> ...
> 
> >
> > Fixes: 1f976f6978bf ("platform/x86: Move Mellanox platform hotplug
> > driver to platform/mellanox")
> 
> You didn't mention above how this commit caused this - how did moving the
> driver create this problem? 

Actually I should reference to 
07b89c2b2a5e ("platform/x86: Introduce support for Mellanox hotplug driver")
which was initial driver commit, before it has been relocated. 

Does this need to go to stable? I'm assuming not as
> you've called it theoretical - not something you've observed in practice?
> 

It's not necessary to go to stable.

> ...
> 
> >  static int mlxreg_hotplug_device_create(struct
> > mlxreg_hotplug_priv_data *priv, @@ -410,6 +413,18 @@ static void
> mlxreg_hotplug_work_handler(struct work_struct *work)
> >  	aggr_asserted = priv->aggr_cache ^ regval;
> >  	priv->aggr_cache = regval;
> >
> > +	/*
> > +	 * Handler is invoked, but no assertion is detected at top aggregation
> > +	 * status level. Set aggr_asserted to mask value to allow handler extra
> > +	 * run over all relevant signals to recover any missed signal.
> > +	 */
> > +	if (priv->not_asserted == MLXREG_HOTPLUG_NOT_ASSERT) {
> > +		priv->not_asserted = 0;
> > +		aggr_asserted = pdata->mask;
> > +	}
> > +	if (!aggr_asserted)
> 
> We seem to check aggr_asserted in several places in this routine now.
> Looks like something we could simplify. If you check it here, can you drop the
> check lower in the routine? Can you remove it from the for loop if conditional
> entirely? Please consider how to simplify.

OK, will review this code.

> 
> --
> Darren Hart
> VMware Open Source Technology Center