| 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: <1301356123.27727.41.camel@bwh-desktop>
Date: Tue, 29 Mar 2011 00:48:43 +0100
From: Ben Hutchings <bhutchings@...arflare.com>
To: Mahesh Bandewar <maheshb@...gle.com>
Cc: David Miller <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>,
Tom Herbert <therbert@...gle.com>,
Michał Mirosław <mirq-linux@...e.qmqm.pl>
Subject: Re: [PATCH net-next] net-ethtool: Allow ethtool to set interface
in loopback mode.
I'm sorry that things keep changing under your feet. Unfortunately I'm
going to have to ask for more changes; see below.
I'm cc'ing Michał Mirosław so he can comment on how he thinks the
generic feature handling should be extended.
On Mon, 2011-03-28 at 15:24 -0700, Mahesh Bandewar wrote:
> Add second word for feature (currently it's single word). Also use the first
> bit of the second word to set the loopback mode. By configuring the interface
> in loopback mode in conjunction with a policy route / rule, a user-land
> application can stress the egress / ingress path exposing the flows of the
> change in progress and potentially help developer(s) understand the impact of
> those changes without even sending a packet out on the network.
>
> Following set of commands illustrates one such example -
> a) ip -4 addr add 192.168.1.1/24 dev eth1
> b) ip -4 rule add from all iif eth1 lookup 250
> c) ip -4 route add local 0/0 dev lo proto kernel scope host table 250
> d) arp -Ds 192.168.1.100 eth1
> e) arp -Ds 192.168.1.200 eth1
> f) sysctl -w net.ipv4.ip_nonlocal_bind=1
> g) sysctl -w net.ipv4.conf.all.accept_local=1
> # Assuming that the machine has 8 cores
> h) taskset 000f netserver -L 192.168.1.200
> i) taskset 00f0 netperf -t TCP_CRR -L 192.168.1.100 -H 192.168.1.200 -l 30
>
> Signed-off-by: Mahesh Bandewar <maheshb@...gle.com>
> ---
> include/linux/ethtool.h | 14 ++++++++++++
> net/core/ethtool.c | 54 +++++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index aac3e2e..a42f60c 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -526,6 +526,8 @@ struct ethtool_flash {
>
> /* for returning and changing feature sets */
>
> +#define ETHTOOL_DEV_FEATURE_WORDS 2
If this needs to be exposed in ethtool.h, it should be #ifdef
__KERNEL__. User-space should find out the number of features at
run-time.
> /**
> * struct ethtool_get_features_block - block with state of 32 features
> * @available: mask of changeable features
> @@ -594,6 +596,8 @@ struct ethtool_sfeatures {
> * %ETHTOOL_F_COMPAT - some or all changes requested were made by calling
> * compatibility functions. Requested offload state cannot be properly
> * managed by kernel.
> + * %ETHTOOL_F_2NDWORD - some or all changes requested in the second word of
> + * the features failed.
Why do you think it is necessary to distinguish between the words?
> *
> * Meaning of bits in the masks are obtained by %ETHTOOL_GSSET_INFO (number of
> * bits in the arrays - always multiple of 32) and %ETHTOOL_GSTRINGS commands
> @@ -604,11 +608,13 @@ enum ethtool_sfeatures_retval_bits {
> ETHTOOL_F_UNSUPPORTED__BIT,
> ETHTOOL_F_WISH__BIT,
> ETHTOOL_F_COMPAT__BIT,
> + ETHTOOL_F_2NDWORD__BIT,
> };
>
> #define ETHTOOL_F_UNSUPPORTED (1 << ETHTOOL_F_UNSUPPORTED__BIT)
> #define ETHTOOL_F_WISH (1 << ETHTOOL_F_WISH__BIT)
> #define ETHTOOL_F_COMPAT (1 << ETHTOOL_F_COMPAT__BIT)
> +#define ETHTOOL_F_2NDWORD (1 << ETHTOOL_F_2NDWORD__BIT)
>
> #ifdef __KERNEL__
>
> @@ -764,6 +770,8 @@ struct ethtool_ops {
> struct ethtool_rxfh_indir *);
> int (*set_rxfh_indir)(struct net_device *,
> const struct ethtool_rxfh_indir *);
> + int (*get_loopback)(struct net_device *);
> + int (*set_loopback)(struct net_device *, u32);
Part of the idea of the 'features' interface is to treat feature flags
uniformly in ethtool. I think there should be just two driver
operations to get/set all features outside the first word.
[...]
> +static int ethtool_set_secondword_features(struct net_device *dev,
> + struct ethtool_set_features_block *features)
> +{
> + int ret = 0;
> +
> + if (features[1].valid & ETHTOOL_LOOPBACK) {
> + features[1].valid &= ~ETHTOOL_LOOPBACK;
> + if (dev->ethtool_ops->set_loopback) {
> + ret |= dev->ethtool_ops->set_loopback(dev,
> + !!(features[1].requested & ETHTOOL_LOOPBACK));
> + }
> + }
> +
> + return(ret);
> +}
This fails silently if the device doesn't implement set_loopback.
Minor style point: don't use parentheses around the return value.
> +
> static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
> {
> struct ethtool_gfeatures cmd = {
> @@ -251,12 +281,19 @@ static int ethtool_get_features(struct net_device *dev, void __user *useraddr)
> .active = dev->features,
> .never_changed = NETIF_F_NEVER_CHANGE,
> },
> + {
> + .available = ETHTOOL_VALID_2NDWORD_FEATURES,
Should be 0 by default.
> + .active = 0,
> + },
> };
> u32 __user *sizeaddr;
> u32 copy_size;
>
> ethtool_get_features_compat(dev, features);
>
> + /* Get 2nd-word features */
> + ethtool_get_secondword_features(dev, features);
> +
If you define a get_ext_features operation, this could be:
if (dev->ethtool_ops && dev->ethtool_ops->get_ext_features)
dev->ethtool_ops->get_ext_features(dev, features);
> sizeaddr = useraddr + offsetof(struct ethtool_gfeatures, size);
> if (get_user(copy_size, sizeaddr))
> return -EFAULT;
> @@ -289,14 +326,20 @@ static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
> if (copy_from_user(features, useraddr, sizeof(features)))
> return -EFAULT;
>
> - if (features[0].valid & ~NETIF_F_ETHTOOL_BITS)
> + if (features[0].valid & ~NETIF_F_ETHTOOL_BITS ||
> + features[1].valid & ~ETHTOOL_VALID_2NDWORD_FEATURES)
> return -EINVAL;
>
> if (ethtool_set_features_compat(dev, features))
> ret |= ETHTOOL_F_COMPAT;
>
> - if (features[0].valid & ~dev->hw_features) {
> + if (ethtool_set_secondword_features(dev, features))
> + ret |= ETHTOOL_F_2NDWORD;
> +
> + if ((features[0].valid & ~dev->hw_features) ||
> + (features[1].valid & ~ETHTOOL_VALID_2NDWORD_FEATURES)) {
We've already checked features[1].valid against this mask, so it is
pointless to do it again. What we want to do here is to check against
what the hardware and driver support. And beyond the first word of
features, we don't know the answer to that. Maybe dev->hw_features
should be turned into an array?
If you define a set_ext_features operation,
ethtool_set_secondword_features() could be replaced with:
if (features[1].valid) {
if (!dev->ethtool_ops || !dev->ethtool_ops->set_ext_features) {
ret |= ETHTOOL_F_UNSUPPORTED;
} else {
int ext_ret =
dev->ethtool_ops->set_ext_features(dev, features);
if (ext_ret < 0)
return ext_ret;
ret |= ext_ret;
}
}
Ben.
> features[0].valid &= dev->hw_features;
> + features[1].valid &= ETHTOOL_VALID_2NDWORD_FEATURES;
> ret |= ETHTOOL_F_UNSUPPORTED;
> }
>
> @@ -346,6 +389,9 @@ static const char netdev_features_strings[ETHTOOL_DEV_FEATURE_WORDS * 32][ETH_GS
> /* NETIF_F_RXCSUM */ "rx-checksum",
> "",
> "",
> +
> + /* 2nd word of features */
> + /* ETHTOOL_LOOPBACK */ "loopback",
> };
>
> static int __ethtool_get_sset_count(struct net_device *dev, int sset)
--
Ben Hutchings, Senior Software Engineer, Solarflare
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