Linux Ethernet Bridging - Aug 2007 - [Bridge] Re: oops with recent wireless-dev tree

On Thu, 30 Aug 2007 14:05:30 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:
> 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?
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.

The bug was introduced by this:

commit 61a44b9c4b20d40c41fd1b70a4ceb13b75ea79a4
Author: Matthew Wilcox <matthew@wil.cx>
Date:   Tue Jul 31 14:00:02 2007 -0700

    [NET]: ethtool ops are the only way
    
    During the transition to the ethtool_ops way of doing things, we supported
    calling the device's ->do_ioctl method to allow unconverted drivers
to
    continue working.  Those days are long behind us, all in-tree drivers
    use the ethtool_ops way, and so we no longer need to support this.
    
    The bonding driver is the biggest beneficiary of this; it no longer
    needs to call ioctl() as a fallback if ethtool_ops aren't supported.
    
    Also put a proper copyright statement on ethtool.c.
    
    Signed-off-by: Matthew Wilcox <matthew@wil.cx>
    Signed-off-by: David S. Miller <davem@davemloft.net>

-- 
Stephen Hemminger <shemminger@linux-foundation.org>

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;
 			}

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."

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."