| 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: <1292541186.18294.16.camel@bwh-desktop>
Date: Thu, 16 Dec 2010 23:13:06 +0000
From: Ben Hutchings <bhutchings@...arflare.com>
To: Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc: netdev@...r.kernel.org
Subject: Re: [RFC PATCH 02/12] net: Introduce new feature setting ops
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.
> [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;
...
[...]
> --- 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.
> --- 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.
> + 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.
[...]
> 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?
> + 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.)
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.
--
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