netdev - Re: [PATCH net-next] net: sfp: Stop SFP polling and interrupt handling during shutdown

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: <a992a085-7a3a-fa6e-1261-878c5ec6c1c0@sedsystems.ca>
Date:   Fri, 7 Jun 2019 10:08:02 -0600
From:   Robert Hancock <hancock@...systems.ca>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, andrew@...n.ch, f.fainelli@...il.com,
        hkallweit1@...il.com
Subject: Re: [PATCH net-next] net: sfp: Stop SFP polling and interrupt
 handling during shutdown

On 2019-06-07 4:42 a.m., Russell King - ARM Linux admin wrote:
> On Thu, Jun 06, 2019 at 02:57:22PM -0600, Robert Hancock wrote:
>> The idea there was to deal with the case where GPIO interrupts were
>> previously raised before shutdown and not yet handled by the threaded
>> interrupt handler by the time shutdown is called. After shutdown on the
>> SFP completes, the bus the GPIO stuff is on could potentially be shut
>> down at any moment, so we really don't want to be digging into the GPIO
>> states after that. Locking the mutex there ensures that we don't read a
>> stale value for the shutdown flag in the interrupt handler, since AFAIK
>> there's no other synchronization around that value.
> 
> There are two cases:
> 
> 1) The interrupt is raised just as sfp_shutdown() is called but before
>    the mutex is taken.  We will get the full processing in this case.
> 
> 2) The interrupt is raised during the mutex-locked bit of sfp_shutdown()
>    or after the mutex in sfp_shutdown() is released.  We will get the
>    abbreviated processing.
> 
> This means that the mutex doesn't provide any protection against full
> interrupt processing if it occurs just prior to or during the initial
> execution of sfp_shutdown().
> 
> All that we need to ensure is that the state of sfp->shutdown is
> visible by the time sfp_shutdown() returns, and that none of the
> interrupt and worker functions are executing.  We have the worker
> functions covered by the synchronous cancelling of them, but not the
> interrupts, and as Florian points out, it's probably better to disable
> the interrupts, and again, that can be done synchronously to ensure
> that the handlers are not running.
> 
> If the workers and interrupt handlers are synchronously disabled, we
> can be sure by the end of sfp_shutdown() that none of those paths are
> executing, so the next time something attempts to trigger them, they
> will see sfp->shutdown is set.
> 
> I'm not convinced that we even need sfp->shutdown if we have cancelled
> the workers and disabled the interrupts.

I think you're right that if we just free the interrupts we avoid the
need for that flag, as that should ensure that the interrupt handlers
are no longer running or pending. Will resubmit with that approach and
also add a patch to add another mutex to the state checking as you
suggested.

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@...systems.ca