[NET]: Fix dev->qdisc race for NETDEV_TX_LOCKED case

[NET]: Fix dev->qdisc race for NETDEV_TX_LOCKED case

[Thomas Graf]

When transmit fails with NETDEV_TX_LOCKED the skb is requeued
to dev->qdisc again. The dev->qdisc pointer is protected by
the queue lock which needs to be dropped when attempting to
transmit and acquired again before requeing. The problem is
that qdisc_restart() fetches the dev->qdisc pointer once and
stores it in the `q' variable which is invalidated when
dropping the queue_lock, therefore the variable needs to be
refreshed before requeueing.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

Index: net-2.6/net/sched/sch_generic.c
===================================================================
--- net-2.6.orig/net/sched/sch_generic.c	2007-05-10 11:32:18.000000000 +0200
+++ net-2.6/net/sched/sch_generic.c	2007-05-10 11:34:37.000000000 +0200
@@ -139,6 +139,7 @@ static inline int qdisc_restart(struct n
 				}
 				if (ret == NETDEV_TX_LOCKED && nolock) {
 					spin_lock(&dev->queue_lock);
+					q = dev->qdisc;
 					goto collision;
 				}
 			}

Re: [NET]: Fix dev->qdisc race for NETDEV_TX_LOCKED case

[Herbert Xu]

Thomas Graf <tgraf@suug.ch> wrote:
> When transmit fails with NETDEV_TX_LOCKED the skb is requeued
> to dev->qdisc again. The dev->qdisc pointer is protected by
> the queue lock which needs to be dropped when attempting to
> transmit and acquired again before requeing. The problem is
> that qdisc_restart() fetches the dev->qdisc pointer once and
> stores it in the `q' variable which is invalidated when
> dropping the queue_lock, therefore the variable needs to be
> refreshed before requeueing.
> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>

I agree, good catch Thomas.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NET]: Fix dev->qdisc race for NETDEV_TX_LOCKED case

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 10 May 2007 21:55:17 +1000

> Thomas Graf <tgraf@suug.ch> wrote:
> > When transmit fails with NETDEV_TX_LOCKED the skb is requeued
> > to dev->qdisc again. The dev->qdisc pointer is protected by
> > the queue lock which needs to be dropped when attempting to
> > transmit and acquired again before requeing. The problem is
> > that qdisc_restart() fetches the dev->qdisc pointer once and
> > stores it in the `q' variable which is invalidated when
> > dropping the queue_lock, therefore the variable needs to be
> > refreshed before requeueing.
> > 
> > Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 
> I agree, good catch Thomas.

Applied to my tree and I'll push it off to Linus tomorrow unless
someone finds a problem with it by then.

Thanks.

Re: [NET]: Fix dev->qdisc race for NETDEV_TX_LOCKED case

[Herbert Xu]

On Thu, May 10, 2007 at 04:56:18AM -0700, David Miller wrote:
> 
> Applied to my tree and I'll push it off to Linus tomorrow unless
> someone finds a problem with it by then.

BTW, guess how this bug was introduced? By the lockless patch,
surprise surprise :)

The sooner we are rid of it the better.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt