[PATCH net-next] dropwatch: Support monitoring of dropped frames

[PATCH net-next] dropwatch: Support monitoring of dropped frames

[izabela.bakollari]

From: Izabela Bakollari <izabela.bakollari@gmail.com>

Dropwatch is a utility that monitors dropped frames by having userspace
record them over the dropwatch protocol over a file. This augument
allows live monitoring of dropped frames using tools like tcpdump.

With this feature, dropwatch allows two additional commands (start and
stop interface) which allows the assignment of a net_device to the
dropwatch protocol. When assinged, dropwatch will clone dropped frames,
and receive them on the assigned interface, allowing tools like tcpdump
to monitor for them.

With this feature, create a dummy ethernet interface (ip link add dev
dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
these new commands, and then monitor dropped frames in real time by
running tcpdump -i dummy0.

Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
---
 include/uapi/linux/net_dropmon.h |  3 ++
 net/core/drop_monitor.c          | 79 +++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/net_dropmon.h b/include/uapi/linux/net_dropmon.h
index 67e31f329190..e8e861e03a8a 100644
--- a/include/uapi/linux/net_dropmon.h
+++ b/include/uapi/linux/net_dropmon.h
@@ -58,6 +58,8 @@ enum {
 	NET_DM_CMD_CONFIG_NEW,
 	NET_DM_CMD_STATS_GET,
 	NET_DM_CMD_STATS_NEW,
+	NET_DM_CMD_START_IFC,
+	NET_DM_CMD_STOP_IFC,
 	_NET_DM_CMD_MAX,
 };
 
@@ -93,6 +95,7 @@ enum net_dm_attr {
 	NET_DM_ATTR_SW_DROPS,			/* flag */
 	NET_DM_ATTR_HW_DROPS,			/* flag */
 	NET_DM_ATTR_FLOW_ACTION_COOKIE,		/* binary */
+	NET_DM_ATTR_IFNAME,			/* string */
 
 	__NET_DM_ATTR_MAX,
 	NET_DM_ATTR_MAX = __NET_DM_ATTR_MAX - 1
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 8e33cec9fc4e..8049bff05abd 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -30,6 +30,7 @@
 #include <net/genetlink.h>
 #include <net/netevent.h>
 #include <net/flow_offload.h>
+#include <net/sock.h>
 
 #include <trace/events/skb.h>
 #include <trace/events/napi.h>
@@ -46,6 +47,7 @@
  */
 static int trace_state = TRACE_OFF;
 static bool monitor_hw;
+struct net_device *interface;
 
 /* net_dm_mutex
  *
@@ -220,9 +222,8 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
 	struct per_cpu_dm_data *data;
 	unsigned long flags;
 
-	local_irq_save(flags);
+	spin_lock_irqsave(&data->lock, flags);
 	data = this_cpu_ptr(&dm_cpu_data);
-	spin_lock(&data->lock);
 	dskb = data->skb;
 
 	if (!dskb)
@@ -255,6 +256,12 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
 
 out:
 	spin_unlock_irqrestore(&data->lock, flags);
+
+	if (interface && interface != skb->dev) {
+		skb = skb_clone(skb, GFP_ATOMIC);
+		skb->dev = interface;
+		netif_receive_skb(skb);
+	}
 }
 
 static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
@@ -1315,6 +1322,63 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
 	return -EOPNOTSUPP;
 }
 
+static int net_dm_interface_start(struct net *net, const char *ifname)
+{
+	struct net_device *nd;
+
+	nd = dev_get_by_name(net, ifname);
+
+	if (nd) {
+		interface = nd;
+		dev_hold(interface);
+	} else {
+		return -ENODEV;
+	}
+	return 0;
+}
+
+static int net_dm_interface_stop(struct net *net, const char *ifname)
+{
+	struct net_device *nd;
+
+	nd = dev_get_by_name(net, ifname);
+
+	if (nd) {
+		interface = nd;
+		dev_put(interface);
+	} else {
+		return -ENODEV;
+	}
+	return 0;
+}
+
+static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	char ifname[IFNAMSIZ];
+	int rc;
+
+	memset(ifname, 0, IFNAMSIZ);
+	nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
+
+	switch (info->genlhdr->cmd) {
+	case NET_DM_CMD_START_IFC:
+		rc = net_dm_interface_start(net, ifname);
+		if (rc)
+			return rc;
+		break;
+	case NET_DM_CMD_STOP_IFC:
+		if (interface) {
+			rc = net_dm_interface_stop(net, interface->ifname);
+			return rc;
+		} else {
+			return -ENODEV;
+		}
+	}
+
+	return 0;
+}
+
 static int net_dm_config_fill(struct sk_buff *msg, struct genl_info *info)
 {
 	void *hdr;
@@ -1543,6 +1607,7 @@ static const struct nla_policy net_dm_nl_policy[NET_DM_ATTR_MAX + 1] = {
 	[NET_DM_ATTR_QUEUE_LEN] = { .type = NLA_U32 },
 	[NET_DM_ATTR_SW_DROPS]	= {. type = NLA_FLAG },
 	[NET_DM_ATTR_HW_DROPS]	= {. type = NLA_FLAG },
+	[NET_DM_ATTR_IFNAME] = {. type = NLA_STRING, .len = IFNAMSIZ },
 };
 
 static const struct genl_ops dropmon_ops[] = {
@@ -1570,6 +1635,16 @@ static const struct genl_ops dropmon_ops[] = {
 		.cmd = NET_DM_CMD_STATS_GET,
 		.doit = net_dm_cmd_stats_get,
 	},
+	{
+		.cmd = NET_DM_CMD_START_IFC,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = net_dm_cmd_ifc_trace,
+	},
+	{
+		.cmd = NET_DM_CMD_STOP_IFC,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = net_dm_cmd_ifc_trace,
+	},
 };
 
 static int net_dm_nl_pre_doit(const struct genl_ops *ops,
-- 
2.18.4

Re: [PATCH net-next] dropwatch: Support monitoring of dropped frames

[Eric Dumazet]

On 7/7/20 10:15 AM, izabela.bakollari@gmail.com wrote:
> From: Izabela Bakollari <izabela.bakollari@gmail.com>
> 
> Dropwatch is a utility that monitors dropped frames by having userspace
> record them over the dropwatch protocol over a file. This augument
> allows live monitoring of dropped frames using tools like tcpdump.
> 
> With this feature, dropwatch allows two additional commands (start and
> stop interface) which allows the assignment of a net_device to the
> dropwatch protocol. When assinged, dropwatch will clone dropped frames,
> and receive them on the assigned interface, allowing tools like tcpdump
> to monitor for them.
> 
> With this feature, create a dummy ethernet interface (ip link add dev
> dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
> these new commands, and then monitor dropped frames in real time by
> running tcpdump -i dummy0.
> 
> Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
> ---
>  include/uapi/linux/net_dropmon.h |  3 ++
>  net/core/drop_monitor.c          | 79 +++++++++++++++++++++++++++++++-
>  2 files changed, 80 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/net_dropmon.h b/include/uapi/linux/net_dropmon.h
> index 67e31f329190..e8e861e03a8a 100644
> --- a/include/uapi/linux/net_dropmon.h
> +++ b/include/uapi/linux/net_dropmon.h
> @@ -58,6 +58,8 @@ enum {
>  	NET_DM_CMD_CONFIG_NEW,
>  	NET_DM_CMD_STATS_GET,
>  	NET_DM_CMD_STATS_NEW,
> +	NET_DM_CMD_START_IFC,
> +	NET_DM_CMD_STOP_IFC,
>  	_NET_DM_CMD_MAX,
>  };
>  
> @@ -93,6 +95,7 @@ enum net_dm_attr {
>  	NET_DM_ATTR_SW_DROPS,			/* flag */
>  	NET_DM_ATTR_HW_DROPS,			/* flag */
>  	NET_DM_ATTR_FLOW_ACTION_COOKIE,		/* binary */
> +	NET_DM_ATTR_IFNAME,			/* string */
>  
>  	__NET_DM_ATTR_MAX,
>  	NET_DM_ATTR_MAX = __NET_DM_ATTR_MAX - 1
> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index 8e33cec9fc4e..8049bff05abd 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -30,6 +30,7 @@
>  #include <net/genetlink.h>
>  #include <net/netevent.h>
>  #include <net/flow_offload.h>
> +#include <net/sock.h>
>  
>  #include <trace/events/skb.h>
>  #include <trace/events/napi.h>
> @@ -46,6 +47,7 @@
>   */
>  static int trace_state = TRACE_OFF;
>  static bool monitor_hw;
> +struct net_device *interface;
>  
>  /* net_dm_mutex
>   *
> @@ -220,9 +222,8 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
>  	struct per_cpu_dm_data *data;
>  	unsigned long flags;
>  
> -	local_irq_save(flags);
> +	spin_lock_irqsave(&data->lock, flags);
>  	data = this_cpu_ptr(&dm_cpu_data);
> -	spin_lock(&data->lock);

This change seems unrelated ?

And also buggy, since data is essentially garbage when you call spin_lock_irqsave(&data->lock, flags);

>  	dskb = data->skb;
>  
>  	if (!dskb)
> @@ -255,6 +256,12 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
>  
>  out:
>  	spin_unlock_irqrestore(&data->lock, flags);
> +
> +	if (interface && interface != skb->dev) {
> +		skb = skb_clone(skb, GFP_ATOMIC);

skb_clone() can return NULL

> +		skb->dev = interface;
> +		netif_receive_skb(skb);
> +	}
>  }
>  
>  static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
> @@ -1315,6 +1322,63 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
>  	return -EOPNOTSUPP;
>  }
>  
> +static int net_dm_interface_start(struct net *net, const char *ifname)
> +{
> +	struct net_device *nd;
> +
> +	nd = dev_get_by_name(net, ifname);
> +
> +	if (nd) {
> +		interface = nd;
> +		dev_hold(interface);
> +	} else {
> +		return -ENODEV;
> +	}
> +	return 0;
> +}
> +


What happens after this monitoring is started, then the admin does :

rmmod ifb

Re: [PATCH net-next] dropwatch: Support monitoring of dropped frames

[Eric Dumazet]

On 7/7/20 10:33 AM, Eric Dumazet wrote:
> 
> 
> What happens after this monitoring is started, then the admin does :
> 
> rmmod ifb
> 

I meant  :  rmmod dummy

Re: [PATCH net-next] dropwatch: Support monitoring of dropped frames

[Eric Dumazet]

On 7/7/20 10:15 AM, izabela.bakollari@gmail.com wrote:
> From: Izabela Bakollari <izabela.bakollari@gmail.com>
> 
> Dropwatch is a utility that monitors dropped frames by having userspace
> record them over the dropwatch protocol over a file. This augument
> allows live monitoring of dropped frames using tools like tcpdump.
> 
> With this feature, dropwatch allows two additional commands (start and
> stop interface) which allows the assignment of a net_device to the
> dropwatch protocol. When assinged, dropwatch will clone dropped frames,
> and receive them on the assigned interface, allowing tools like tcpdump
> to monitor for them.
> 
> With this feature, create a dummy ethernet interface (ip link add dev
> dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
> these new commands, and then monitor dropped frames in real time by
> running tcpdump -i dummy0.
> 
> Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
> ---
>  include/uapi/linux/net_dropmon.h |  3 ++
>  net/core/drop_monitor.c          | 79 +++++++++++++++++++++++++++++++-
>  2 files changed, 80 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/net_dropmon.h b/include/uapi/linux/net_dropmon.h
> index 67e31f329190..e8e861e03a8a 100644
> --- a/include/uapi/linux/net_dropmon.h
> +++ b/include/uapi/linux/net_dropmon.h
> @@ -58,6 +58,8 @@ enum {
>  	NET_DM_CMD_CONFIG_NEW,
>  	NET_DM_CMD_STATS_GET,
>  	NET_DM_CMD_STATS_NEW,
> +	NET_DM_CMD_START_IFC,
> +	NET_DM_CMD_STOP_IFC,
>  	_NET_DM_CMD_MAX,
>  };
>  
> @@ -93,6 +95,7 @@ enum net_dm_attr {
>  	NET_DM_ATTR_SW_DROPS,			/* flag */
>  	NET_DM_ATTR_HW_DROPS,			/* flag */
>  	NET_DM_ATTR_FLOW_ACTION_COOKIE,		/* binary */
> +	NET_DM_ATTR_IFNAME,			/* string */
>  
>  	__NET_DM_ATTR_MAX,
>  	NET_DM_ATTR_MAX = __NET_DM_ATTR_MAX - 1
> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index 8e33cec9fc4e..8049bff05abd 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -30,6 +30,7 @@
>  #include <net/genetlink.h>
>  #include <net/netevent.h>
>  #include <net/flow_offload.h>
> +#include <net/sock.h>
>  
>  #include <trace/events/skb.h>
>  #include <trace/events/napi.h>
> @@ -46,6 +47,7 @@
>   */
>  static int trace_state = TRACE_OFF;
>  static bool monitor_hw;
> +struct net_device *interface;
>  
>  /* net_dm_mutex
>   *
> @@ -220,9 +222,8 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
>  	struct per_cpu_dm_data *data;
>  	unsigned long flags;
>  
> -	local_irq_save(flags);
> +	spin_lock_irqsave(&data->lock, flags);
>  	data = this_cpu_ptr(&dm_cpu_data);
> -	spin_lock(&data->lock);
>  	dskb = data->skb;
>  
>  	if (!dskb)
> @@ -255,6 +256,12 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
>  
>  out:
>  	spin_unlock_irqrestore(&data->lock, flags);
> +

What protects interface from being changed under us by another thread/cpu ?

> +	if (interface && interface != skb->dev) {
> +		skb = skb_clone(skb, GFP_ATOMIC);
> +		skb->dev = interface;
> +		netif_receive_skb(skb);
> +	}
>  }
>  
>  static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
> @@ -1315,6 +1322,63 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
>  	return -EOPNOTSUPP;
>  }
>  
> +static int net_dm_interface_start(struct net *net, const char *ifname)
> +{
> +	struct net_device *nd;
> +
> +	nd = dev_get_by_name(net, ifname);
> +
> +	if (nd) {
> +		interface = nd;

If interface was already set, you forgot to dev_put() it.

> +		dev_hold(interface);

Note that dev_get_by_name() already did a dev_hold()

> +	} else {
> +		return -ENODEV;
> +	}
> +	return 0;
> +}
> +
> +static int net_dm_interface_stop(struct net *net, const char *ifname)
> +{
> +	struct net_device *nd;
> +
> +	nd = dev_get_by_name(net, ifname);
> +
> +	if (nd) {



> +		interface = nd;


You probably meant : interface = NULL; ?

> +		dev_put(interface);

		and dev_put(nd);


> +	} else {
> +		return -ENODEV;
> +	}
> +	return 0;
> +}
> +
> +static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct net *net = sock_net(skb->sk);
> +	char ifname[IFNAMSIZ];
> +	int rc;
> +
> +	memset(ifname, 0, IFNAMSIZ);
> +	nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
> +
> +	switch (info->genlhdr->cmd) {
> +	case NET_DM_CMD_START_IFC:
> +		rc = net_dm_interface_start(net, ifname);
> +		if (rc)
> +			return rc;
> +		break;
> +	case NET_DM_CMD_STOP_IFC:
> +		if (interface) {
> +			rc = net_dm_interface_stop(net, interface->ifname);
> +			return rc;
> +		} else {
> +			return -ENODEV;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int net_dm_config_fill(struct sk_buff *msg, struct genl_info *info)
>  {
>  	void *hdr;
> @@ -1543,6 +1607,7 @@ static const struct nla_policy net_dm_nl_policy[NET_DM_ATTR_MAX + 1] = {
>  	[NET_DM_ATTR_QUEUE_LEN] = { .type = NLA_U32 },
>  	[NET_DM_ATTR_SW_DROPS]	= {. type = NLA_FLAG },
>  	[NET_DM_ATTR_HW_DROPS]	= {. type = NLA_FLAG },
> +	[NET_DM_ATTR_IFNAME] = {. type = NLA_STRING, .len = IFNAMSIZ },
>  };
>  
>  static const struct genl_ops dropmon_ops[] = {
> @@ -1570,6 +1635,16 @@ static const struct genl_ops dropmon_ops[] = {
>  		.cmd = NET_DM_CMD_STATS_GET,
>  		.doit = net_dm_cmd_stats_get,
>  	},
> +	{
> +		.cmd = NET_DM_CMD_START_IFC,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = net_dm_cmd_ifc_trace,
> +	},
> +	{
> +		.cmd = NET_DM_CMD_STOP_IFC,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = net_dm_cmd_ifc_trace,
> +	},
>  };
>  
>  static int net_dm_nl_pre_doit(const struct genl_ops *ops,
> 

Re: [PATCH net-next] dropwatch: Support monitoring of dropped frames

[kernel test robot]

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

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/izabela-bakollari-gmail-com/dropwatch-Support-monitoring-of-dropped-frames/20200708-011806
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git d47a72152097d7be7cfc453d205196c0aa976c33
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   net/core/drop_monitor.c: In function 'net_dm_cmd_ifc_trace':
>> net/core/drop_monitor.c:1375:47: error: 'struct net_device' has no member named 'ifname'; did you mean 'name'?
    1375 |    rc = net_dm_interface_stop(net, interface->ifname);
         |                                               ^~~~~~
         |                                               name

vim +1375 net/core/drop_monitor.c

  1357	
  1358	static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
  1359	{
  1360		struct net *net = sock_net(skb->sk);
  1361		char ifname[IFNAMSIZ];
  1362		int rc;
  1363	
  1364		memset(ifname, 0, IFNAMSIZ);
  1365		nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
  1366	
  1367		switch (info->genlhdr->cmd) {
  1368		case NET_DM_CMD_START_IFC:
  1369			rc = net_dm_interface_start(net, ifname);
  1370			if (rc)
  1371				return rc;
  1372			break;
  1373		case NET_DM_CMD_STOP_IFC:
  1374			if (interface) {
> 1375				rc = net_dm_interface_stop(net, interface->ifname);
  1376				return rc;
  1377			} else {
  1378				return -ENODEV;
  1379			}
  1380		}
  1381	
  1382		return 0;
  1383	}
  1384	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 74063 bytes --]

Re: [PATCH net-next] dropwatch: Support monitoring of dropped frames

[kernel test robot]

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

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/izabela-bakollari-gmail-com/dropwatch-Support-monitoring-of-dropped-frames/20200708-011806
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git d47a72152097d7be7cfc453d205196c0aa976c33
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 02946de3802d3bc65bc9f2eb9b8d4969b5a7add8)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> net/core/drop_monitor.c:226:21: warning: variable 'data' is uninitialized when used here [-Wuninitialized]
           spin_lock_irqsave(&data->lock, flags);
                              ^~~~
   include/linux/spinlock.h:383:39: note: expanded from macro 'spin_lock_irqsave'
           raw_spin_lock_irqsave(spinlock_check(lock), flags);     \
                                                ^~~~
   include/linux/spinlock.h:251:34: note: expanded from macro 'raw_spin_lock_irqsave'
                   flags = _raw_spin_lock_irqsave(lock);   \
                                                  ^~~~
   net/core/drop_monitor.c:223:30: note: initialize the variable 'data' to silence this warning
           struct per_cpu_dm_data *data;
                                       ^
                                        = NULL
>> net/core/drop_monitor.c:1375:47: error: no member named 'ifname' in 'struct net_device'; did you mean 'name'?
                           rc = net_dm_interface_stop(net, interface->ifname);
                                                                      ^~~~~~
                                                                      name
   include/linux/netdevice.h:1844:9: note: 'name' declared here
           char                    name[IFNAMSIZ];
                                   ^
   1 warning and 1 error generated.

vim +1375 net/core/drop_monitor.c

  1357	
  1358	static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
  1359	{
  1360		struct net *net = sock_net(skb->sk);
  1361		char ifname[IFNAMSIZ];
  1362		int rc;
  1363	
  1364		memset(ifname, 0, IFNAMSIZ);
  1365		nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
  1366	
  1367		switch (info->genlhdr->cmd) {
  1368		case NET_DM_CMD_START_IFC:
  1369			rc = net_dm_interface_start(net, ifname);
  1370			if (rc)
  1371				return rc;
  1372			break;
  1373		case NET_DM_CMD_STOP_IFC:
  1374			if (interface) {
> 1375				rc = net_dm_interface_stop(net, interface->ifname);
  1376				return rc;
  1377			} else {
  1378				return -ENODEV;
  1379			}
  1380		}
  1381	
  1382		return 0;
  1383	}
  1384	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75310 bytes --]

Re: [PATCH net-next] dropwatch: Support monitoring of dropped frames

[Izabela Bakollari]

Hi Eric,

Thank you for reviewing my patch. I understand your comments
and will be working on correcting what you pointed out.

Best,
Izabela


On Tue, Jul 7, 2020 at 7:52 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 7/7/20 10:15 AM, izabela.bakollari@gmail.com wrote:
> > From: Izabela Bakollari <izabela.bakollari@gmail.com>
> >
> > Dropwatch is a utility that monitors dropped frames by having userspace
> > record them over the dropwatch protocol over a file. This augument
> > allows live monitoring of dropped frames using tools like tcpdump.
> >
> > With this feature, dropwatch allows two additional commands (start and
> > stop interface) which allows the assignment of a net_device to the
> > dropwatch protocol. When assinged, dropwatch will clone dropped frames,
> > and receive them on the assigned interface, allowing tools like tcpdump
> > to monitor for them.
> >
> > With this feature, create a dummy ethernet interface (ip link add dev
> > dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
> > these new commands, and then monitor dropped frames in real time by
> > running tcpdump -i dummy0.
> >
> > Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
> > ---
> >  include/uapi/linux/net_dropmon.h |  3 ++
> >  net/core/drop_monitor.c          | 79 +++++++++++++++++++++++++++++++-
> >  2 files changed, 80 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/net_dropmon.h b/include/uapi/linux/net_dropmon.h
> > index 67e31f329190..e8e861e03a8a 100644
> > --- a/include/uapi/linux/net_dropmon.h
> > +++ b/include/uapi/linux/net_dropmon.h
> > @@ -58,6 +58,8 @@ enum {
> >       NET_DM_CMD_CONFIG_NEW,
> >       NET_DM_CMD_STATS_GET,
> >       NET_DM_CMD_STATS_NEW,
> > +     NET_DM_CMD_START_IFC,
> > +     NET_DM_CMD_STOP_IFC,
> >       _NET_DM_CMD_MAX,
> >  };
> >
> > @@ -93,6 +95,7 @@ enum net_dm_attr {
> >       NET_DM_ATTR_SW_DROPS,                   /* flag */
> >       NET_DM_ATTR_HW_DROPS,                   /* flag */
> >       NET_DM_ATTR_FLOW_ACTION_COOKIE,         /* binary */
> > +     NET_DM_ATTR_IFNAME,                     /* string */
> >
> >       __NET_DM_ATTR_MAX,
> >       NET_DM_ATTR_MAX = __NET_DM_ATTR_MAX - 1
> > diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> > index 8e33cec9fc4e..8049bff05abd 100644
> > --- a/net/core/drop_monitor.c
> > +++ b/net/core/drop_monitor.c
> > @@ -30,6 +30,7 @@
> >  #include <net/genetlink.h>
> >  #include <net/netevent.h>
> >  #include <net/flow_offload.h>
> > +#include <net/sock.h>
> >
> >  #include <trace/events/skb.h>
> >  #include <trace/events/napi.h>
> > @@ -46,6 +47,7 @@
> >   */
> >  static int trace_state = TRACE_OFF;
> >  static bool monitor_hw;
> > +struct net_device *interface;
> >
> >  /* net_dm_mutex
> >   *
> > @@ -220,9 +222,8 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
> >       struct per_cpu_dm_data *data;
> >       unsigned long flags;
> >
> > -     local_irq_save(flags);
> > +     spin_lock_irqsave(&data->lock, flags);
> >       data = this_cpu_ptr(&dm_cpu_data);
> > -     spin_lock(&data->lock);
> >       dskb = data->skb;
> >
> >       if (!dskb)
> > @@ -255,6 +256,12 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
> >
> >  out:
> >       spin_unlock_irqrestore(&data->lock, flags);
> > +
>
> What protects interface from being changed under us by another thread/cpu ?
>
> > +     if (interface && interface != skb->dev) {
> > +             skb = skb_clone(skb, GFP_ATOMIC);
> > +             skb->dev = interface;
> > +             netif_receive_skb(skb);
> > +     }
> >  }
> >
> >  static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
> > @@ -1315,6 +1322,63 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
> >       return -EOPNOTSUPP;
> >  }
> >
> > +static int net_dm_interface_start(struct net *net, const char *ifname)
> > +{
> > +     struct net_device *nd;
> > +
> > +     nd = dev_get_by_name(net, ifname);
> > +
> > +     if (nd) {
> > +             interface = nd;
>
> If interface was already set, you forgot to dev_put() it.
>
> > +             dev_hold(interface);
>
> Note that dev_get_by_name() already did a dev_hold()
>
> > +     } else {
> > +             return -ENODEV;
> > +     }
> > +     return 0;
> > +}
> > +
> > +static int net_dm_interface_stop(struct net *net, const char *ifname)
> > +{
> > +     struct net_device *nd;
> > +
> > +     nd = dev_get_by_name(net, ifname);
> > +
> > +     if (nd) {
>
>
>
> > +             interface = nd;
>
>
> You probably meant : interface = NULL; ?
>
> > +             dev_put(interface);
>
>                 and dev_put(nd);
>
>
> > +     } else {
> > +             return -ENODEV;
> > +     }
> > +     return 0;
> > +}
> > +
> > +static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +     struct net *net = sock_net(skb->sk);
> > +     char ifname[IFNAMSIZ];
> > +     int rc;
> > +
> > +     memset(ifname, 0, IFNAMSIZ);
> > +     nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
> > +
> > +     switch (info->genlhdr->cmd) {
> > +     case NET_DM_CMD_START_IFC:
> > +             rc = net_dm_interface_start(net, ifname);
> > +             if (rc)
> > +                     return rc;
> > +             break;
> > +     case NET_DM_CMD_STOP_IFC:
> > +             if (interface) {
> > +                     rc = net_dm_interface_stop(net, interface->ifname);
> > +                     return rc;
> > +             } else {
> > +                     return -ENODEV;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static int net_dm_config_fill(struct sk_buff *msg, struct genl_info *info)
> >  {
> >       void *hdr;
> > @@ -1543,6 +1607,7 @@ static const struct nla_policy net_dm_nl_policy[NET_DM_ATTR_MAX + 1] = {
> >       [NET_DM_ATTR_QUEUE_LEN] = { .type = NLA_U32 },
> >       [NET_DM_ATTR_SW_DROPS]  = {. type = NLA_FLAG },
> >       [NET_DM_ATTR_HW_DROPS]  = {. type = NLA_FLAG },
> > +     [NET_DM_ATTR_IFNAME] = {. type = NLA_STRING, .len = IFNAMSIZ },
> >  };
> >
> >  static const struct genl_ops dropmon_ops[] = {
> > @@ -1570,6 +1635,16 @@ static const struct genl_ops dropmon_ops[] = {
> >               .cmd = NET_DM_CMD_STATS_GET,
> >               .doit = net_dm_cmd_stats_get,
> >       },
> > +     {
> > +             .cmd = NET_DM_CMD_START_IFC,
> > +             .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> > +             .doit = net_dm_cmd_ifc_trace,
> > +     },
> > +     {
> > +             .cmd = NET_DM_CMD_STOP_IFC,
> > +             .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> > +             .doit = net_dm_cmd_ifc_trace,
> > +     },
> >  };
> >
> >  static int net_dm_nl_pre_doit(const struct genl_ops *ops,
> >

[PATCHv2 net-next] dropwatch: Support monitoring of dropped frames

[izabela.bakollari]

From: Izabela Bakollari <izabela.bakollari@gmail.com>

Dropwatch is a utility that monitors dropped frames by having userspace
record them over the dropwatch protocol over a file. This augument
allows live monitoring of dropped frames using tools like tcpdump.

With this feature, dropwatch allows two additional commands (start and
stop interface) which allows the assignment of a net_device to the
dropwatch protocol. When assinged, dropwatch will clone dropped frames,
and receive them on the assigned interface, allowing tools like tcpdump
to monitor for them.

With this feature, create a dummy ethernet interface (ip link add dev
dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
these new commands, and then monitor dropped frames in real time by
running tcpdump -i dummy0.

Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
---
Changes in v2:
- protect the dummy ethernet interface from being changed by another
thread/cpu
---
 include/uapi/linux/net_dropmon.h |  3 ++
 net/core/drop_monitor.c          | 84 ++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/include/uapi/linux/net_dropmon.h b/include/uapi/linux/net_dropmon.h
index 67e31f329190..e8e861e03a8a 100644
--- a/include/uapi/linux/net_dropmon.h
+++ b/include/uapi/linux/net_dropmon.h
@@ -58,6 +58,8 @@ enum {
 	NET_DM_CMD_CONFIG_NEW,
 	NET_DM_CMD_STATS_GET,
 	NET_DM_CMD_STATS_NEW,
+	NET_DM_CMD_START_IFC,
+	NET_DM_CMD_STOP_IFC,
 	_NET_DM_CMD_MAX,
 };
 
@@ -93,6 +95,7 @@ enum net_dm_attr {
 	NET_DM_ATTR_SW_DROPS,			/* flag */
 	NET_DM_ATTR_HW_DROPS,			/* flag */
 	NET_DM_ATTR_FLOW_ACTION_COOKIE,		/* binary */
+	NET_DM_ATTR_IFNAME,			/* string */
 
 	__NET_DM_ATTR_MAX,
 	NET_DM_ATTR_MAX = __NET_DM_ATTR_MAX - 1
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 8e33cec9fc4e..781e69876d2f 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -30,6 +30,7 @@
 #include <net/genetlink.h>
 #include <net/netevent.h>
 #include <net/flow_offload.h>
+#include <net/sock.h>
 
 #include <trace/events/skb.h>
 #include <trace/events/napi.h>
@@ -46,6 +47,7 @@
  */
 static int trace_state = TRACE_OFF;
 static bool monitor_hw;
+struct net_device *interface;
 
 /* net_dm_mutex
  *
@@ -54,6 +56,8 @@ static bool monitor_hw;
  */
 static DEFINE_MUTEX(net_dm_mutex);
 
+static DEFINE_SPINLOCK(interface_lock);
+
 struct net_dm_stats {
 	u64 dropped;
 	struct u64_stats_sync syncp;
@@ -255,6 +259,21 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
 
 out:
 	spin_unlock_irqrestore(&data->lock, flags);
+	spin_lock_irqsave(&interface_lock, flags);
+	if (interface && interface != skb->dev) {
+		skb = skb_clone(skb, GFP_ATOMIC);
+		if (skb) {
+			skb->dev = interface;
+			spin_unlock_irqrestore(&interface_lock, flags);
+			netif_receive_skb(skb);
+		} else {
+			spin_unlock_irqrestore(&interface_lock, flags);
+			pr_err("dropwatch: Not enough memory to clone dropped skb\n");
+			return;
+		}
+	} else {
+		spin_unlock_irqrestore(&interface_lock, flags);
+	}
 }
 
 static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
@@ -1315,6 +1334,53 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
 	return -EOPNOTSUPP;
 }
 
+static int net_dm_interface_start(struct net *net, const char *ifname)
+{
+	struct net_device *nd = dev_get_by_name(net, ifname);
+
+	if (nd)
+		interface = nd;
+	else
+		return -ENODEV;
+
+	return 0;
+}
+
+static int net_dm_interface_stop(struct net *net, const char *ifname)
+{
+	dev_put(interface);
+	interface = NULL;
+
+	return 0;
+}
+
+static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	char ifname[IFNAMSIZ];
+
+	if (net_dm_is_monitoring())
+		return -EBUSY;
+
+	memset(ifname, 0, IFNAMSIZ);
+	nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
+
+	switch (info->genlhdr->cmd) {
+	case NET_DM_CMD_START_IFC:
+		if (!interface)
+			return net_dm_interface_start(net, ifname);
+		else
+			return -EBUSY;
+	case NET_DM_CMD_STOP_IFC:
+		if (interface)
+			return net_dm_interface_stop(net, interface->name);
+		else
+			return -ENODEV;
+	}
+
+	return 0;
+}
+
 static int net_dm_config_fill(struct sk_buff *msg, struct genl_info *info)
 {
 	void *hdr;
@@ -1503,6 +1569,7 @@ static int dropmon_net_event(struct notifier_block *ev_block,
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct dm_hw_stat_delta *new_stat = NULL;
 	struct dm_hw_stat_delta *tmp;
+	unsigned long flags;
 
 	switch (event) {
 	case NETDEV_REGISTER:
@@ -1529,6 +1596,12 @@ static int dropmon_net_event(struct notifier_block *ev_block,
 				}
 			}
 		}
+		spin_lock_irqsave(&interface_lock, flags);
+		if (interface && interface == dev) {
+			dev_put(interface);
+			interface = NULL;
+		}
+		spin_unlock_irqrestore(&interface_lock, flags);
 		mutex_unlock(&net_dm_mutex);
 		break;
 	}
@@ -1543,6 +1616,7 @@ static const struct nla_policy net_dm_nl_policy[NET_DM_ATTR_MAX + 1] = {
 	[NET_DM_ATTR_QUEUE_LEN] = { .type = NLA_U32 },
 	[NET_DM_ATTR_SW_DROPS]	= {. type = NLA_FLAG },
 	[NET_DM_ATTR_HW_DROPS]	= {. type = NLA_FLAG },
+	[NET_DM_ATTR_IFNAME] = {. type = NLA_STRING, .len = IFNAMSIZ },
 };
 
 static const struct genl_ops dropmon_ops[] = {
@@ -1570,6 +1644,16 @@ static const struct genl_ops dropmon_ops[] = {
 		.cmd = NET_DM_CMD_STATS_GET,
 		.doit = net_dm_cmd_stats_get,
 	},
+	{
+		.cmd = NET_DM_CMD_START_IFC,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = net_dm_cmd_ifc_trace,
+	},
+	{
+		.cmd = NET_DM_CMD_STOP_IFC,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = net_dm_cmd_ifc_trace,
+	},
 };
 
 static int net_dm_nl_pre_doit(const struct genl_ops *ops,
-- 
2.18.4

Re: [PATCHv2 net-next] dropwatch: Support monitoring of dropped frames

[Cong Wang]

On Tue, Aug 4, 2020 at 9:14 AM <izabela.bakollari@gmail.com> wrote:
>
> From: Izabela Bakollari <izabela.bakollari@gmail.com>
>
> Dropwatch is a utility that monitors dropped frames by having userspace
> record them over the dropwatch protocol over a file. This augument
> allows live monitoring of dropped frames using tools like tcpdump.
>
> With this feature, dropwatch allows two additional commands (start and
> stop interface) which allows the assignment of a net_device to the
> dropwatch protocol. When assinged, dropwatch will clone dropped frames,
> and receive them on the assigned interface, allowing tools like tcpdump
> to monitor for them.
>
> With this feature, create a dummy ethernet interface (ip link add dev
> dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
> these new commands, and then monitor dropped frames in real time by
> running tcpdump -i dummy0.

drop monitor is already able to send dropped packets to user-space,
and wireshark already catches up with this feature:

https://code.wireshark.org/review/gitweb?p=wireshark.git;a=commitdiff;h=a94a860c0644ec3b8a129fd243674a2e376ce1c8

So what you propose here seems pretty much a duplicate?

Thanks.

Re: [Linux-kernel-mentees] [PATCHv2 net-next] dropwatch: Support monitoring of dropped frames

[Neil Horman]

On Tue, Aug 04, 2020 at 02:28:28PM -0700, Cong Wang wrote:
> On Tue, Aug 4, 2020 at 9:14 AM <izabela.bakollari@gmail.com> wrote:
> >
> > From: Izabela Bakollari <izabela.bakollari@gmail.com>
> >
> > Dropwatch is a utility that monitors dropped frames by having userspace
> > record them over the dropwatch protocol over a file. This augument
> > allows live monitoring of dropped frames using tools like tcpdump.
> >
> > With this feature, dropwatch allows two additional commands (start and
> > stop interface) which allows the assignment of a net_device to the
> > dropwatch protocol. When assinged, dropwatch will clone dropped frames,
> > and receive them on the assigned interface, allowing tools like tcpdump
> > to monitor for them.
> >
> > With this feature, create a dummy ethernet interface (ip link add dev
> > dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
> > these new commands, and then monitor dropped frames in real time by
> > running tcpdump -i dummy0.
> 
> drop monitor is already able to send dropped packets to user-space,
> and wireshark already catches up with this feature:
> 
> https://code.wireshark.org/review/gitweb?p=wireshark.git;a=commitdiff;h=a94a860c0644ec3b8a129fd243674a2e376ce1c8
> 
> So what you propose here seems pretty much a duplicate?
> 
I had asked Izabela to implement this feature as an alternative approach to
doing live capture of dropped packets, as part of the Linux foundation
mentorship program.  I'm supportive of this additional feature as the added code
is fairly minimal, and allows for the use of other user space packet monitoring
tools without additional code changes (i.e. tcpdump/snort/etc can now monitor
dropped packets without the need to augment those tools with netlink capture
code.

Best
Neil 
> Thanks.
> _______________________________________________
> Linux-kernel-mentees mailing list
> Linux-kernel-mentees@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
> 

Re: [PATCHv2 net-next] dropwatch: Support monitoring of dropped frames

[David Miller]

From: izabela.bakollari@gmail.com
Date: Tue,  4 Aug 2020 18:09:08 +0200

> @@ -1315,6 +1334,53 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
>  	return -EOPNOTSUPP;
>  }
>  
> +static int net_dm_interface_start(struct net *net, const char *ifname)
> +{
> +	struct net_device *nd = dev_get_by_name(net, ifname);
> +
> +	if (nd)
> +		interface = nd;
> +	else
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int net_dm_interface_stop(struct net *net, const char *ifname)
> +{
> +	dev_put(interface);
> +	interface = NULL;
> +
> +	return 0;
> +}

Where is the netdev notifier that will drop this reference if the network
device is unregistered?

Re: [PATCHv2 net-next] dropwatch: Support monitoring of dropped frames

[Neil Horman]

On Tue, Aug 04, 2020 at 04:14:14PM -0700, David Miller wrote:
> From: izabela.bakollari@gmail.com
> Date: Tue,  4 Aug 2020 18:09:08 +0200
> 
> > @@ -1315,6 +1334,53 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
> >  	return -EOPNOTSUPP;
> >  }
> >  
> > +static int net_dm_interface_start(struct net *net, const char *ifname)
> > +{
> > +	struct net_device *nd = dev_get_by_name(net, ifname);
> > +
> > +	if (nd)
> > +		interface = nd;
> > +	else
> > +		return -ENODEV;
> > +
> > +	return 0;
> > +}
> > +
> > +static int net_dm_interface_stop(struct net *net, const char *ifname)
> > +{
> > +	dev_put(interface);
> > +	interface = NULL;
> > +
> > +	return 0;
> > +}
> 
> Where is the netdev notifier that will drop this reference if the network
> device is unregistered?
> 
See the changes to dropmon_net_event in the patch.  Its there under the case for
NETDEV_UNREGISTER

Neil

Re: [PATCHv2 net-next] dropwatch: Support monitoring of dropped frames

[Izabela Bakollari]

I have worked on this feature as part of the Linux Kernel Mentorship
Program. Your review would really help me in this learning process.

Thanks,
Izabela

On Tue, Aug 4, 2020 at 6:09 PM <izabela.bakollari@gmail.com> wrote:
>
> From: Izabela Bakollari <izabela.bakollari@gmail.com>
>
> Dropwatch is a utility that monitors dropped frames by having userspace
> record them over the dropwatch protocol over a file. This augument
> allows live monitoring of dropped frames using tools like tcpdump.
>
> With this feature, dropwatch allows two additional commands (start and
> stop interface) which allows the assignment of a net_device to the
> dropwatch protocol. When assinged, dropwatch will clone dropped frames,
> and receive them on the assigned interface, allowing tools like tcpdump
> to monitor for them.
>
> With this feature, create a dummy ethernet interface (ip link add dev
> dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
> these new commands, and then monitor dropped frames in real time by
> running tcpdump -i dummy0.
>
> Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
> ---
> Changes in v2:
> - protect the dummy ethernet interface from being changed by another
> thread/cpu
> ---
>  include/uapi/linux/net_dropmon.h |  3 ++
>  net/core/drop_monitor.c          | 84 ++++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+)
>
> diff --git a/include/uapi/linux/net_dropmon.h b/include/uapi/linux/net_dropmon.h
> index 67e31f329190..e8e861e03a8a 100644
> --- a/include/uapi/linux/net_dropmon.h
> +++ b/include/uapi/linux/net_dropmon.h
> @@ -58,6 +58,8 @@ enum {
>         NET_DM_CMD_CONFIG_NEW,
>         NET_DM_CMD_STATS_GET,
>         NET_DM_CMD_STATS_NEW,
> +       NET_DM_CMD_START_IFC,
> +       NET_DM_CMD_STOP_IFC,
>         _NET_DM_CMD_MAX,
>  };
>
> @@ -93,6 +95,7 @@ enum net_dm_attr {
>         NET_DM_ATTR_SW_DROPS,                   /* flag */
>         NET_DM_ATTR_HW_DROPS,                   /* flag */
>         NET_DM_ATTR_FLOW_ACTION_COOKIE,         /* binary */
> +       NET_DM_ATTR_IFNAME,                     /* string */
>
>         __NET_DM_ATTR_MAX,
>         NET_DM_ATTR_MAX = __NET_DM_ATTR_MAX - 1
> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index 8e33cec9fc4e..781e69876d2f 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -30,6 +30,7 @@
>  #include <net/genetlink.h>
>  #include <net/netevent.h>
>  #include <net/flow_offload.h>
> +#include <net/sock.h>
>
>  #include <trace/events/skb.h>
>  #include <trace/events/napi.h>
> @@ -46,6 +47,7 @@
>   */
>  static int trace_state = TRACE_OFF;
>  static bool monitor_hw;
> +struct net_device *interface;
>
>  /* net_dm_mutex
>   *
> @@ -54,6 +56,8 @@ static bool monitor_hw;
>   */
>  static DEFINE_MUTEX(net_dm_mutex);
>
> +static DEFINE_SPINLOCK(interface_lock);
> +
>  struct net_dm_stats {
>         u64 dropped;
>         struct u64_stats_sync syncp;
> @@ -255,6 +259,21 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
>
>  out:
>         spin_unlock_irqrestore(&data->lock, flags);
> +       spin_lock_irqsave(&interface_lock, flags);
> +       if (interface && interface != skb->dev) {
> +               skb = skb_clone(skb, GFP_ATOMIC);
> +               if (skb) {
> +                       skb->dev = interface;
> +                       spin_unlock_irqrestore(&interface_lock, flags);
> +                       netif_receive_skb(skb);
> +               } else {
> +                       spin_unlock_irqrestore(&interface_lock, flags);
> +                       pr_err("dropwatch: Not enough memory to clone dropped skb\n");
> +                       return;
> +               }
> +       } else {
> +               spin_unlock_irqrestore(&interface_lock, flags);
> +       }
>  }
>
>  static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
> @@ -1315,6 +1334,53 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
>         return -EOPNOTSUPP;
>  }
>
> +static int net_dm_interface_start(struct net *net, const char *ifname)
> +{
> +       struct net_device *nd = dev_get_by_name(net, ifname);
> +
> +       if (nd)
> +               interface = nd;
> +       else
> +               return -ENODEV;
> +
> +       return 0;
> +}
> +
> +static int net_dm_interface_stop(struct net *net, const char *ifname)
> +{
> +       dev_put(interface);
> +       interface = NULL;
> +
> +       return 0;
> +}
> +
> +static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
> +{
> +       struct net *net = sock_net(skb->sk);
> +       char ifname[IFNAMSIZ];
> +
> +       if (net_dm_is_monitoring())
> +               return -EBUSY;
> +
> +       memset(ifname, 0, IFNAMSIZ);
> +       nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
> +
> +       switch (info->genlhdr->cmd) {
> +       case NET_DM_CMD_START_IFC:
> +               if (!interface)
> +                       return net_dm_interface_start(net, ifname);
> +               else
> +                       return -EBUSY;
> +       case NET_DM_CMD_STOP_IFC:
> +               if (interface)
> +                       return net_dm_interface_stop(net, interface->name);
> +               else
> +                       return -ENODEV;
> +       }
> +
> +       return 0;
> +}
> +
>  static int net_dm_config_fill(struct sk_buff *msg, struct genl_info *info)
>  {
>         void *hdr;
> @@ -1503,6 +1569,7 @@ static int dropmon_net_event(struct notifier_block *ev_block,
>         struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>         struct dm_hw_stat_delta *new_stat = NULL;
>         struct dm_hw_stat_delta *tmp;
> +       unsigned long flags;
>
>         switch (event) {
>         case NETDEV_REGISTER:
> @@ -1529,6 +1596,12 @@ static int dropmon_net_event(struct notifier_block *ev_block,
>                                 }
>                         }
>                 }
> +               spin_lock_irqsave(&interface_lock, flags);
> +               if (interface && interface == dev) {
> +                       dev_put(interface);
> +                       interface = NULL;
> +               }
> +               spin_unlock_irqrestore(&interface_lock, flags);
>                 mutex_unlock(&net_dm_mutex);
>                 break;
>         }
> @@ -1543,6 +1616,7 @@ static const struct nla_policy net_dm_nl_policy[NET_DM_ATTR_MAX + 1] = {
>         [NET_DM_ATTR_QUEUE_LEN] = { .type = NLA_U32 },
>         [NET_DM_ATTR_SW_DROPS]  = {. type = NLA_FLAG },
>         [NET_DM_ATTR_HW_DROPS]  = {. type = NLA_FLAG },
> +       [NET_DM_ATTR_IFNAME] = {. type = NLA_STRING, .len = IFNAMSIZ },
>  };
>
>  static const struct genl_ops dropmon_ops[] = {
> @@ -1570,6 +1644,16 @@ static const struct genl_ops dropmon_ops[] = {
>                 .cmd = NET_DM_CMD_STATS_GET,
>                 .doit = net_dm_cmd_stats_get,
>         },
> +       {
> +               .cmd = NET_DM_CMD_START_IFC,
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +               .doit = net_dm_cmd_ifc_trace,
> +       },
> +       {
> +               .cmd = NET_DM_CMD_STOP_IFC,
> +               .validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +               .doit = net_dm_cmd_ifc_trace,
> +       },
>  };
>
>  static int net_dm_nl_pre_doit(const struct genl_ops *ops,
> --
> 2.18.4
>

Re: [Linux-kernel-mentees] [PATCHv2 net-next] dropwatch: Support monitoring of dropped frames

[Greg KH]

On Mon, Aug 10, 2020 at 03:39:40PM +0200, Izabela Bakollari wrote:
> I have worked on this feature as part of the Linux Kernel Mentorship
> Program. Your review would really help me in this learning process.

You sent this just a bit less than 1 week ago, and it's the middle of
the kernel merge window, where no maintainer can take any new patches
that are not bugfixes, and they are totally busy with the merge window
issues.

Give people a chance, try resending this after the net-next tree is open
in a few weeks.

thanks,

greg k-h

Re: [PATCHv2 net-next] dropwatch: Support monitoring of dropped frames

[Michal Schmidt]

Dne 04. 08. 20 v 18:09 izabela.bakollari@gmail.com napsala:
> From: Izabela Bakollari <izabela.bakollari@gmail.com>
> 
> Dropwatch is a utility that monitors dropped frames by having userspace
> record them over the dropwatch protocol over a file. This augument
> allows live monitoring of dropped frames using tools like tcpdump.
> 
> With this feature, dropwatch allows two additional commands (start and
> stop interface) which allows the assignment of a net_device to the
> dropwatch protocol. When assinged, dropwatch will clone dropped frames,
> and receive them on the assigned interface, allowing tools like tcpdump
> to monitor for them.
> 
> With this feature, create a dummy ethernet interface (ip link add dev
> dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
> these new commands, and then monitor dropped frames in real time by
> running tcpdump -i dummy0.
> 
> Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
> ---
> Changes in v2:
> - protect the dummy ethernet interface from being changed by another
> thread/cpu
> ---
>   include/uapi/linux/net_dropmon.h |  3 ++
>   net/core/drop_monitor.c          | 84 ++++++++++++++++++++++++++++++++
>   2 files changed, 87 insertions(+)
[...]
> @@ -255,6 +259,21 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
>   
>   out:
>   	spin_unlock_irqrestore(&data->lock, flags);
> +	spin_lock_irqsave(&interface_lock, flags);
> +	if (interface && interface != skb->dev) {
> +		skb = skb_clone(skb, GFP_ATOMIC);

I suggest naming the cloned skb "nskb". Less potential for confusion 
that way.

> +		if (skb) {
> +			skb->dev = interface;
> +			spin_unlock_irqrestore(&interface_lock, flags);
> +			netif_receive_skb(skb);
> +		} else {
> +			spin_unlock_irqrestore(&interface_lock, flags);
> +			pr_err("dropwatch: Not enough memory to clone dropped skb\n");

Maybe avoid logging the error here. In NET_DM_ALERT_MODE_PACKET mode, 
drop monitor does not log about the skb_clone() failure either.
We don't want to open the possibility to flood the logs in case this 
somehow gets triggered by every packet.

A coding style suggestion - can you rearrange it so that the error path 
code is spelled out first? Then the regular path does not have to be 
indented further:

       nskb = skb_clone(skb, GFP_ATOMIC);
       if (!nskb) {
               spin_unlock_irqrestore(&interface_lock, flags);
               return;
       }

       /* ... implicit else ... Proceed normally ... */

> +			return;
> +		}
> +	} else {
> +		spin_unlock_irqrestore(&interface_lock, flags);
> +	}
>   }
>   
>   static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
> @@ -1315,6 +1334,53 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
>   	return -EOPNOTSUPP;
>   }
>   
> +static int net_dm_interface_start(struct net *net, const char *ifname)
> +{
> +	struct net_device *nd = dev_get_by_name(net, ifname);
> +
> +	if (nd)
> +		interface = nd;
> +	else
> +		return -ENODEV;
> +
> +	return 0;

Similarly here, consider:

   if (!nd)
           return -ENODEV;

   interface = nd;
   return 0;

But maybe I'm nitpicking ...

> +}
> +
> +static int net_dm_interface_stop(struct net *net, const char *ifname)
> +{
> +	dev_put(interface);
> +	interface = NULL;
> +
> +	return 0;
> +}
> +
> +static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct net *net = sock_net(skb->sk);
> +	char ifname[IFNAMSIZ];
> +
> +	if (net_dm_is_monitoring())
> +		return -EBUSY;
> +
> +	memset(ifname, 0, IFNAMSIZ);
> +	nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
> +
> +	switch (info->genlhdr->cmd) {
> +	case NET_DM_CMD_START_IFC:
> +		if (!interface)
> +			return net_dm_interface_start(net, ifname);
> +		else
> +			return -EBUSY;
> +	case NET_DM_CMD_STOP_IFC:
> +		if (interface)
> +			return net_dm_interface_stop(net, interface->name);
> +		else
> +			return -ENODEV;

... and here too.

Best regards,
Michal

Re: [PATCHv2 net-next] dropwatch: Support monitoring of dropped frames

[Izabela Bakollari]

Thank you for your review. I am working on a patch v3 and will apply
your suggestions where possible.

Best,
Izabela

On Mon, Aug 31, 2020 at 3:18 PM Michal Schmidt <mschmidt@redhat.com> wrote:
>
> Dne 04. 08. 20 v 18:09 izabela.bakollari@gmail.com napsala:
> > From: Izabela Bakollari <izabela.bakollari@gmail.com>
> >
> > Dropwatch is a utility that monitors dropped frames by having userspace
> > record them over the dropwatch protocol over a file. This augument
> > allows live monitoring of dropped frames using tools like tcpdump.
> >
> > With this feature, dropwatch allows two additional commands (start and
> > stop interface) which allows the assignment of a net_device to the
> > dropwatch protocol. When assinged, dropwatch will clone dropped frames,
> > and receive them on the assigned interface, allowing tools like tcpdump
> > to monitor for them.
> >
> > With this feature, create a dummy ethernet interface (ip link add dev
> > dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
> > these new commands, and then monitor dropped frames in real time by
> > running tcpdump -i dummy0.
> >
> > Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
> > ---
> > Changes in v2:
> > - protect the dummy ethernet interface from being changed by another
> > thread/cpu
> > ---
> >   include/uapi/linux/net_dropmon.h |  3 ++
> >   net/core/drop_monitor.c          | 84 ++++++++++++++++++++++++++++++++
> >   2 files changed, 87 insertions(+)
> [...]
> > @@ -255,6 +259,21 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
> >
> >   out:
> >       spin_unlock_irqrestore(&data->lock, flags);
> > +     spin_lock_irqsave(&interface_lock, flags);
> > +     if (interface && interface != skb->dev) {
> > +             skb = skb_clone(skb, GFP_ATOMIC);
>
> I suggest naming the cloned skb "nskb". Less potential for confusion
> that way.
>
> > +             if (skb) {
> > +                     skb->dev = interface;
> > +                     spin_unlock_irqrestore(&interface_lock, flags);
> > +                     netif_receive_skb(skb);
> > +             } else {
> > +                     spin_unlock_irqrestore(&interface_lock, flags);
> > +                     pr_err("dropwatch: Not enough memory to clone dropped skb\n");
>
> Maybe avoid logging the error here. In NET_DM_ALERT_MODE_PACKET mode,
> drop monitor does not log about the skb_clone() failure either.
> We don't want to open the possibility to flood the logs in case this
> somehow gets triggered by every packet.
>
> A coding style suggestion - can you rearrange it so that the error path
> code is spelled out first? Then the regular path does not have to be
> indented further:
>
>        nskb = skb_clone(skb, GFP_ATOMIC);
>        if (!nskb) {
>                spin_unlock_irqrestore(&interface_lock, flags);
>                return;
>        }
>
>        /* ... implicit else ... Proceed normally ... */
>
> > +                     return;
> > +             }
> > +     } else {
> > +             spin_unlock_irqrestore(&interface_lock, flags);
> > +     }
> >   }
> >
> >   static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
> > @@ -1315,6 +1334,53 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
> >       return -EOPNOTSUPP;
> >   }
> >
> > +static int net_dm_interface_start(struct net *net, const char *ifname)
> > +{
> > +     struct net_device *nd = dev_get_by_name(net, ifname);
> > +
> > +     if (nd)
> > +             interface = nd;
> > +     else
> > +             return -ENODEV;
> > +
> > +     return 0;
>
> Similarly here, consider:
>
>    if (!nd)
>            return -ENODEV;
>
>    interface = nd;
>    return 0;
>
> But maybe I'm nitpicking ...
>
> > +}
> > +
> > +static int net_dm_interface_stop(struct net *net, const char *ifname)
> > +{
> > +     dev_put(interface);
> > +     interface = NULL;
> > +
> > +     return 0;
> > +}
> > +
> > +static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +     struct net *net = sock_net(skb->sk);
> > +     char ifname[IFNAMSIZ];
> > +
> > +     if (net_dm_is_monitoring())
> > +             return -EBUSY;
> > +
> > +     memset(ifname, 0, IFNAMSIZ);
> > +     nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
> > +
> > +     switch (info->genlhdr->cmd) {
> > +     case NET_DM_CMD_START_IFC:
> > +             if (!interface)
> > +                     return net_dm_interface_start(net, ifname);
> > +             else
> > +                     return -EBUSY;
> > +     case NET_DM_CMD_STOP_IFC:
> > +             if (interface)
> > +                     return net_dm_interface_stop(net, interface->name);
> > +             else
> > +                     return -ENODEV;
>
> ... and here too.
>
> Best regards,
> Michal
>

[PATCHv3 net-next] dropwatch: Support monitoring of dropped frames

[izabela.bakollari]

From: Izabela Bakollari <izabela.bakollari@gmail.com>

Dropwatch is a utility that monitors dropped frames by having userspace
record them over the dropwatch protocol over a file. This augument
allows live monitoring of dropped frames using tools like tcpdump.

With this feature, dropwatch allows two additional commands (start and
stop interface) which allows the assignment of a net_device to the
dropwatch protocol. When assinged, dropwatch will clone dropped frames,
and receive them on the assigned interface, allowing tools like tcpdump
to monitor for them.

With this feature, create a dummy ethernet interface (ip link add dev
dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
these new commands, and then monitor dropped frames in real time by
running tcpdump -i dummy0.

Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
---
Changes in v3:
- Name the cloned skb "nskb"
- Remove the error log
- Change coding style in some if statements
---
 include/uapi/linux/net_dropmon.h |  3 ++
 net/core/drop_monitor.c          | 80 ++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/include/uapi/linux/net_dropmon.h b/include/uapi/linux/net_dropmon.h
index 67e31f329190..e8e861e03a8a 100644
--- a/include/uapi/linux/net_dropmon.h
+++ b/include/uapi/linux/net_dropmon.h
@@ -58,6 +58,8 @@ enum {
 	NET_DM_CMD_CONFIG_NEW,
 	NET_DM_CMD_STATS_GET,
 	NET_DM_CMD_STATS_NEW,
+	NET_DM_CMD_START_IFC,
+	NET_DM_CMD_STOP_IFC,
 	_NET_DM_CMD_MAX,
 };
 
@@ -93,6 +95,7 @@ enum net_dm_attr {
 	NET_DM_ATTR_SW_DROPS,			/* flag */
 	NET_DM_ATTR_HW_DROPS,			/* flag */
 	NET_DM_ATTR_FLOW_ACTION_COOKIE,		/* binary */
+	NET_DM_ATTR_IFNAME,			/* string */
 
 	__NET_DM_ATTR_MAX,
 	NET_DM_ATTR_MAX = __NET_DM_ATTR_MAX - 1
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 8e33cec9fc4e..ae5ed70b6b2a 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -30,6 +30,7 @@
 #include <net/genetlink.h>
 #include <net/netevent.h>
 #include <net/flow_offload.h>
+#include <net/sock.h>
 
 #include <trace/events/skb.h>
 #include <trace/events/napi.h>
@@ -46,6 +47,7 @@
  */
 static int trace_state = TRACE_OFF;
 static bool monitor_hw;
+struct net_device *interface;
 
 /* net_dm_mutex
  *
@@ -54,6 +56,8 @@ static bool monitor_hw;
  */
 static DEFINE_MUTEX(net_dm_mutex);
 
+static DEFINE_SPINLOCK(interface_lock);
+
 struct net_dm_stats {
 	u64 dropped;
 	struct u64_stats_sync syncp;
@@ -217,6 +221,7 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
 	struct nlattr *nla;
 	int i;
 	struct sk_buff *dskb;
+	struct sk_buff *nskb;
 	struct per_cpu_dm_data *data;
 	unsigned long flags;
 
@@ -255,6 +260,18 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
 
 out:
 	spin_unlock_irqrestore(&data->lock, flags);
+	spin_lock_irqsave(&interface_lock, flags);
+	nskb = skb_clone(skb, GFP_ATOMIC);
+	if (!nskb) {
+		spin_unlock_irqrestore(&interface_lock, flags);
+		return;
+	} else if (interface && interface != nskb->dev) {
+		nskb->dev = interface;
+		spin_unlock_irqrestore(&interface_lock, flags);
+		netif_receive_skb(nskb);
+	} else {
+		spin_unlock_irqrestore(&interface_lock, flags);
+	}
 }
 
 static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
@@ -1315,6 +1332,51 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
 	return -EOPNOTSUPP;
 }
 
+static int net_dm_interface_start(struct net *net, const char *ifname)
+{
+	struct net_device *nd = dev_get_by_name(net, ifname);
+
+	if (!nd)
+		return -ENODEV;
+
+	interface = nd;
+
+	return 0;
+}
+
+static int net_dm_interface_stop(struct net *net, const char *ifname)
+{
+	dev_put(interface);
+	interface = NULL;
+
+	return 0;
+}
+
+static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	char ifname[IFNAMSIZ];
+
+	if (net_dm_is_monitoring())
+		return -EBUSY;
+
+	memset(ifname, 0, IFNAMSIZ);
+	nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
+
+	switch (info->genlhdr->cmd) {
+	case NET_DM_CMD_START_IFC:
+		if (interface)
+			return -EBUSY;
+		return net_dm_interface_start(net, ifname);
+	case NET_DM_CMD_STOP_IFC:
+		if (!interface)
+			return -ENODEV;
+		return net_dm_interface_stop(net, interface->name);
+	}
+
+	return 0;
+}
+
 static int net_dm_config_fill(struct sk_buff *msg, struct genl_info *info)
 {
 	void *hdr;
@@ -1503,6 +1565,7 @@ static int dropmon_net_event(struct notifier_block *ev_block,
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct dm_hw_stat_delta *new_stat = NULL;
 	struct dm_hw_stat_delta *tmp;
+	unsigned long flags;
 
 	switch (event) {
 	case NETDEV_REGISTER:
@@ -1529,6 +1592,12 @@ static int dropmon_net_event(struct notifier_block *ev_block,
 				}
 			}
 		}
+		spin_lock_irqsave(&interface_lock, flags);
+		if (interface && interface == dev) {
+			dev_put(interface);
+			interface = NULL;
+		}
+		spin_unlock_irqrestore(&interface_lock, flags);
 		mutex_unlock(&net_dm_mutex);
 		break;
 	}
@@ -1543,6 +1612,7 @@ static const struct nla_policy net_dm_nl_policy[NET_DM_ATTR_MAX + 1] = {
 	[NET_DM_ATTR_QUEUE_LEN] = { .type = NLA_U32 },
 	[NET_DM_ATTR_SW_DROPS]	= {. type = NLA_FLAG },
 	[NET_DM_ATTR_HW_DROPS]	= {. type = NLA_FLAG },
+	[NET_DM_ATTR_IFNAME] = {. type = NLA_STRING, .len = IFNAMSIZ },
 };
 
 static const struct genl_ops dropmon_ops[] = {
@@ -1570,6 +1640,16 @@ static const struct genl_ops dropmon_ops[] = {
 		.cmd = NET_DM_CMD_STATS_GET,
 		.doit = net_dm_cmd_stats_get,
 	},
+	{
+		.cmd = NET_DM_CMD_START_IFC,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = net_dm_cmd_ifc_trace,
+	},
+	{
+		.cmd = NET_DM_CMD_STOP_IFC,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = net_dm_cmd_ifc_trace,
+	},
 };
 
 static int net_dm_nl_pre_doit(const struct genl_ops *ops,
-- 
2.18.4

Re: [PATCHv3 net-next] dropwatch: Support monitoring of dropped frames

[Eric Dumazet]

On 9/2/20 10:16 AM, izabela.bakollari@gmail.com wrote:
> From: Izabela Bakollari <izabela.bakollari@gmail.com>
> 
> Dropwatch is a utility that monitors dropped frames by having userspace
> record them over the dropwatch protocol over a file. This augument
> allows live monitoring of dropped frames using tools like tcpdump.
> 
> With this feature, dropwatch allows two additional commands (start and
> stop interface) which allows the assignment of a net_device to the
> dropwatch protocol. When assinged, dropwatch will clone dropped frames,
> and receive them on the assigned interface, allowing tools like tcpdump
> to monitor for them.
> 
> With this feature, create a dummy ethernet interface (ip link add dev
> dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
> these new commands, and then monitor dropped frames in real time by
> running tcpdump -i dummy0.
> 
> Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
> ---
> Changes in v3:
> - Name the cloned skb "nskb"
> - Remove the error log
> - Change coding style in some if statements
> ---
>  include/uapi/linux/net_dropmon.h |  3 ++
>  net/core/drop_monitor.c          | 80 ++++++++++++++++++++++++++++++++
>  2 files changed, 83 insertions(+)
> 
> diff --git a/include/uapi/linux/net_dropmon.h b/include/uapi/linux/net_dropmon.h
> index 67e31f329190..e8e861e03a8a 100644
> --- a/include/uapi/linux/net_dropmon.h
> +++ b/include/uapi/linux/net_dropmon.h
> @@ -58,6 +58,8 @@ enum {
>  	NET_DM_CMD_CONFIG_NEW,
>  	NET_DM_CMD_STATS_GET,
>  	NET_DM_CMD_STATS_NEW,
> +	NET_DM_CMD_START_IFC,
> +	NET_DM_CMD_STOP_IFC,
>  	_NET_DM_CMD_MAX,
>  };
>  
> @@ -93,6 +95,7 @@ enum net_dm_attr {
>  	NET_DM_ATTR_SW_DROPS,			/* flag */
>  	NET_DM_ATTR_HW_DROPS,			/* flag */
>  	NET_DM_ATTR_FLOW_ACTION_COOKIE,		/* binary */
> +	NET_DM_ATTR_IFNAME,			/* string */
>  
>  	__NET_DM_ATTR_MAX,
>  	NET_DM_ATTR_MAX = __NET_DM_ATTR_MAX - 1
> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index 8e33cec9fc4e..ae5ed70b6b2a 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -30,6 +30,7 @@
>  #include <net/genetlink.h>
>  #include <net/netevent.h>
>  #include <net/flow_offload.h>
> +#include <net/sock.h>
>  
>  #include <trace/events/skb.h>
>  #include <trace/events/napi.h>
> @@ -46,6 +47,7 @@
>   */
>  static int trace_state = TRACE_OFF;
>  static bool monitor_hw;
> +struct net_device *interface;
>  
>  /* net_dm_mutex
>   *
> @@ -54,6 +56,8 @@ static bool monitor_hw;
>   */
>  static DEFINE_MUTEX(net_dm_mutex);
>  
> +static DEFINE_SPINLOCK(interface_lock);
> +
>  struct net_dm_stats {
>  	u64 dropped;
>  	struct u64_stats_sync syncp;
> @@ -217,6 +221,7 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
>  	struct nlattr *nla;
>  	int i;
>  	struct sk_buff *dskb;
> +	struct sk_buff *nskb;
>  	struct per_cpu_dm_data *data;
>  	unsigned long flags;
>  
> @@ -255,6 +260,18 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
>  
>  out:
>  	spin_unlock_irqrestore(&data->lock, flags);
> +	spin_lock_irqsave(&interface_lock, flags);
> +	nskb = skb_clone(skb, GFP_ATOMIC);

1) Why calling skb_clone() if @interface is NULL ?


> +	if (!nskb) {
> +		spin_unlock_irqrestore(&interface_lock, flags);
> +		return;
> +	} else if (interface && interface != nskb->dev) {

2) Since there is no check about @interface being a dummy device,
it seems possible for a malicious user to set up another virtual
device (like bonding) so that the "interface != nskb->dev"  test
wont be able to detect a loop.

We could therefore have an infinite loop.


> +		nskb->dev = interface;
> +		spin_unlock_irqrestore(&interface_lock, flags);
> +		netif_receive_skb(nskb);



> +	} else {

 3)  nskb seems to be leaked here ? See point 1)

> +		spin_unlock_irqrestore(&interface_lock, flags);
> +	}
>  }
>  
>  static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
> @@ -1315,6 +1332,51 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
>  	return -EOPNOTSUPP;
>  }
>  
> +static int net_dm_interface_start(struct net *net, const char *ifname)
> +{
> +	struct net_device *nd = dev_get_by_name(net, ifname);
> +
> +	if (!nd)
> +		return -ENODEV;
> +
> +	interface = nd;
> +
> +	return 0;
> +}
> +
> +static int net_dm_interface_stop(struct net *net, const char *ifname)
> +{
> +	dev_put(interface);
> +	interface = NULL;
> +
> +	return 0;
> +}
> +
> +static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct net *net = sock_net(skb->sk);
> +	char ifname[IFNAMSIZ];
> +
> +	if (net_dm_is_monitoring())
> +		return -EBUSY;
> +
> +	memset(ifname, 0, IFNAMSIZ);
> +	nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);

4) info->attrs[NET_DM_ATTR_IFNAME] could be NULL at this point.

> +
> +	switch (info->genlhdr->cmd) {
> +	case NET_DM_CMD_START_IFC:

5) interface_lock is not held, this seems racy.

> +		if (interface)
> +			return -EBUSY;
> +		return net_dm_interface_start(net, ifname);
> +	case NET_DM_CMD_STOP_IFC:

6) interface_lock is not held, this seems racy.
> +		if (!interface)
> +			return -ENODEV;
> +		return net_dm_interface_stop(net, interface->name);
> +	}
> +
> +	return 0;
> +}
> +
>  static int net_dm_config_fill(struct sk_buff *msg, struct genl_info *info)
>  {
>  	void *hdr;
> @@ -1503,6 +1565,7 @@ static int dropmon_net_event(struct notifier_block *ev_block,
>  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>  	struct dm_hw_stat_delta *new_stat = NULL;
>  	struct dm_hw_stat_delta *tmp;
> +	unsigned long flags;
>  
>  	switch (event) {
>  	case NETDEV_REGISTER:
> @@ -1529,6 +1592,12 @@ static int dropmon_net_event(struct notifier_block *ev_block,
>  				}
>  			}
>  		}
> +		spin_lock_irqsave(&interface_lock, flags);
> +		if (interface && interface == dev) {
> +			dev_put(interface);
> +			interface = NULL;
> +		}
> +		spin_unlock_irqrestore(&interface_lock, flags);
>  		mutex_unlock(&net_dm_mutex);
>  		break;
>  	}
> @@ -1543,6 +1612,7 @@ static const struct nla_policy net_dm_nl_policy[NET_DM_ATTR_MAX + 1] = {
>  	[NET_DM_ATTR_QUEUE_LEN] = { .type = NLA_U32 },
>  	[NET_DM_ATTR_SW_DROPS]	= {. type = NLA_FLAG },
>  	[NET_DM_ATTR_HW_DROPS]	= {. type = NLA_FLAG },
> +	[NET_DM_ATTR_IFNAME] = {. type = NLA_STRING, .len = IFNAMSIZ },
>  };
>  
>  static const struct genl_ops dropmon_ops[] = {
> @@ -1570,6 +1640,16 @@ static const struct genl_ops dropmon_ops[] = {
>  		.cmd = NET_DM_CMD_STATS_GET,
>  		.doit = net_dm_cmd_stats_get,
>  	},
> +	{
> +		.cmd = NET_DM_CMD_START_IFC,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = net_dm_cmd_ifc_trace,
> +	},
> +	{
> +		.cmd = NET_DM_CMD_STOP_IFC,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = net_dm_cmd_ifc_trace,
> +	},
>  };
>  
>  static int net_dm_nl_pre_doit(const struct genl_ops *ops,
> 

[PATCHv4 net-next] dropwatch: Support monitoring of dropped frames

[izabela.bakollari]

From: Izabela Bakollari <izabela.bakollari@gmail.com>

Dropwatch is a utility that monitors dropped frames by having userspace
record them over the dropwatch protocol over a file. This augument
allows live monitoring of dropped frames using tools like tcpdump.

With this feature, dropwatch allows two additional commands (start and
stop interface) which allows the assignment of a net_device to the
dropwatch protocol. When assinged, dropwatch will clone dropped frames,
and receive them on the assigned interface, allowing tools like tcpdump
to monitor for them.

With this feature, create a dummy ethernet interface (ip link add dev
dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
these new commands, and then monitor dropped frames in real time by
running tcpdump -i dummy0.

Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>
---
 include/uapi/linux/net_dropmon.h |   3 +
 net/core/drop_monitor.c          | 120 +++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+)

diff --git a/include/uapi/linux/net_dropmon.h b/include/uapi/linux/net_dropmon.h
index 67e31f329190..e8e861e03a8a 100644
--- a/include/uapi/linux/net_dropmon.h
+++ b/include/uapi/linux/net_dropmon.h
@@ -58,6 +58,8 @@ enum {
 	NET_DM_CMD_CONFIG_NEW,
 	NET_DM_CMD_STATS_GET,
 	NET_DM_CMD_STATS_NEW,
+	NET_DM_CMD_START_IFC,
+	NET_DM_CMD_STOP_IFC,
 	_NET_DM_CMD_MAX,
 };
 
@@ -93,6 +95,7 @@ enum net_dm_attr {
 	NET_DM_ATTR_SW_DROPS,			/* flag */
 	NET_DM_ATTR_HW_DROPS,			/* flag */
 	NET_DM_ATTR_FLOW_ACTION_COOKIE,		/* binary */
+	NET_DM_ATTR_IFNAME,			/* string */
 
 	__NET_DM_ATTR_MAX,
 	NET_DM_ATTR_MAX = __NET_DM_ATTR_MAX - 1
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 8e33cec9fc4e..dea85291808b 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -30,6 +30,7 @@
 #include <net/genetlink.h>
 #include <net/netevent.h>
 #include <net/flow_offload.h>
+#include <net/sock.h>
 
 #include <trace/events/skb.h>
 #include <trace/events/napi.h>
@@ -46,6 +47,7 @@
  */
 static int trace_state = TRACE_OFF;
 static bool monitor_hw;
+struct net_device *interface;
 
 /* net_dm_mutex
  *
@@ -54,6 +56,8 @@ static bool monitor_hw;
  */
 static DEFINE_MUTEX(net_dm_mutex);
 
+static DEFINE_SPINLOCK(interface_lock);
+
 struct net_dm_stats {
 	u64 dropped;
 	struct u64_stats_sync syncp;
@@ -217,6 +221,7 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
 	struct nlattr *nla;
 	int i;
 	struct sk_buff *dskb;
+	struct sk_buff *nskb = NULL;
 	struct per_cpu_dm_data *data;
 	unsigned long flags;
 
@@ -255,6 +260,20 @@ static void trace_drop_common(struct sk_buff *skb, void *location)
 
 out:
 	spin_unlock_irqrestore(&data->lock, flags);
+	spin_lock_irqsave(&interface_lock, flags);
+	if (interface && interface != skb->dev) {
+		nskb = skb_clone(skb, GFP_ATOMIC);
+		if (!nskb)
+			goto free;
+		nskb->dev = interface;
+	}
+	spin_unlock_irqrestore(&interface_lock, flags);
+	if (nskb)
+		netif_receive_skb(nskb);
+
+free:
+	spin_unlock_irqrestore(&interface_lock, flags);
+	return;
 }
 
 static void trace_kfree_skb_hit(void *ignore, struct sk_buff *skb, void *location)
@@ -1315,6 +1334,89 @@ static int net_dm_cmd_trace(struct sk_buff *skb,
 	return -EOPNOTSUPP;
 }
 
+static bool is_dummy_dev(struct net_device *dev)
+{
+	struct ethtool_drvinfo drvinfo;
+
+	if (dev->ethtool_ops && dev->ethtool_ops->get_drvinfo) {
+		memset(&drvinfo, 0, sizeof(drvinfo));
+		dev->ethtool_ops->get_drvinfo(dev, &drvinfo);
+
+		if (strcmp(drvinfo.driver, "dummy"))
+			return false;
+		return true;
+	}
+	return false;
+}
+
+static int net_dm_interface_start(struct net *net, const char *ifname)
+{
+	struct net_device *dev = dev_get_by_name(net, ifname);
+	unsigned long flags;
+	int rc = -EBUSY;
+
+	if (!dev)
+		return -ENODEV;
+
+	if (!is_dummy_dev(dev)) {
+		rc = -EOPNOTSUPP;
+		goto out;
+	}
+
+	spin_lock_irqsave(&interface_lock, flags);
+	if (!interface) {
+		interface = dev;
+		rc = 0;
+	}
+	spin_unlock_irqrestore(&interface_lock, flags);
+
+	goto out;
+
+out:
+	dev_put(dev);
+	return rc;
+}
+
+static int net_dm_interface_stop(struct net *net, const char *ifname)
+{
+	unsigned long flags;
+	int rc = -ENODEV;
+
+	spin_lock_irqsave(&interface_lock, flags);
+	if (interface && interface->name == ifname) {
+		dev_put(interface);
+		interface = NULL;
+		rc = 0;
+	}
+	spin_unlock_irqrestore(&interface_lock, flags);
+
+	return rc;
+}
+
+static int net_dm_cmd_ifc_trace(struct sk_buff *skb, struct genl_info *info)
+{
+	struct net *net = sock_net(skb->sk);
+	char ifname[IFNAMSIZ];
+
+	if (net_dm_is_monitoring())
+		return -EBUSY;
+
+	if (!info->attrs[NET_DM_ATTR_IFNAME])
+		return -EINVAL;
+
+	memset(ifname, 0, IFNAMSIZ);
+	nla_strlcpy(ifname, info->attrs[NET_DM_ATTR_IFNAME], IFNAMSIZ - 1);
+
+	switch (info->genlhdr->cmd) {
+	case NET_DM_CMD_START_IFC:
+		return net_dm_interface_start(net, ifname);
+	case NET_DM_CMD_STOP_IFC:
+		return net_dm_interface_stop(net, ifname);
+	}
+
+	return 0;
+}
+
 static int net_dm_config_fill(struct sk_buff *msg, struct genl_info *info)
 {
 	void *hdr;
@@ -1503,6 +1605,7 @@ static int dropmon_net_event(struct notifier_block *ev_block,
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct dm_hw_stat_delta *new_stat = NULL;
 	struct dm_hw_stat_delta *tmp;
+	unsigned long flags;
 
 	switch (event) {
 	case NETDEV_REGISTER:
@@ -1529,6 +1632,12 @@ static int dropmon_net_event(struct notifier_block *ev_block,
 				}
 			}
 		}
+		spin_lock_irqsave(&interface_lock, flags);
+		if (interface && interface == dev) {
+			dev_put(interface);
+			interface = NULL;
+		}
+		spin_unlock_irqrestore(&interface_lock, flags);
 		mutex_unlock(&net_dm_mutex);
 		break;
 	}
@@ -1543,6 +1652,7 @@ static const struct nla_policy net_dm_nl_policy[NET_DM_ATTR_MAX + 1] = {
 	[NET_DM_ATTR_QUEUE_LEN] = { .type = NLA_U32 },
 	[NET_DM_ATTR_SW_DROPS]	= {. type = NLA_FLAG },
 	[NET_DM_ATTR_HW_DROPS]	= {. type = NLA_FLAG },
+	[NET_DM_ATTR_IFNAME] = {. type = NLA_STRING, .len = IFNAMSIZ },
 };
 
 static const struct genl_ops dropmon_ops[] = {
@@ -1570,6 +1680,16 @@ static const struct genl_ops dropmon_ops[] = {
 		.cmd = NET_DM_CMD_STATS_GET,
 		.doit = net_dm_cmd_stats_get,
 	},
+	{
+		.cmd = NET_DM_CMD_START_IFC,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = net_dm_cmd_ifc_trace,
+	},
+	{
+		.cmd = NET_DM_CMD_STOP_IFC,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = net_dm_cmd_ifc_trace,
+	},
 };
 
 static int net_dm_nl_pre_doit(const struct genl_ops *ops,
-- 
2.18.4

Re: [PATCHv4 net-next] dropwatch: Support monitoring of dropped frames

[Jakub Kicinski]

On Fri, 23 Oct 2020 06:29:43 +0200 izabela.bakollari@gmail.com wrote:
> From: Izabela Bakollari <izabela.bakollari@gmail.com>
> 
> Dropwatch is a utility that monitors dropped frames by having userspace
> record them over the dropwatch protocol over a file. This augument
> allows live monitoring of dropped frames using tools like tcpdump.
> 
> With this feature, dropwatch allows two additional commands (start and
> stop interface) which allows the assignment of a net_device to the
> dropwatch protocol. When assinged, dropwatch will clone dropped frames,
> and receive them on the assigned interface, allowing tools like tcpdump
> to monitor for them.
> 
> With this feature, create a dummy ethernet interface (ip link add dev
> dummy0 type dummy), assign it to the dropwatch kernel subsystem, by using
> these new commands, and then monitor dropped frames in real time by
> running tcpdump -i dummy0.
> 
> Signed-off-by: Izabela Bakollari <izabela.bakollari@gmail.com>

Doesn't seem to apply to net-next, also the tree is closed during the
merge window so please rebase and repost after the weekend.