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: <20190607102603.4fp4v5pidkhjcygt@shell.armlinux.org.uk>
Date:   Fri, 7 Jun 2019 11:26:03 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Robert Hancock <hancock@...systems.ca>
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 Thu, Jun 06, 2019 at 02:57:22PM -0600, Robert Hancock wrote:
> 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).

That's a good point, one that we should address separately.  Rather
than re-using the existing mutex (which would be difficult to hold
across calling the state machine due to locking inversion) how about
this:

 drivers/net/phy/sfp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 8a21294d1ce8..5ff427dcbb31 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -190,6 +190,7 @@ struct sfp {
 	struct delayed_work poll;
 	struct delayed_work timeout;
 	struct mutex sm_mutex;
+	struct mutex st_mutex;
 	unsigned char sm_mod_state;
 	unsigned char sm_dev_state;
 	unsigned short sm_state;
@@ -1976,6 +1977,7 @@ static void sfp_check_state(struct sfp *sfp)
 {
 	unsigned int state, i, changed;
 
+	mutex_lock(&sfp->st_mutex);
 	state = sfp_get_state(sfp);
 	changed = state ^ sfp->state;
 	changed &= SFP_F_PRESENT | SFP_F_LOS | SFP_F_TX_FAULT;
@@ -2001,6 +2003,7 @@ static void sfp_check_state(struct sfp *sfp)
 		sfp_sm_event(sfp, state & SFP_F_LOS ?
 				SFP_E_LOS_HIGH : SFP_E_LOS_LOW);
 	rtnl_unlock();
+	mutex_unlock(&sfp->st_mutex);
 }
 
 static irqreturn_t sfp_irq(int irq, void *data)
@@ -2031,6 +2034,7 @@ static struct sfp *sfp_alloc(struct device *dev)
 	sfp->dev = dev;
 
 	mutex_init(&sfp->sm_mutex);
+	mutex_init(&sfp->st_mutex);
 	INIT_DELAYED_WORK(&sfp->poll, sfp_poll);
 	INIT_DELAYED_WORK(&sfp->timeout, sfp_timeout);
 
-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up