netdev - RE: [Pv-drivers] [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect implementation of ethtool_ops::get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8B1F619C9F5F454E81D90D3C161698D7033625111F@EXCH-MBX-3.vmware.com>
Date:	Wed, 30 Jun 2010 09:46:51 -0700
From:	Bhavesh Davda <bhavesh@...are.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
CC:	David Miller <davem@...emloft.net>,
	"VMware, Inc." <pv-drivers@...are.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [Pv-drivers] [PATCH net-next-2.6 3/3] vmxnet3: Remove incorrect
 implementation of ethtool_ops::get_flags()

> From: Ben Hutchings [mailto:bhutchings@...arflare.com]
> Sent: Wednesday, June 30, 2010 8:59 AM
> 
> On Wed, 2010-06-30 at 08:44 -0700, Bhavesh Davda wrote:
> > Thanks for fixing this, Ben! Had to look at ethtool-2.6.34 src to
> > convince myself of the correctness. Looks good to me.
> >
> > Signed-off-by: Bhavesh Davda <bhavesh@...are.com>
> >
> > ps: I do wonder, however, why not always use ethtool_op_get_flags for
> > all drivers, and mask whatever is returned by the driver specific
> > dev->ethtool_ops->get_flags with flags_dup_features instead of this
> > approach?
> 
> I think you're right that ethtool_op_get_flags could be the implicit
> default (i.e. used if ethtool_ops::get_flags is NULL) and drivers
> should
> not have to set it.  Following this change, no drivers in net-next-2.6
> will be using any other implementation.  However, I don't think
> ethtool_ops::get_flags should be removed - in future there are likely
> to
> be additional ethtool flags that do not correspond to net device
> feature
> flags, and some drivers will need a different implementation.
> 
> Ben.
> 

Hi Ben,

I'm not suggesting nuking ethtool_ops::get_flags. What I was suggesting is *always* calling ethtool_ops_get_flags from dev_ethtool for case ETHTOOL_GFLAGS, but doing something like this simple patch:

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index a0f4964..f5da6ed 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -139,8 +139,9 @@ u32 ethtool_op_get_flags(struct net_device *dev)
         * handling for flags which are not so easily handled
         * by a simple masking operation
         */
-
-       return dev->features & flags_dup_features;
+       return (dev->ethtool_ops->get_flags ?
+               dev->ethtool_ops->get_flags(dev) :
+               dev->features) & flags_dup_features;
 }
 EXPORT_SYMBOL(ethtool_op_get_flags);

@@ -1495,9 +1496,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
                break;
        case ETHTOOL_GFLAGS:
                rc = ethtool_get_value(dev, useraddr, ethcmd,
-                                      (dev->ethtool_ops->get_flags ?
-                                       dev->ethtool_ops->get_flags :
-                                       ethtool_op_get_flags));
+                                       ethtool_op_get_flags);
                break;
        case ETHTOOL_SFLAGS:
                rc = ethtool_set_value(dev, useraddr,

Plus nuking all such code in the netdev drivers:
        .get_flags              = ethtool_op_get_flags,

This is still pretty fragile and could lead to infinite recursion if some drivers are missed. But you get the idea.

Thanks

- Bhavesh
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html