Re: + oops-in-drivers-net-shaperc.patch added to -mm tree

Re: + oops-in-drivers-net-shaperc.patch added to -mm tree

[David Miller]

From: akpm@osdl.org
Date: Wed, 24 Jan 2007 19:54:51 -0800

> Hi,
> 
> The following code:
> [...]
> 
> Causes the following oops:
> 
 ...
> [   66.355188]  [<c0396c74>] error_code+0x7c/0x84
> [   66.355192]  [<f8adaf03>] packet_sendmsg+0x147/0x201 [af_packet]
> [   66.355199]  [<c030e1c5>] sock_sendmsg+0xf9/0x116
> [   66.355204]  [<c030eb54>] sys_sendto+0xbf/0xe0
> [   66.355208]  [<c030f494>] sys_socketcall+0x1aa/0x277
> [   66.355212]  [<c01041ea>] sysenter_past_esp+0x5f/0x99
> [   66.355216]  =======================
> [   66.355218] Code:  Bad EIP value.
> [   66.355223] EIP: [<00000000>] 0x0 SS:ESP 0068:f6261d70
> 
> shaper_header() should check for shaper->dev not being NULL (ie. the
> shaper was actually attached) as in the following patch.
> This happens in mainline too (tested 2.6.19.2).
> 
> Signed-off-by: Frederik Deweerdt <frederik.deweerdt@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Stephen Hemminger <shemminger@osdl.org>
> Signed-off-by: Andrew Morton <akpm@osdl.org>

Shaper is actually OK.  None of these hardware header callbacks
should be invoked if the device is down.  Yet, this is what is
accidently being allowed in the AF_PACKET socket layer.

Shaper makes sure to fail ->open() if shaper->dev is NULL, in order
to prevent this.

But AF_PACKET does it's check of device state too late, after the
dev->header() call.  That's the bug.

I'll fix it like this:

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 594c078..6dc01bd 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -359,6 +359,10 @@ static int packet_sendmsg_spkt(struct kiocb *iocb, struct socket *sock,
 	if (dev == NULL)
 		goto out_unlock;
 	
+	err = -ENETDOWN;
+	if (!(dev->flags & IFF_UP))
+		goto out_unlock;
+
 	/*
 	 *	You may not queue a frame bigger than the mtu. This is the lowest level
 	 *	raw protocol and you must do your own fragmentation at this level.
@@ -407,10 +411,6 @@ static int packet_sendmsg_spkt(struct kiocb *iocb, struct socket *sock,
 	if (err)
 		goto out_free;
 
-	err = -ENETDOWN;
-	if (!(dev->flags & IFF_UP))
-		goto out_free;
-
 	/*
 	 *	Now send it
 	 */
@@ -738,6 +738,10 @@ static int packet_sendmsg(struct kiocb *iocb, struct socket *sock,
 	if (sock->type == SOCK_RAW)
 		reserve = dev->hard_header_len;
 
+	err = -ENETDOWN;
+	if (!(dev->flags & IFF_UP))
+		goto out_unlock;
+
 	err = -EMSGSIZE;
 	if (len > dev->mtu+reserve)
 		goto out_unlock;
@@ -770,10 +774,6 @@ static int packet_sendmsg(struct kiocb *iocb, struct socket *sock,
 	skb->dev = dev;
 	skb->priority = sk->sk_priority;
 
-	err = -ENETDOWN;
-	if (!(dev->flags & IFF_UP))
-		goto out_free;
-
 	/*
 	 *	Now send it
 	 */

Re: + oops-in-drivers-net-shaperc.patch added to -mm tree

[Herbert Xu]

David Miller <davem@davemloft.net> wrote:
> 
> Shaper is actually OK.  None of these hardware header callbacks
> should be invoked if the device is down.  Yet, this is what is
> accidently being allowed in the AF_PACKET socket layer.

Hmm, what if the device goes down after the check?

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: + oops-in-drivers-net-shaperc.patch added to -mm tree

[Herbert Xu]

On Sat, Jan 27, 2007 at 11:45:05AM +1100, Herbert Xu wrote:
> David Miller <davem@davemloft.net> wrote:
> > 
> > Shaper is actually OK.  None of these hardware header callbacks
> > should be invoked if the device is down.  Yet, this is what is
> > accidently being allowed in the AF_PACKET socket layer.
> 
> Hmm, what if the device goes down after the check?

In fact, the shaper device doesn't even seem to take a ref count of
the device it has attached to.  So that device can go away at any time.
What's more, there are drivers that can change hard_header at run-time
(s390).

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: + oops-in-drivers-net-shaperc.patch added to -mm tree

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 27 Jan 2007 12:01:46 +1100

> In fact, the shaper device doesn't even seem to take a ref count of
> the device it has attached to.  So that device can go away at any time.
> What's more, there are drivers that can change hard_header at run-time
> (s390).

Indeed, it needs to do refcounting or something here, at a minimum,
this driver is so buggy and full of problems :-/

Thankfully it's marked OBSOLETE so we'll get rid of it eventually :)

Re: + oops-in-drivers-net-shaperc.patch added to -mm tree

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Sat, 27 Jan 2007 11:45:05 +1100

> David Miller <davem@davemloft.net> wrote:
> > 
> > Shaper is actually OK.  None of these hardware header callbacks
> > should be invoked if the device is down.  Yet, this is what is
> > accidently being allowed in the AF_PACKET socket layer.
> 
> Hmm, what if the device goes down after the check?

For the shaper case it's OK, because once a shaper is
associated it's ->dev never goes back to NULL.

In general, yes it's a problem :-/