Re: [RFC PATCH 02/12] net: Introduce new feature setting ops

[Ben Hutchings <bhutchings@solarflare.com>]

On Sun, 2010-12-19 at 01:49 +0100, Michał Mirosław wrote:
> On Thu, Dec 16, 2010 at 11:13:06PM +0000, Ben Hutchings wrote:
> > On Wed, 2010-12-15 at 23:24 +0100, Michał Mirosław wrote:
[...]
> > > +static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
> > > +{
> > > +	struct ethtool_features cmd;
> > > +	struct ethtool_set_features_block features[ETHTOOL_DEV_FEATURE_WORDS];
> > > +
> > > +	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> > > +		return -EFAULT;
> > > +	useraddr += sizeof(cmd);
> > > +
> > > +	if (cmd.count > ETHTOOL_DEV_FEATURE_WORDS)
> > > +		cmd.count = ETHTOOL_DEV_FEATURE_WORDS;
> > So additional feature words will be silently ignored...
> > > +	if (copy_from_user(features, useraddr, sizeof(*features) * cmd.count))
> > > +		return -EFAULT;
> > > +	memset(features + cmd.count, 0,
> > > +		sizeof(features) - sizeof(*features) * cmd.count);
> > > +
> > > +	features[0].valid &= dev->hw_features | NETIF_F_SOFT_FEATURES;
> > [...]
> > 
> > ...as will any other unsupported features.  This is not a good idea.
> > (However, remembering which features are wanted does seem like a good
> > idea.)
> 
> That's intentional. Unsupported features can't be enabled anyway.
> hw_features is supposed to contain all features that the device can support
> and is able to enable/disable. This set should be constant and anything that
> is in the wanted_features set but is not supported because of other conditions
> will be stripped by ndo_fix_features() call.
> 
> Other way would be to return EINVAL when bits not changeable are present in
> the valid mask. I don't want to do that, since then your example of changing
> a feature without GFEATURES first will not work.

That's right, it shouldn't work.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.