[PATCH git latest] drivers/net: fixing a datarace related to update_stats()

[PATCH git latest] drivers/net: fixing a datarace related to update_stats()

[Lin Tan]

Fixing a datarace.

As indicated by the following comment, a lock must be held before calling 
function update_stats(). This rule is followed in some cases, but not in 
others. For example, the lock is held when the function is called in function 
el3_get_stats(), but the lock is NOT held when called in el3_close(). It can 
cause potential data races. 

/*
  ...
  
  Caller must hold the lock for this
*/
static void update_stats(struct net_device *dev)
{
...
}


Signed-off-by: Lin Tan <tammy000@gmail.com>

---

--- a/drivers/net/pcmcia/3c589_cs.c	2008-09-25 11:52:42.000000000 -0500
+++ b/drivers/net/pcmcia/3c589_cs.c	2008-09-25 13:01:44.000000000 -0500
@@ -920,6 +920,7 @@ static int el3_close(struct net_device *
     struct el3_private *lp = netdev_priv(dev);
     struct pcmcia_device *link = lp->p_dev;
     unsigned int ioaddr = dev->base_addr;
+    unsigned long flags;
     
     DEBUG(1, "%s: shutting down ethercard.\n", dev->name);
 
@@ -947,7 +948,9 @@ static int el3_close(struct net_device *
         
 	/* Check if the card still exists */
 	if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000)
+    	    spin_lock_irqsave(&lp->lock, flags);
 	    update_stats(dev);
+    	    spin_unlock_irqrestore(&lp->lock, flags);
     }
 
     link->open--;

Re: [PATCH git latest] drivers/net: fixing a datarace related to update_stats()

[Francois Romieu]

(Please Cc: netdev@vger.kernel.org)

Lin Tan <tammy000@gmail.com> :
[drivers/net/pcmcia/3c589_cs.c race]
> As indicated by the following comment, a lock must be held before calling 
> function update_stats(). This rule is followed in some cases, but not in 
> others. For example, the lock is held when the function is called in function 
> el3_get_stats(), but the lock is NOT held when called in el3_close(). It can 
> cause potential data races. 

I would not bet that the irq handler does not race with el3_close()
in the first place. Could you dig it ?

-- 
Ueimor

[PATCH] pcmcia: Fix broken abuse of dev->driver_data

[Alan Cox]

From: Alan Cox <alan@redhat.com>

PCMCIA abuses dev->private_data in the probe methods. Unfortunately it
continues to abuse it after calling drv->probe() which leads to crashes and
other nasties (such as bogus probes of multifunction devices) giving errors like

pcmcia: registering new device pcmcia0.1
kernel: 0.1: GetNextTuple: No more items

Extract the passed data before calling the driver probe function that way
we don't blow up when the driver reuses dev->private_data as its right.

As its close to the final release just move the hack so it works out,
hopefully someone will be sufficiently embarrassed to produce a nice rework
for 2.6.28.

Signed-off-by: Alan Cox <alan@redhat.com>
---

 drivers/pcmcia/ds.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)


diff --git a/drivers/pcmcia/ds.c b/drivers/pcmcia/ds.c
index 4174d96..853d342 100644
--- a/drivers/pcmcia/ds.c
+++ b/drivers/pcmcia/ds.c
@@ -426,6 +426,18 @@ static int pcmcia_device_probe(struct device * dev)
 	p_dev = to_pcmcia_dev(dev);
 	p_drv = to_pcmcia_drv(dev->driver);
 	s = p_dev->socket;
+	
+	/* The PCMCIA code passes the match data in via dev->driver_data
+	 * which is an ugly hack. Once the driver probe is called it may
+	 * and often will overwrite the match data so we must save it first 
+	 *
+	 * handle pseudo multifunction devices:
+	 * there are at most two pseudo multifunction devices.
+	 * if we're matching against the first, schedule a
+	 * call which will then check whether there are two
+	 * pseudo devices, and if not, add the second one.
+	 */
+	did = p_dev->dev.driver_data;
 
 	ds_dbg(1, "trying to bind %s to %s\n", p_dev->dev.bus_id,
 	       p_drv->drv.name);
@@ -455,21 +467,14 @@ static int pcmcia_device_probe(struct device * dev)
 		goto put_module;
 	}
 
-	/* handle pseudo multifunction devices:
-	 * there are at most two pseudo multifunction devices.
-	 * if we're matching against the first, schedule a
-	 * call which will then check whether there are two
-	 * pseudo devices, and if not, add the second one.
-	 */
-	did = p_dev->dev.driver_data;
 	if (did && (did->match_flags & PCMCIA_DEV_ID_MATCH_DEVICE_NO) &&
 	    (p_dev->socket->device_count == 1) && (p_dev->device_no ==
0)) pcmcia_add_device_later(p_dev->socket, 0);
 
- put_module:
+put_module:
 	if (ret)
 		module_put(p_drv->owner);
- put_dev:
+put_dev:
 	if (ret)
 		put_device(dev);
 	return (ret);
--
<meme>discombobulated echidna</meme>

Re: [PATCH] pcmcia: Fix broken abuse of dev->driver_data

[Dominik Brodowski]

Alan,

(CC: linux-pcmcia added)

Many thanks for finding this bug.

On Mon, Sep 22, 2008 at 03:58:14PM +0100, Alan Cox wrote:
> PCMCIA abuses dev->private_data in the probe methods. Unfortunately it
> continues to abuse it after calling drv->probe() which leads to crashes and
> other nasties (such as bogus probes of multifunction devices) giving errors like

Well, PCMCIA drivers have (struct pcmcia_device *) p_dev->priv available and
use this regularly, so using dev->private_data never has been supported.
Nonetheless, let's add support for using dev->private_data also for PCMCIA
devices and remove p_dev->priv eventually.

> As its close to the final release just move the hack so it works out,
> hopefully someone will be sufficiently embarrassed to produce a nice rework
> for 2.6.28.

Could you remove this from the changelog entry, please?


> @@ -426,6 +426,18 @@ static int pcmcia_device_probe(struct device * dev)
>  	p_dev = to_pcmcia_dev(dev);
>  	p_drv = to_pcmcia_drv(dev->driver);
>  	s = p_dev->socket;
> +	

trailing whitespace

> +	/* The PCMCIA code passes the match data in via dev->driver_data
> +	 * which is an ugly hack. Once the driver probe is called it may
> +	 * and often will overwrite the match data so we must save it first 

trailing whitespace

>  	if (did && (did->match_flags & PCMCIA_DEV_ID_MATCH_DEVICE_NO) &&
>  	    (p_dev->socket->device_count == 1) && (p_dev->device_no ==
> 0)) pcmcia_add_device_later(p_dev->socket, 0);

uh?

>  
> - put_module:
> +put_module:
>  	if (ret)
>  		module_put(p_drv->owner);
> - put_dev:
> +put_dev:

unrelated -- please do not change it this time.

Best and thanks again,

	Dominik

Re: [PATCH] pcmcia: Fix broken abuse of dev->driver_data

[Alan Cox]

> > +	
> 
> trailing whitespace

Nod..

> >  	if (did && (did->match_flags & PCMCIA_DEV_ID_MATCH_DEVICE_NO) &&
> >  	    (p_dev->socket->device_count == 1) && (p_dev->device_no ==
> > 0)) pcmcia_add_device_later(p_dev->socket, 0);
> 
> uh?

Looks like a mailer folded it later, but the patch itself seems just fine.

> >  
> > - put_module:
> > +put_module:
> >  	if (ret)
> >  		module_put(p_drv->owner);
> > - put_dev:
> > +put_dev:
> 
> unrelated -- please do not change it this time.

Not sure you can have it both ways - if you don't want stray whitespace
then fixing the labels to conform to coding style seems to go with it ;)

Alan

Re: [PATCH] pcmcia: Fix broken abuse of dev->driver_data

[Dominik Brodowski]

On Mon, Sep 22, 2008 at 11:12:27PM +0100, Alan Cox wrote:
> Nod..
...
> Looks like a mailer folded it later, but the patch itself seems just fine.

*gnah* already applied upstream, including the comment implying that PCMCIA
driver's making use of driver_data were correct, while they were not. At
least the trailing whitespace didn't get merged.

> > >  
> > > - put_module:
> > > +put_module:
> > >  	if (ret)
> > >  		module_put(p_drv->owner);
> > > - put_dev:
> > > +put_dev:
> > 
> > unrelated -- please do not change it this time.
> 
> Not sure you can have it both ways - if you don't want stray whitespace
> then fixing the labels to conform to coding style seems to go with it ;)

Well, there's a difference, and I know you know that I know that you know ;)
but you should also know that I already tried to apply the patch as it is,
which implies that I do not object too strongly to the removal of these two
non-characters from ds.c ;)

Thanks,
	Dominik

Re: [PATCH] pcmcia: Fix broken abuse of dev->driver_data

[Alan Cox]

> *gnah* already applied upstream, including the comment implying that PCMCIA
> driver's making use of driver_data were correct, while they were not. At
> least the trailing whitespace didn't get merged.

It was correct - dev->private_data for struct device is driver stuff, and
as the same drivers are used for pcmcia and non pcmcia its a bit hard to
argue otherwise.

Anyway tis finally sorted (and boy was it fun to pin down)

Alan

Re: [PATCH git latest] drivers/net: fixing a datarace related to update_stats()

[Komuro]

Hi,

You need two braces...

- 	if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000)
+ 	if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000) {
+    	    spin_lock_irqsave(&lp->lock, flags);
 	    update_stats(dev);
+    	    spin_unlock_irqrestore(&lp->lock, flags);
+	}
     }

Re: [PATCH git latest] drivers/net: fixing a datarace related to update_stats()

[Tammy]

> You need two braces...
>
> -       if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000)
> +       if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000) {
> +           spin_lock_irqsave(&lp->lock, flags);
>            update_stats(dev);
> +           spin_unlock_irqrestore(&lp->lock, flags);
> +       }
>     }
>


Thanks a lot. I will resubmit the patch then.

Lin

Re: [PATCH git latest] drivers/net: fixing a datarace related to update_stats()

[Lin Tan]

[-- Attachment #1: Type: text/plain, Size: 31 bytes --]

Resending the corrected patch.

[-- Attachment #2: update_stats.readytosend --]
[-- Type: text/plain, Size: 1275 bytes --]

Fixing a datarace.

As indicated by the following comment, a lock must be held before calling 
function update_stats(). This rule is followed in some cases, but not in 
others. For example, the lock is held when the function is called in function 
el3_get_stats(), but the lock is NOT held when called in el3_close(). It can 
cause potential data races. 

/*
  ...
  
  Caller must hold the lock for this
*/
static void update_stats(struct net_device *dev)
{
...
}


Signed-off-by: Lin Tan <tammy000@gmail.com>

---

--- a/drivers/net/pcmcia/3c589_cs.c	2008-09-25 11:52:42.000000000 -0500
+++ b/drivers/net/pcmcia/3c589_cs.c	2008-09-26 11:07:04.000000000 -0500
@@ -920,6 +920,7 @@
     struct el3_private *lp = netdev_priv(dev);
     struct pcmcia_device *link = lp->p_dev;
     unsigned int ioaddr = dev->base_addr;
+    unsigned long flags;
     
     DEBUG(1, "%s: shutting down ethercard.\n", dev->name);
 
@@ -946,8 +947,11 @@
 	outw(0x0f00, ioaddr + WN0_IRQ);
         
 	/* Check if the card still exists */
-	if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000)
+	if ((inw(ioaddr+EL3_STATUS) & 0xe000) == 0x2000) {
+    	    spin_lock_irqsave(&lp->lock, flags);
 	    update_stats(dev);
+    	    spin_unlock_irqrestore(&lp->lock, flags);
+	}
     }
 
     link->open--;