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
| ||
|
Message-ID: <20101219004959.GA12005@rere.qmqm.pl> Date: Sun, 19 Dec 2010 01:49:59 +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 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: > > This introduces a new framework to handle device features setting. > > It consists of: > > - new fields in struct net_device: > > + hw_features - features that hw/driver supports toggling > > + wanted_features - features that user wants enabled, when possible > > - new netdev_ops: > > + feat = ndo_fix_features(dev, feat) - API checking constraints for > > enabling features or their combinations > > + ndo_set_features(dev) - API updating hardware state to match > > changed dev->features > > - new ethtool commands: > > + ETHTOOL_GFEATURES/ETHTOOL_SFEATURES: get/set dev->wanted_features > > and trigger device reconfiguration if resulting dev->features > > changed > > [TODO: this might be extended to support device-specific flags, and > > keep NETIF_F flags from becoming part of ABI by using GET_STRINGS > > for describing the bits] > We already have ETHTOOL_{G,S}PFLAGS for that, though. Since we're almost out of bits in features, I thought that I might as well just take care of that in one go. I'll implement the GET_STRINGS part to show what I mean about bits not becoming an ABI. > > [Note: ETHTOOL_GFEATURES and ETHTOOL_SFEATURES' data is supposed to > > be 'compatible', so that you can R/M/W without additional copying] > But if you expect userland to do that, what is the point of the 'valid' > mask? Shouldn't userland do something like: > > struct ethtool_features feat = { ETHTOOL_SFEATURES }; > ... > if (off_tso_wanted >= 0) > feat.features[0].valid |= NETIF_F_TSO; > if (off_tso_wanted > 0) > feat.features[0].requested |= NETIF_F_TSO; > ... That looks even better than what I thought originally. So this means I can just get rid of the combined ethtool cmd struct. > [...] > > --- a/include/linux/ethtool.h > > +++ b/include/linux/ethtool.h > > @@ -523,6 +523,31 @@ struct ethtool_flash { > > char data[ETHTOOL_FLASH_MAX_FILENAME]; > > }; > > > > +/* for returning feature sets */ > > +#define ETHTOOL_DEV_FEATURE_WORDS 1 > > + > > +struct ethtool_get_features_block { > > + __u32 available; /* features togglable */ > > + __u32 requested; /* features requested to be enabled */ > > + __u32 active; /* features currently enabled */ > > + __u32 __pad[1]; > > +}; > > + > > +struct ethtool_set_features_block { > > + __u32 valid; /* bits valid in .requested */ > > + __u32 requested; /* features requested */ > > + __u32 __pad[2]; > > +}; > > + > > +struct ethtool_features { > > + __u32 cmd; > > + __u32 count; /* blocks */ > > + union { > > + struct ethtool_get_features_block get; > > + struct ethtool_set_features_block set; > > + } features[0]; > > +}; > I want kernel-doc comments with a proper description of semantics. Will do. > > --- a/include/linux/netdevice.h > > +++ b/include/linux/netdevice.h > [...] > > @@ -934,6 +949,14 @@ struct net_device { > > NETIF_F_SG | NETIF_F_HIGHDMA | \ > > NETIF_F_FRAGLIST) > > > > + /* toggable features with no driver requirements */ > > +#define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO) > > + > > + /* ethtool-toggable features */ > > The verb is 'toggle' so this adjective should be either 'togglable' or > 'toggleable'. Or you could choose a different adjective. 'changed'. Or fixed. ;-) > > + unsigned long hw_features; > > + /* ethtool-requested features */ > > + unsigned long wanted_features; > > + > While you're at it, you could change all these 'features' fields and > parameters to u32, since we presumably won't be defining features that > can only be enabled on 64-bit architectures. Done. I also went through functions that pass features along and fixed their types. Patch will be in the next version of this series. > [...] > > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > > index 1774178..f08e7f1 100644 > > --- a/net/core/ethtool.c > > +++ b/net/core/ethtool.c > > @@ -171,6 +171,55 @@ EXPORT_SYMBOL(ethtool_ntuple_flush); > > > > /* Handlers for each ethtool command */ > > > > +static int ethtool_get_features(struct net_device *dev, void __user *useraddr) > > +{ > > + struct ethtool_features cmd = { > > + .cmd = ETHTOOL_GFEATURES, > > + .count = ETHTOOL_DEV_FEATURE_WORDS, > > + }; > > + struct ethtool_get_features_block features[ETHTOOL_DEV_FEATURE_WORDS] = { > > + { > > + .available = dev->hw_features, > > + .requested = dev->wanted_features, > > + .active = dev->features, > > + }, > > + }; > > + > > + if (copy_to_user(useraddr, &cmd, sizeof(cmd))) > > + return -EFAULT; > > + useraddr += sizeof(cmd); > > + if (copy_to_user(useraddr, features, sizeof(features))) > > + return -EFAULT; > If ETHTOOL_DEV_FEATURE_WORDS increases, how do you know the user buffer > will be big enough? I'll change that to use count from GET_STRINGS. > > + return 0; > > +} > > + > > +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. 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
Powered by blists - more mailing lists