oops with recent wireless-dev tree

oops with recent wireless-dev tree

[Jochen Voss]

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

Hi,

when I use the b43 driver with my PCMCIA LinkSys WRT54GL adapter
(Broadcom 4318), run hostapd on the interface, and then try to add the
interface to a bridge, I get the following oops:

    BUG: unable to handle kernel NULL pointer dereference at virtual address 00000000
     printing eip:
    *pde = 00000000
    Oops: 0000 [#1]
    CPU:    0
    EIP:    0060:[<c02a2cb2>]    Not tainted VLI
    EFLAGS: 00010292   (2.6.23-rc3-wd #1)
    EIP is at port_cost+0x11/0xaa
    eax: c3c40000   ebx: c3c40000   ecx: 00000000   edx: 000000c0
    esi: 00000000   edi: c2a3ea80   ebp: c2a31e00   esp: c2a31dc8
    ds: 007b   es: 007b   fs: 0000  gs: 0033  ss: 0068
    Process brctl (pid: 1498, ti=c2a30000 task=c25734c0 task.ti=c2a30000)
    Stack: c0119bcf c3680140 c10beea0 c2a31ddc c0148dc4 0000000d c2a31e00 00000296 
	   f000007e c10beea0 000080d0 c2bb7700 c3c40000 c2a3ea80 c2a31e2c c02a2ee3 
	   c251b600 c3680270 00000001 c2bb7700 c3073380 c01117df c3c40000 00000001 
    Call Trace:
     [<c010294f>] show_trace_log_lvl+0x1a/0x2f
     [<c0102a01>] show_stack_log_lvl+0x9d/0xa5
     [<c0102d76>] show_registers+0x1a5/0x277
     [<c0102f24>] die+0xdc/0x1a5
     [<c01082d1>] do_page_fault+0x461/0x530
     [<c02db49a>] error_code+0x6a/0x70
     [<c02a2ee3>] br_add_if+0x119/0x290
     [<c02a35b0>] add_del_if+0x40/0x56
     [<c02a3acc>] br_dev_ioctl+0x506/0x537
     [<c0219bc8>] dev_ifsioc+0x3c1/0x3dd
     [<c0219f2b>] dev_ioctl+0x347/0x3d4
     [<c02110ec>] sock_ioctl+0x183/0x18f
     [<c01435d4>] do_ioctl+0x1c/0x4b
     [<c01437d3>] vfs_ioctl+0x1d0/0x1df
     [<c0143815>] sys_ioctl+0x33/0x4a
     [<c0102422>] syscall_call+0x7/0xb
     =======================
    Code: ff 31 c0 eb 05 b8 ea ff ff ff 5b 5d c3 55 89 e5 83 e8 7c e8 35 58 e9 ff 5d c3 55 89 e5 57 56 53 83 ec 2c 89 c3 8b b0 b4 00 00 00 <83> 3e 00 74 53 
    EIP: [<c02a2cb2>] port_cost+0x11/0xaa SS:ESP 0068:c2a31dc8

If I read this correctly, the EIP in the last line corresponds to
net/bridge/br_if.c, line 36:

    static int port_cost(struct net_device *dev)
    {
	    if (dev->ethtool_ops->get_settings) {
                               ^^^^

As far as I can figure out, dev->ethtool_ops is NULL and the crash
happens while trying to derefernce ...->get_settings.

Is dev->ethtool_ops allowed to be NULL?  In this case the appended
patch might be the correct fix.  At least it makes the oops disappear
for me.  Another possible fix would be to add an ethtool_ops structure
to the device created by b43.

I hope this helps,
Jochen
-- 
http://seehuhn.de/
----------------------------------------------------------------------
Avoid crashes while determining initial path cost for a bridge.

Check whether 'dev->ethtool_ops' is non-NULL before accessing
'dev->ethtool_ops->get_settings'.

Signed-off-by: Jochen Voss <voss@seehuhn.de>

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index b40dada..5b396ea 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -33,7 +33,7 @@
  */
 static int port_cost(struct net_device *dev)
 {
-       if (dev->ethtool_ops->get_settings) {
+       if (dev->ethtool_ops && dev->ethtool_ops->get_settings) {
                struct ethtool_cmd ecmd = { ETHTOOL_GSET };
                int err = dev->ethtool_ops->get_settings(dev, &ecmd);
                if (!err) {

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: oops with recent wireless-dev tree

[Johannes Berg]

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

Hi Jochen,

[added CCs since it affects bridge code]

> If I read this correctly, the EIP in the last line corresponds to
> net/bridge/br_if.c, line 36:
> 
>     static int port_cost(struct net_device *dev)
>     {
> 	    if (dev->ethtool_ops->get_settings) {
>                                ^^^^
> 
> As far as I can figure out, dev->ethtool_ops is NULL and the crash
> happens while trying to derefernce ...->get_settings.
> 
> Is dev->ethtool_ops allowed to be NULL?  In this case the appended
> patch might be the correct fix.  At least it makes the oops disappear
> for me.  Another possible fix would be to add an ethtool_ops structure
> to the device created by b43.

I don't think adding ethtool_ops in mac80211 should be necessary.
Stephen?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: oops with recent wireless-dev tree

[Matthew Wilcox]

On Thu, Aug 30, 2007 at 07:49:49AM -0700, Stephen Hemminger wrote:
> > >     static int port_cost(struct net_device *dev)
> > >     {
> > > 	    if (dev->ethtool_ops->get_settings) {
> > >                                ^^^^
> > > 
> > > As far as I can figure out, dev->ethtool_ops is NULL and the crash
> > > happens while trying to derefernce ...->get_settings.
> 
> Devices aren't required to have ethtool_ops. The code there used to
> call ethtool directly, and it would handle the error cases. I'll rollup
> a fix this morning.

Yep, clearly my fault; that should read:

	if (dev->ethtool_ops && dev->ethtool_ops->get_settings) {

Since Stephen's already bagged fixing this, I shan't send a patch.
But if it includes something like the line above please add:

Acked-by: Matthew Wilcox <matthew@wil.cx>

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: oops with recent wireless-dev tree

[Johannes Berg]

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

On Thu, 2007-08-30 at 07:49 -0700, Stephen Hemminger wrote:

> Devices aren't required to have ethtool_ops. The code there used to
> call ethtool directly, and it would handle the error cases. I'll rollup
> a fix this morning.

Great, thanks.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: oops with recent wireless-dev tree

[Johannes Berg]

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

On Thu, 2007-08-30 at 07:49 -0700, Stephen Hemminger wrote:

> Devices aren't required to have ethtool_ops. The code there used to
> call ethtool directly, and it would handle the error cases. I'll rollup
> a fix this morning.

Great, thanks.

Jochen had a patch: http://marc.info/?l=linux-wireless&m=118842715026614&w=2

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[PATCH] bridge: fix OOPS when bridging device without ethtool

[Stephen Hemminger]

Bridge code calls ethtool to get speed. The conversion to using
only ethtool_ops broke the case of devices without ethtool_ops.
This is a new regression in 2.6.23.

Rearranged the switch to a logical order, and use gcc initializer.

Ps: speed should have been part of the network device structure from
    the start rather than burying it in ethtool.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>


--- a/net/bridge/br_if.c	2007-08-30 07:49:01.000000000 -0700
+++ b/net/bridge/br_if.c	2007-08-30 07:48:16.000000000 -0700
@@ -33,17 +33,17 @@
  */
 static int port_cost(struct net_device *dev)
 {
-	if (dev->ethtool_ops->get_settings) {
-		struct ethtool_cmd ecmd = { ETHTOOL_GSET };
-		int err = dev->ethtool_ops->get_settings(dev, &ecmd);
-		if (!err) {
+	if (dev->ethtool_ops && dev->ethtool_ops->get_settings) {
+		struct ethtool_cmd ecmd = { .cmd = ETHTOOL_GSET, };
+
+		if (!dev->ethtool_ops->get_settings(dev, &ecmd)) {
 			switch(ecmd.speed) {
-			case SPEED_100:
-				return 19;
-			case SPEED_1000:
-				return 4;
 			case SPEED_10000:
 				return 2;
+			case SPEED_1000:
+				return 4;
+			case SPEED_100:
+				return 19;
 			case SPEED_10:
 				return 100;
 			}

Re: oops with recent wireless-dev tree

[Matthew Wilcox]

On Thu, Aug 30, 2007 at 05:01:31PM +0200, Johannes Berg wrote:
> Jochen had a patch: http://marc.info/?l=linux-wireless&m=118842715026614&w=2

That's exactly the right patch, please add

Acked-by: Matthew Wilcox <matthew@wil.cx>

I just checked over the commit that introduced the bug, and I didn't
make the same mistake anywhere else.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] bridge: fix OOPS when bridging device without ethtool

[Matthew Wilcox]

On Thu, Aug 30, 2007 at 08:29:32AM -0700, Stephen Hemminger wrote:
> Bridge code calls ethtool to get speed. The conversion to using
> only ethtool_ops broke the case of devices without ethtool_ops.
> This is a new regression in 2.6.23.
> 
> Rearranged the switch to a logical order, and use gcc initializer.
> 
> Ps: speed should have been part of the network device structure from
>     the start rather than burying it in ethtool.

Feel free to do the conversion ;-)  One of the things I like about the
ethtool framework is it gives us a way to take stuff out of the drivers
and put it in the midlayer without disturbing userspace.

> Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>

Acked-by: Matthew Wilcox <matthew@wil.cx>

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] bridge: fix OOPS when bridging device without ethtool

[David Miller]

From: Matthew Wilcox <matthew@wil.cx>
Date: Thu, 30 Aug 2007 10:48:13 -0600

> On Thu, Aug 30, 2007 at 08:29:32AM -0700, Stephen Hemminger wrote:
> > Bridge code calls ethtool to get speed. The conversion to using
> > only ethtool_ops broke the case of devices without ethtool_ops.
> > This is a new regression in 2.6.23.
> > 
> > Rearranged the switch to a logical order, and use gcc initializer.
> > 
> > Ps: speed should have been part of the network device structure from
> >     the start rather than burying it in ethtool.
> 
> Feel free to do the conversion ;-)  One of the things I like about the
> ethtool framework is it gives us a way to take stuff out of the drivers
> and put it in the midlayer without disturbing userspace.
> 
> > Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
> 
> Acked-by: Matthew Wilcox <matthew@wil.cx>

Applied, thanks!