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

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: <20101219234343.GA15644@rere.qmqm.pl>
Date:	Mon, 20 Dec 2010 00:43:43 +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 02/12] net: Introduce new feature setting ops

On Sun, Dec 19, 2010 at 09:22:39PM +0000, Ben Hutchings wrote:
> 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.

A user says "enable any TSO available". This means ethtool could issue
a request with .valid = NETIF_F_ALL_TSO, .requested = NETIF_F_ALL_TSO.
If the device supports only TSOv4 this should enable it and leave others
alone as whatever the user wants they can't be enabled.

This is 1-1 conversion of the semantics current ethtool ops have - set_tso
corresponds exactly to the request described above. This behaviour also
allows to execute a command like "enable as many as you can of ..." that
is usual goal of user enabling hardware offloads - to get best possible
performance.

Nevertheless, what problem is generated by ignoring unsupported bits here?
I can see the point of returning -EINVAL on bits that are not defined, though.
Is that a good direction?

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