netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations
       [not found]       ` <51E5BE27.3060207@qti.qualcomm.com>
@ 2013-07-23 13:32         ` Helmut Grohne
  2013-07-23 15:49           ` Rick Jones
                             ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Helmut Grohne @ 2013-07-23 13:32 UTC (permalink / raw)
  To: netdev; +Cc: David Miller, Max Krasnyansky

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]


On 16.07.2013, at 23:41, Max Krasnyansky <maxk@qti.qualcomm.com> wrote:
> The patch looks fine to me (must admit that I only glanced at it). Please send it to the netdev
> mailing list netdev@vger.kernel.org, if you have't already done so.
> CC David Miller <davem@davemloft.net> and me.

Doing that. Now summarizing the issue for the new recipients:

Problem:

When querying a tap device for its speed using ethtool the tun driver reports a speed
of SPEED_10. This number is hard coded into the driver. Nowadays virtual network devices
can go way faster than that. When doing automatic bandwidth monitoring based on the
speed reported by ethtool, tap devices tend to come up as false positives. Arguably the
hard coded speed is wrong.

Proposed solution:

To that end I propose supporting the ETHTOOL_SSET command in addition to the already
supported ETHTOOL_GSET. It would deny setting any setting except for the bandwidth
where it would allow arbitrary values. You can find this patch attached.

With this patch an administrator can increase the reported speed for tap devices and
keep using automatic detection of interface speeds.

Workarounds:

Using ethtool a detection utility can determine the driver for an interface. If the
driver matches the string "tun", the reported speed should not be used.

Helmut


[-- Attachment #2: 0001-tuntap-allow-changing-ethtool-speed.patch --]
[-- Type: application/octet-stream, Size: 2816 bytes --]

From 9e0d2928ebed1d74ec92d131dcdc7511dde867ac Mon Sep 17 00:00:00 2001
From: Helmut Grohne <h.grohne@cygnusnetworks.de>
Date: Fri, 17 May 2013 14:03:47 +0200
Subject: [PATCH] tuntap: allow changing ethtool speed

The data reported by the ethtool interface for tun/tap devices is
artificial. The default speed is 10Mbit. Virtual interfaces are often
faster than this nowadays, so the observed bandwidth may exceed the
available bandwidth. This patch allows an administrator to change the
available bandwidth as reported by ethtool.

Signed-off-by: Helmut Grohne <h.grohne@cygnusnetworks.de>
---
 drivers/net/tun.c |   25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index f042b03..2555a11 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -187,6 +187,7 @@ struct tun_struct {
 	struct list_head disabled;
 	void *security;
 	u32 flow_count;
+	u32 advertised_speed;
 };
 
 static inline u32 tun_hashfn(u32 rxhash)
@@ -1404,6 +1405,7 @@ static void tun_setup(struct net_device *dev)
 
 	tun->owner = INVALID_UID;
 	tun->group = INVALID_GID;
+	tun->advertised_speed = SPEED_10;
 
 	dev->ethtool_ops = &tun_ethtool_ops;
 	dev->destructor = tun_free_netdev;
@@ -2197,9 +2199,11 @@ static struct miscdevice tun_miscdev = {
 
 static int tun_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
+	struct tun_struct *tun = netdev_priv(dev);
+
 	cmd->supported		= 0;
 	cmd->advertising	= 0;
-	ethtool_cmd_speed_set(cmd, SPEED_10);
+	ethtool_cmd_speed_set(cmd, tun->advertised_speed);
 	cmd->duplex		= DUPLEX_FULL;
 	cmd->port		= PORT_TP;
 	cmd->phy_address	= 0;
@@ -2210,6 +2214,24 @@ static int tun_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 	return 0;
 }
 
+static int tun_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+
+	if(cmd->supported != 0 ||
+			cmd->advertising != 0 ||
+			cmd->duplex != DUPLEX_FULL ||
+			cmd->port != PORT_TP ||
+			cmd->phy_address != 0 ||
+			cmd->transceiver != XCVR_INTERNAL ||
+			cmd->autoneg != AUTONEG_DISABLE ||
+			cmd->maxtxpkt != 0 ||
+			cmd->maxrxpkt != 0)
+		return -EOPNOTSUPP;
+	tun->advertised_speed = ethtool_cmd_speed(cmd);
+	return 0;
+}
+
 static void tun_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
 {
 	struct tun_struct *tun = netdev_priv(dev);
@@ -2247,6 +2269,7 @@ static void tun_set_msglevel(struct net_device *dev, u32 value)
 
 static const struct ethtool_ops tun_ethtool_ops = {
 	.get_settings	= tun_get_settings,
+	.set_settings	= tun_set_settings,
 	.get_drvinfo	= tun_get_drvinfo,
 	.get_msglevel	= tun_get_msglevel,
 	.set_msglevel	= tun_set_msglevel,
-- 
1.7.10.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations
  2013-07-23 13:32         ` TUN/TAP: tap driver reports bogus interface speed in ethtool operations Helmut Grohne
@ 2013-07-23 15:49           ` Rick Jones
  2013-07-23 15:56             ` Stephen Hemminger
  2013-07-23 17:28           ` [RFC 1/2] tun: allow overrriding ethtool info Stephen Hemminger
  2013-07-23 19:04           ` TUN/TAP: tap driver reports bogus interface speed in ethtool operations Ben Hutchings
  2 siblings, 1 reply; 9+ messages in thread
From: Rick Jones @ 2013-07-23 15:49 UTC (permalink / raw)
  To: Helmut Grohne; +Cc: netdev, David Miller, Max Krasnyansky

On 07/23/2013 06:32 AM, Helmut Grohne wrote:
>
> On 16.07.2013, at 23:41, Max Krasnyansky <maxk@qti.qualcomm.com> wrote:
>> The patch looks fine to me (must admit that I only glanced at it). Please send it to the netdev
>> mailing list netdev@vger.kernel.org, if you have't already done so.
>> CC David Miller <davem@davemloft.net> and me.
>
> Doing that. Now summarizing the issue for the new recipients:
>
> Problem:
>
> When querying a tap device for its speed using ethtool the tun driver reports a speed
> of SPEED_10. This number is hard coded into the driver. Nowadays virtual network devices
> can go way faster than that. When doing automatic bandwidth monitoring based on the
> speed reported by ethtool, tap devices tend to come up as false positives. Arguably the
> hard coded speed is wrong.
>
> Proposed solution:
>
> To that end I propose supporting the ETHTOOL_SSET command in addition to the already
> supported ETHTOOL_GSET. It would deny setting any setting except for the bandwidth
> where it would allow arbitrary values. You can find this patch attached.
>
> With this patch an administrator can increase the reported speed for tap devices and
> keep using automatic detection of interface speeds.
>
> Workarounds:
>
> Using ethtool a detection utility can determine the driver for an interface. If the
> driver matches the string "tun", the reported speed should not be used.
>
> Helmut
>

I like the idea of being able to set the reported speed for a tun/tap 
interface.

rick jones

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations
  2013-07-23 15:49           ` Rick Jones
@ 2013-07-23 15:56             ` Stephen Hemminger
  2013-07-23 16:03               ` Helmut Grohne
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2013-07-23 15:56 UTC (permalink / raw)
  To: Rick Jones; +Cc: Helmut Grohne, netdev, David Miller, Max Krasnyansky

On Tue, 23 Jul 2013 08:49:01 -0700
Rick Jones <rick.jones2@hp.com> wrote:

> On 07/23/2013 06:32 AM, Helmut Grohne wrote:
> >
> > On 16.07.2013, at 23:41, Max Krasnyansky <maxk@qti.qualcomm.com> wrote:
> >> The patch looks fine to me (must admit that I only glanced at it). Please send it to the netdev
> >> mailing list netdev@vger.kernel.org, if you have't already done so.
> >> CC David Miller <davem@davemloft.net> and me.
> >
> > Doing that. Now summarizing the issue for the new recipients:
> >
> > Problem:
> >
> > When querying a tap device for its speed using ethtool the tun driver reports a speed
> > of SPEED_10. This number is hard coded into the driver. Nowadays virtual network devices
> > can go way faster than that. When doing automatic bandwidth monitoring based on the
> > speed reported by ethtool, tap devices tend to come up as false positives. Arguably the
> > hard coded speed is wrong.
> >
> > Proposed solution:
> >
> > To that end I propose supporting the ETHTOOL_SSET command in addition to the already
> > supported ETHTOOL_GSET. It would deny setting any setting except for the bandwidth
> > where it would allow arbitrary values. You can find this patch attached.
> >
> > With this patch an administrator can increase the reported speed for tap devices and
> > keep using automatic detection of interface speeds.
> >
> > Workarounds:
> >
> > Using ethtool a detection utility can determine the driver for an interface. If the
> > driver matches the string "tun", the reported speed should not be used.
> >
> > Helmut
> >
> 
> I like the idea of being able to set the reported speed for a tun/tap 
> interface.
> 
> rick jones

I have hack patch to allow overwriting speed, info, and statistics that is part
of how we use TUN as dummy devices. How far do you think makes sense?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations
  2013-07-23 15:56             ` Stephen Hemminger
@ 2013-07-23 16:03               ` Helmut Grohne
  0 siblings, 0 replies; 9+ messages in thread
From: Helmut Grohne @ 2013-07-23 16:03 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Rick Jones, netdev, David Miller, Max Krasnyansky

On 23.07.2013, at 17:56, Stephen Hemminger <stephen@networkplumber.org> wrote:
> I have hack patch to allow overwriting speed, info, and statistics that is part
> of how we use TUN as dummy devices. How far do you think makes sense?

That could prove useful for creating test environments for tools, that use ethtool.
If you have a better patch, then go for it. The reason for why I did not include
other settings was that I don't need them. I'm open to other solutions as well, as
long as they either make the speed modifiable or something faster than SPEED_10.

Helmut

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC 1/2] tun: allow overrriding ethtool info
  2013-07-23 13:32         ` TUN/TAP: tap driver reports bogus interface speed in ethtool operations Helmut Grohne
  2013-07-23 15:49           ` Rick Jones
@ 2013-07-23 17:28           ` Stephen Hemminger
  2013-07-23 17:29             ` [RFC 2/2] tun: allow overriding statistics Stephen Hemminger
  2013-07-23 19:04           ` TUN/TAP: tap driver reports bogus interface speed in ethtool operations Ben Hutchings
  2 siblings, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2013-07-23 17:28 UTC (permalink / raw)
  To: Helmut Grohne; +Cc: netdev, David Miller, Max Krasnyansky

This patch adds new ioctl to allow overrriding the ethtool information
returned by the TUN device. This is useful when using tun device as a surrogate
for hardware or other software emulation.

If the application does not override the ethtool settings, the
original hard coded values are used.

This version is compile tested only, included to show how API works.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

---
 drivers/net/tun.c           |   56 +++++++++++++++++++++++++++++++++-----------
 include/uapi/linux/if_tun.h |   15 +++++++++++
 2 files changed, 58 insertions(+), 13 deletions(-)

--- a/drivers/net/tun.c	2013-07-23 09:18:01.936046624 -0700
+++ b/drivers/net/tun.c	2013-07-23 09:52:37.054143993 -0700
@@ -187,6 +187,8 @@ struct tun_struct {
 	struct list_head disabled;
 	void *security;
 	u32 flow_count;
+
+	struct tun_info info;
 };
 
 static inline u32 tun_hashfn(u32 rxhash)
@@ -870,9 +872,12 @@ static void tun_net_init(struct net_devi
 {
 	struct tun_struct *tun = netdev_priv(dev);
 
+	strlcpy(tun->info.driver, DRV_NAME, sizeof(tun->info.driver));
+
 	switch (tun->flags & TUN_TYPE_MASK) {
 	case TUN_TUN_DEV:
 		dev->netdev_ops = &tun_netdev_ops;
+		strlcpy(tun->info.bus, "tun", sizeof(tun->info.bus));
 
 		/* Point-to-Point TUN Device */
 		dev->hard_header_len = 0;
@@ -887,6 +892,8 @@ static void tun_net_init(struct net_devi
 
 	case TUN_TAP_DEV:
 		dev->netdev_ops = &tap_netdev_ops;
+		strlcpy(tun->info.bus, "tap", sizeof(tun->info.bus));
+
 		/* Ethernet TAP Device */
 		ether_setup(dev);
 		dev->priv_flags &= ~IFF_TX_SKB_SHARING;
@@ -1426,6 +1433,9 @@ static void tun_setup(struct net_device
 
 	tun->owner = INVALID_UID;
 	tun->group = INVALID_GID;
+	tun->info.speed = SPEED_10;
+	tun->info.duplex = DUPLEX_FULL;
+	tun->info.port = PORT_TP;
 
 	dev->ethtool_ops = &tun_ethtool_ops;
 	dev->destructor = tun_free_netdev;
@@ -2088,6 +2098,15 @@ static long __tun_chr_ioctl(struct file
 		tun_detach_filter(tun, tun->numqueues);
 		break;
 
+	case TUNSETINFO: {
+		struct tun_info info;
+		if (copy_from_user(argp, &info, sizeof(info)))
+			ret = -EFAULT;
+
+		tun->info = info;
+		break;
+	}
+
 	default:
 		ret = -EINVAL;
 		break;
@@ -2229,11 +2248,13 @@ static struct miscdevice tun_miscdev = {
 
 static int tun_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
+	struct tun_struct *tun = netdev_priv(dev);
+
 	cmd->supported		= 0;
 	cmd->advertising	= 0;
-	ethtool_cmd_speed_set(cmd, SPEED_10);
-	cmd->duplex		= DUPLEX_FULL;
-	cmd->port		= PORT_TP;
+	ethtool_cmd_speed_set(cmd, tun->info.speed);
+	cmd->duplex		= tun->info.duplex;
+	cmd->port		= tun->info.port;
 	cmd->phy_address	= 0;
 	cmd->transceiver	= XCVR_INTERNAL;
 	cmd->autoneg		= AUTONEG_DISABLE;
@@ -2242,21 +2263,24 @@ static int tun_get_settings(struct net_d
 	return 0;
 }
 
+static int tun_set_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+
+	tun->info.speed  = ethtool_cmd_speed(ecmd);
+	tun->info.duplex = ecmd->duplex;
+	tun->info.port   = ecmd->port;
+
+	return 0;
+}
+
 static void tun_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
 {
 	struct tun_struct *tun = netdev_priv(dev);
 
-	strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
+	strlcpy(info->driver, tun->info.driver, sizeof(info->driver));
 	strlcpy(info->version, DRV_VERSION, sizeof(info->version));
-
-	switch (tun->flags & TUN_TYPE_MASK) {
-	case TUN_TUN_DEV:
-		strlcpy(info->bus_info, "tun", sizeof(info->bus_info));
-		break;
-	case TUN_TAP_DEV:
-		strlcpy(info->bus_info, "tap", sizeof(info->bus_info));
-		break;
-	}
+	strlcpy(info->bus_info, tun->info.bus, sizeof(info->bus_info));
 }
 
 static u32 tun_get_msglevel(struct net_device *dev)
@@ -2279,6 +2303,7 @@ static void tun_set_msglevel(struct net_
 
 static const struct ethtool_ops tun_ethtool_ops = {
 	.get_settings	= tun_get_settings,
+	.set_settings	= tun_set_settings,
 	.get_drvinfo	= tun_get_drvinfo,
 	.get_msglevel	= tun_get_msglevel,
 	.set_msglevel	= tun_set_msglevel,
--- a/include/uapi/linux/if_tun.h	2013-07-23 09:18:01.936046624 -0700
+++ b/include/uapi/linux/if_tun.h	2013-07-23 09:52:26.870271418 -0700
@@ -56,6 +56,7 @@
 #define TUNGETVNETHDRSZ _IOR('T', 215, int)
 #define TUNSETVNETHDRSZ _IOW('T', 216, int)
 #define TUNSETQUEUE  _IOW('T', 217, int)
+#define TUNSETINFO     _IOW('T', 219, struct tun_info)
 
 /* TUNSETIFF ifr flags */
 #define IFF_TUN		0x0001
@@ -103,4 +104,17 @@ struct tun_filter {
 	__u8   addr[0][ETH_ALEN];
 };
 
+/*
+ * Ethtool info
+ * This is used to allow spoofing the speed/duplex and driver information
+ */
+struct tun_info {
+	__u32 speed;
+	__u8  duplex;
+	__u8  port;
+
+	char driver[32];
+	char bus[32];
+};
+
 #endif /* _UAPI__IF_TUN_H */

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC 2/2] tun: allow overriding statistics
  2013-07-23 17:28           ` [RFC 1/2] tun: allow overrriding ethtool info Stephen Hemminger
@ 2013-07-23 17:29             ` Stephen Hemminger
  0 siblings, 0 replies; 9+ messages in thread
From: Stephen Hemminger @ 2013-07-23 17:29 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Helmut Grohne, netdev, David Miller, Max Krasnyansky

This patch adds new ioctl to allow overrriding the link statistics
returned by the TUN device. This is useful when using tun device as a surrogate
for hardware or other software emulation. 

The application perodically uses ioctl(TUNSETSTTS) to periodically update statistics.
Exchange and RCU is used to make this operation atomic.

If TUNSETSTATS is not used the original software based statistics
are used.

This version compile tested only, to show how API works.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

--- a/drivers/net/tun.c	2013-07-23 10:17:04.335800821 -0700
+++ b/drivers/net/tun.c	2013-07-23 10:22:10.903948347 -0700
@@ -189,6 +189,10 @@ struct tun_struct {
 	u32 flow_count;
 
 	struct tun_info info;
+	struct tun_stats __rcu {
+		struct rtnl_link_stats64 link_stats;
+		struct rcu_head rcu;
+	} *stats;
 };
 
 static inline u32 tun_hashfn(u32 rxhash)
@@ -802,6 +806,32 @@ static netdev_features_t tun_net_fix_fea
 
 	return (features & tun->set_features) | (features & ~TUN_USER_FEATURES);
 }
+
+static struct rtnl_link_stats64 *
+tun_net_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *storage)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+	struct tun_stats __rcu *stats;
+
+	rcu_read_lock();
+	stats = rcu_dereference(tun->stats);
+	if (stats) {
+		/* Stats received from device */
+		*storage = stats->link_stats;
+		rcu_read_unlock();
+
+		/* Add tunnel detected errors to mix */
+		storage->tx_dropped += dev->stats.tx_dropped;
+		storage->rx_dropped += dev->stats.rx_dropped;
+		storage->rx_frame_errors += dev->stats.rx_frame_errors;
+		return storage;
+	}
+	rcu_read_unlock();
+
+	netdev_stats_to_stats64(storage, &dev->stats);
+	return storage;
+}
+
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static void tun_poll_controller(struct net_device *dev)
 {
@@ -824,6 +854,7 @@ static const struct net_device_ops tun_n
 	.ndo_open		= tun_net_open,
 	.ndo_stop		= tun_net_close,
 	.ndo_start_xmit		= tun_net_xmit,
+	.ndo_get_stats64	= tun_net_get_stats64,
 	.ndo_change_mtu		= tun_net_change_mtu,
 	.ndo_fix_features	= tun_net_fix_features,
 	.ndo_select_queue	= tun_select_queue,
@@ -837,6 +868,7 @@ static const struct net_device_ops tap_n
 	.ndo_open		= tun_net_open,
 	.ndo_stop		= tun_net_close,
 	.ndo_start_xmit		= tun_net_xmit,
+	.ndo_get_stats64	= tun_net_get_stats64,
 	.ndo_change_mtu		= tun_net_change_mtu,
 	.ndo_fix_features	= tun_net_fix_features,
 	.ndo_set_rx_mode	= tun_net_mclist,
@@ -1420,9 +1452,13 @@ out:
 static void tun_free_netdev(struct net_device *dev)
 {
 	struct tun_struct *tun = netdev_priv(dev);
+	struct tun_stats *stats;
 
 	BUG_ON(!(list_empty(&tun->disabled)));
 	tun_flow_uninit(tun);
+	stats = xchg(&tun->stats, NULL);
+	if (stats)
+		kfree_rcu(stats, rcu);
 	security_tun_dev_free_security(tun->security);
 	free_netdev(dev);
 }
@@ -1885,6 +1921,28 @@ unlock:
 	return ret;
 }
 
+static int tun_set_stats(struct tun_struct *tun, struct ifreq *ifr)
+{
+	struct tun_stats *stats, *old;
+
+	stats = kmalloc(sizeof(struct tun_stats), GFP_KERNEL);
+	if (!stats)
+		return -ENOMEM;
+
+	if (copy_from_user(&stats->link_stats,
+			   ifr->ifr_ifru.ifru_data,
+			   sizeof(struct rtnl_link_stats64))) {
+		kfree(stats);
+		return -EFAULT;
+	}
+
+	old = xchg(&tun->stats, stats);
+	if (old)
+		kfree_rcu(old, rcu);
+
+	return 0;
+}
+
 static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg, int ifreq_len)
 {
@@ -2107,6 +2165,10 @@ static long __tun_chr_ioctl(struct file
 		break;
 	}
 
+	case TUNSETSTATS:
+		ret = tun_set_stats(tun, &ifr);
+		break;
+
 	default:
 		ret = -EINVAL;
 		break;
--- a/include/uapi/linux/if_tun.h	2013-07-23 10:17:04.335800821 -0700
+++ b/include/uapi/linux/if_tun.h	2013-07-23 10:19:09.358230639 -0700
@@ -57,6 +57,7 @@
 #define TUNSETVNETHDRSZ _IOW('T', 216, int)
 #define TUNSETQUEUE  _IOW('T', 217, int)
 #define TUNSETINFO     _IOW('T', 219, struct tun_info)
+#define TUNSETSTATS    _IOW('T', 220, struct rtnl_link_stats64)
 
 /* TUNSETIFF ifr flags */
 #define IFF_TUN		0x0001

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations
  2013-07-23 13:32         ` TUN/TAP: tap driver reports bogus interface speed in ethtool operations Helmut Grohne
  2013-07-23 15:49           ` Rick Jones
  2013-07-23 17:28           ` [RFC 1/2] tun: allow overrriding ethtool info Stephen Hemminger
@ 2013-07-23 19:04           ` Ben Hutchings
  2013-07-30  6:20             ` Helmut Grohne
  2 siblings, 1 reply; 9+ messages in thread
From: Ben Hutchings @ 2013-07-23 19:04 UTC (permalink / raw)
  To: Helmut Grohne; +Cc: netdev, David Miller, Max Krasnyansky

On Tue, 2013-07-23 at 15:32 +0200, Helmut Grohne wrote:
> On 16.07.2013, at 23:41, Max Krasnyansky <maxk@qti.qualcomm.com> wrote:
> > The patch looks fine to me (must admit that I only glanced at it). Please send it to the netdev
> > mailing list netdev@vger.kernel.org, if you have't already done so.
> > CC David Miller <davem@davemloft.net> and me.
> 
> Doing that. Now summarizing the issue for the new recipients:
> 
> Problem:
> 
> When querying a tap device for its speed using ethtool the tun driver reports a speed
> of SPEED_10. This number is hard coded into the driver. Nowadays virtual network devices
> can go way faster than that. When doing automatic bandwidth monitoring based on the
> speed reported by ethtool, tap devices tend to come up as false positives. Arguably the
> hard coded speed is wrong.
> 
> Proposed solution:
> 
> To that end I propose supporting the ETHTOOL_SSET command in addition to the already
> supported ETHTOOL_GSET. It would deny setting any setting except for the bandwidth
> where it would allow arbitrary values. You can find this patch attached.

Yes, I think this makes some sense.

[...]
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
[...]
> @@ -2197,9 +2199,11 @@ static struct miscdevice tun_miscdev = {
>  
>  static int tun_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
>  {
> +       struct tun_struct *tun = netdev_priv(dev);
> +
>         cmd->supported          = 0;
>         cmd->advertising        = 0;
> -       ethtool_cmd_speed_set(cmd, SPEED_10);
> +       ethtool_cmd_speed_set(cmd, tun->advertised_speed);
>         cmd->duplex             = DUPLEX_FULL;
>         cmd->port               = PORT_TP;
>
>         cmd->phy_address        = 0;
> @@ -2210,6 +2214,24 @@ static int tun_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
>         return 0;
>  }
>  
> +static int tun_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> +       struct tun_struct *tun = netdev_priv(dev);
> +
> +       if(cmd->supported != 0 ||
> +                       cmd->advertising != 0 ||
> +                       cmd->duplex != DUPLEX_FULL ||
> +                       cmd->port != PORT_TP ||
> +                       cmd->phy_address != 0 ||
> +                       cmd->transceiver != XCVR_INTERNAL ||
> +                       cmd->autoneg != AUTONEG_DISABLE ||
> +                       cmd->maxtxpkt != 0 ||
> +                       cmd->maxrxpkt != 0)

This is formatted wrongly - needs a space after 'if' and all the
continuation lines should be lined up under 'cmd->supported'.

> +               return -EOPNOTSUPP;

EINVAL

> +       tun->advertised_speed = ethtool_cmd_speed(cmd);
> +       return 0;
> +}
> +
>  static void tun_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
>  {
>         struct tun_struct *tun = netdev_priv(dev);
[...]

I notice the port type is reported as PORT_TP.  Perhaps that should also
be changed to PORT_NONE (in a separate patch)?

Ben.

-- 
Ben Hutchings, Staff 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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations
  2013-07-23 19:04           ` TUN/TAP: tap driver reports bogus interface speed in ethtool operations Ben Hutchings
@ 2013-07-30  6:20             ` Helmut Grohne
  2013-07-30 10:51               ` Ben Hutchings
  0 siblings, 1 reply; 9+ messages in thread
From: Helmut Grohne @ 2013-07-30  6:20 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: netdev, David Miller, Max Krasnyansky, Stephen Hemminger

Thanks for your review! Given the v2 patch Stephen Hemminger posted, I
think that his series is where development should continue.

On 23.07.2013, at 21:04, Ben Hutchings <bhutchings@solarflare.com> wrote:
> This is formatted wrongly - needs a space after 'if' and all the
> continuation lines should be lined up under 'cmd->supported'.

I am sorry for not having run checkpatch and will try to remember next time.

>> +               return -EOPNOTSUPP;
> 
> EINVAL

Makes sense, once you are pointed to it.

> I notice the port type is reported as PORT_TP.  Perhaps that should also
> be changed to PORT_NONE (in a separate patch)?

This appears not yet addressed in the v2 patch yet.

I am not sure how to go on here. Clearly the v2 patch set, is an improvement
over my initial version. The additional capabilities of changing the reported
driver and similar aspects appear controversial though. Would it make sense
to split this into two patch sets to be handled independently?

Helmut

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: TUN/TAP: tap driver reports bogus interface speed in ethtool operations
  2013-07-30  6:20             ` Helmut Grohne
@ 2013-07-30 10:51               ` Ben Hutchings
  0 siblings, 0 replies; 9+ messages in thread
From: Ben Hutchings @ 2013-07-30 10:51 UTC (permalink / raw)
  To: Helmut Grohne; +Cc: netdev, David Miller, Max Krasnyansky, Stephen Hemminger

On Tue, 2013-07-30 at 08:20 +0200, Helmut Grohne wrote:
> Thanks for your review! Given the v2 patch Stephen Hemminger posted, I
> think that his series is where development should continue.
> 
> On 23.07.2013, at 21:04, Ben Hutchings <bhutchings@solarflare.com> wrote:
> > This is formatted wrongly - needs a space after 'if' and all the
> > continuation lines should be lined up under 'cmd->supported'.
> 
> I am sorry for not having run checkpatch and will try to remember next time.
> 
> >> +               return -EOPNOTSUPP;
> > 
> > EINVAL
> 
> Makes sense, once you are pointed to it.
> 
> > I notice the port type is reported as PORT_TP.  Perhaps that should also
> > be changed to PORT_NONE (in a separate patch)?
> 
> This appears not yet addressed in the v2 patch yet.
> 
> I am not sure how to go on here. Clearly the v2 patch set, is an improvement
> over my initial version. The additional capabilities of changing the reported
> driver and similar aspects appear controversial though. Would it make sense
> to split this into two patch sets to be handled independently?

Yes, I think that would make sense.

Ben.

-- 
Ben Hutchings, Staff 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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2013-07-30 10:51 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <2F174B03-5074-4BA0-B91A-8F2F62C2B082@cygnusnetworks.de>
     [not found] ` <5192BDD6.6060703@qti.qualcomm.com>
     [not found]   ` <3E916AF0-C587-443D-B653-C968A96C8751@cygnusnetworks.de>
     [not found]     ` <48A2F278-A32C-47C6-AD2D-1FC15EC73B88@cygnusnetworks.de>
     [not found]       ` <51E5BE27.3060207@qti.qualcomm.com>
2013-07-23 13:32         ` TUN/TAP: tap driver reports bogus interface speed in ethtool operations Helmut Grohne
2013-07-23 15:49           ` Rick Jones
2013-07-23 15:56             ` Stephen Hemminger
2013-07-23 16:03               ` Helmut Grohne
2013-07-23 17:28           ` [RFC 1/2] tun: allow overrriding ethtool info Stephen Hemminger
2013-07-23 17:29             ` [RFC 2/2] tun: allow overriding statistics Stephen Hemminger
2013-07-23 19:04           ` TUN/TAP: tap driver reports bogus interface speed in ethtool operations Ben Hutchings
2013-07-30  6:20             ` Helmut Grohne
2013-07-30 10:51               ` Ben Hutchings

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).