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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1a329ee9-4292-44a2-90eb-a82ca3de03f3@sedsystems.ca>
Date:   Thu, 6 Jun 2019 14:57:22 -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-06 12:09 p.m., Russell King - ARM Linux admin wrote:
>> @@ -1466,6 +1467,11 @@ static void sfp_sm_mod_remove(struct sfp *sfp)
>>  static void sfp_sm_event(struct sfp *sfp, unsigned int event)
>>  {
>>  	mutex_lock(&sfp->sm_mutex);
>> +	if (unlikely(sfp->shutdown)) {
>> +		/* Do not handle any more state machine events. */
>> +		mutex_unlock(&sfp->sm_mutex);
>> +		return;
>> +	}
>>  
>>  	dev_dbg(sfp->dev, "SM: enter %s:%s:%s event %s\n",
>>  		mod_state_to_str(sfp->sm_mod_state),
>> @@ -1704,6 +1710,13 @@ static void sfp_check_state(struct sfp *sfp)
>>  {
>>  	unsigned int state, i, changed;
>>  
>> +	mutex_lock(&sfp->sm_mutex);
>> +	if (unlikely(sfp->shutdown)) {
>> +		/* No more state checks */
>> +		mutex_unlock(&sfp->sm_mutex);
>> +		return;
>> +	}
>> +
> 
> I don't think you need to add the mutex locking - just check for
> sfp->shutdown and be done with it...

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.

It may also be helpful that the lock is now held for the subsequent code
in sfp_check_state that's comparing the previous and new states - it
seems like you could otherwise run into trouble if that function was
being concurrently called from the polling thread and the interrupt
handler (for example if you had an SFP where some GPIOs supported
interrupts and some didn't).

> 
>> +static void sfp_shutdown(struct platform_device *pdev)
>> +{
>> +	struct sfp *sfp = platform_get_drvdata(pdev);
>> +
>> +	mutex_lock(&sfp->sm_mutex);
>> +	sfp->shutdown = true;
>> +	mutex_unlock(&sfp->sm_mutex);
>> +
>> +	cancel_delayed_work_sync(&sfp->poll);
>> +	cancel_delayed_work_sync(&sfp->timeout);
> 
> Since the work cancellation will ensure that the works are not running
> at the point they return, and should they then run again, they'll hit
> the sfp->shutdown condition.
> 
>> +}
>> +
>>  static struct platform_driver sfp_driver = {
>>  	.probe = sfp_probe,
>>  	.remove = sfp_remove,
>> +	.shutdown = sfp_shutdown,
>>  	.driver = {
>>  		.name = "sfp",
>>  		.of_match_table = sfp_of_match,
>> -- 
>> 1.8.3.1
>>
>>
> 

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