netdev - Re: [RFC PATCH 03/12] net: ethtool: use ndo_fix

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20101219005404.GB12005@rere.qmqm.pl>
Date:	Sun, 19 Dec 2010 01:54:04 +0100
From:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [RFC PATCH 03/12] net: ethtool: use ndo_fix_features for
	offload setting

On Thu, Dec 16, 2010 at 11:23:49PM +0000, Ben Hutchings wrote:
> On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
> > Signed-off-by: Michał Mirosław <mirq-linux@...e.qmqm.pl>
> > ---
> >  include/linux/netdevice.h |    2 +
> >  net/core/dev.c            |    8 ++
> >  net/core/ethtool.c        |  252 +++++++++++++++++++-------------------------
> >  3 files changed, 119 insertions(+), 143 deletions(-)
> > 
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 4b20944..7634cab 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -941,6 +941,8 @@ struct net_device {
> >  #define NETIF_F_V6_CSUM		(NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
> >  #define NETIF_F_ALL_CSUM	(NETIF_F_V4_CSUM | NETIF_F_V6_CSUM)
> >  
> > +#define NETIF_F_ALL_TSO 	(NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_TSO_ECN)
> > +
> >  	/*
> >  	 * If one device supports one of these features, then enable them
> >  	 * for all in netdev_increment_features.
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 1e616bb..95d0a49 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -5054,6 +5054,14 @@ unsigned long netdev_fix_features(unsigned long features, const char *name)
> >  		features &= ~NETIF_F_TSO;
> >  	}
> >  
> > +	/* Software GSO depends on SG. */
> > +	if ((features & NETIF_F_GSO) && !(features & NETIF_F_SG)) {
> > +		if (name)
> > +			printk(KERN_NOTICE "%s: Dropping NETIF_F_GSO since no "
> > +			       "SG feature.\n", name);
> > +		features &= ~NETIF_F_GSO;
> > +	}
> > +
> >  	if (features & NETIF_F_UFO) {
> >  		/* maybe split UFO into V4 and V6? */
> >  		if (!((features & NETIF_F_GEN_CSUM) ||
> The severity of these messages will need to be reduced if ethtool relies
> on this function to propagate feature changes.  (And I wonder why some
> of them are ERR and some NOTICE.)

Patch is ready (all converted to netdev_info).

> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index f08e7f1..b068738 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -55,6 +55,7 @@ int ethtool_op_set_tx_csum(struct net_device *dev, u32 data)
> >  
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL(ethtool_op_set_tx_csum);
> >  
> >  int ethtool_op_set_tx_hw_csum(struct net_device *dev, u32 data)
> >  {
> > @@ -220,6 +221,85 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
> >  	return 0;
> >  }
> >  
> > +static unsigned long ethtool_get_feature_mask(u32 eth_cmd)
> > +{
> > +	switch (eth_cmd) {
> > +	case ETHTOOL_GTXCSUM:
> > +	case ETHTOOL_STXCSUM:
> > +		return NETIF_F_ALL_CSUM|NETIF_F_SCTP_CSUM;
> I wonder whether this should cover NETIF_F_FCOE_CRC as well (ixgbe
> currently doesn't seem to allow toggling it).

This mask is only for 'legacy' set_tx_csum ethtool commands. Other
checksumming options - or whatever bits are included in hw_features - can be
toggled by ETHTOOL_SFEATURES.

> There should be spaces around the '|' operator.

Fixed.

> > +static int __ethtool_set_tx_csum(struct net_device *dev, u32 data);
> > +static int __ethtool_set_sg(struct net_device *dev, u32 data);
> > +static int __ethtool_set_tso(struct net_device *dev, u32 data);
> > +static int __ethtool_set_ufo(struct net_device *dev, u32 data);
> > +
> > +static int ethtool_set_one_feature(struct net_device *dev,
> > +	void __user *useraddr, u32 ethcmd)
> > +{
> > +	struct ethtool_value edata;
> > +	unsigned long mask;
> > +
> > +	if (copy_from_user(&edata, useraddr, sizeof(edata)))
> > +		return -EFAULT;
> > +
> > +	mask = ethtool_get_feature_mask(ethcmd);
> > +	mask &= dev->hw_features | NETIF_F_SOFT_FEATURES;
> > +	if (mask) {
> > +		if (edata.data)
> > +			dev->wanted_features |= mask;
> > +		else
> > +			dev->wanted_features &= ~mask;
> > +
> > +		netdev_update_features(dev);
> > +		return 0;
> > +	}
> > +
> > +	switch (ethcmd) {
> > +	case ETHTOOL_STXCSUM:
> > +		return __ethtool_set_tx_csum(dev, edata.data);
> > +	case ETHTOOL_SSG:
> > +		return __ethtool_set_sg(dev, edata.data);
> > +	case ETHTOOL_STSO:
> > +		return __ethtool_set_tso(dev, edata.data);
> > +	case ETHTOOL_SUFO:
> > +		return __ethtool_set_ufo(dev, edata.data);
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +}
> [...]
> 
> This deserves some comments.

Sure.

This is compatibility layer for current ethtool tools, and drivers
not converted to ndo_fix_features() and friends.

Best Regards,
Michał Mirosław
--
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