[PATCH net] openvswitch: Fix ovs_vport_get_stats()

[PATCH net] openvswitch: Fix ovs_vport_get_stats()

[Pravin B Shelar]

Not every device has dev->tstats set. So when OVS tries to calculate
vport stats it causes kernel panic. Following patch fixes it by
using standard API to get net-device stats.

---8<---
Unable to handle kernel paging request at virtual address 766b4008
Internal error: Oops: 96000005 [#1] PREEMPT SMP
Modules linked in: vport_vxlan vxlan ip6_udp_tunnel udp_tunnel tun bridge stp llc openvswitch ipv6
CPU: 7 PID: 1108 Comm: ovs-vswitchd Not tainted 4.3.0-rc3+ #82
PC is at ovs_vport_get_stats+0x150/0x1f8 [openvswitch]
<snip>
Call trace:
 [<ffffffbffc0859f8>] ovs_vport_get_stats+0x150/0x1f8 [openvswitch]
 [<ffffffbffc07cdb0>] ovs_vport_cmd_fill_info+0x140/0x1e0 [openvswitch]
 [<ffffffbffc07cf0c>] ovs_vport_cmd_dump+0xbc/0x138 [openvswitch]
 [<ffffffc00045a5ac>] netlink_dump+0xb8/0x258
 [<ffffffc00045ace0>] __netlink_dump_start+0x120/0x178
 [<ffffffc00045dd9c>] genl_family_rcv_msg+0x2d4/0x308
 [<ffffffc00045de58>] genl_rcv_msg+0x88/0xc4
 [<ffffffc00045cf24>] netlink_rcv_skb+0xd4/0x100
 [<ffffffc00045dab0>] genl_rcv+0x30/0x48
 [<ffffffc00045c830>] netlink_unicast+0x154/0x200
 [<ffffffc00045cc9c>] netlink_sendmsg+0x308/0x364
 [<ffffffc00041e10c>] sock_sendmsg+0x14/0x2c
 [<ffffffc000420d58>] SyS_sendto+0xbc/0xf0
Code: aa1603e1 f94037a4 aa1303e2 aa1703e0 (f9400465)

Reported-by: Tomasz Sawicki <tomasz.sawicki@objectiveintegration.uk>
Fixes: 8c876639c98 ("openvswitch: Remove vport stats.")
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
 net/openvswitch/vport.c |   42 +++++++++++++-----------------------------
 1 files changed, 13 insertions(+), 29 deletions(-)

diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index dc81dc6..fc5c0b9 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -280,35 +280,19 @@ void ovs_vport_del(struct vport *vport)
  */
 void ovs_vport_get_stats(struct vport *vport, struct ovs_vport_stats *stats)
 {
-	struct net_device *dev = vport->dev;
-	int i;
-
-	memset(stats, 0, sizeof(*stats));
-	stats->rx_errors  = dev->stats.rx_errors;
-	stats->tx_errors  = dev->stats.tx_errors;
-	stats->tx_dropped = dev->stats.tx_dropped;
-	stats->rx_dropped = dev->stats.rx_dropped;
-
-	stats->rx_dropped += atomic_long_read(&dev->rx_dropped);
-	stats->tx_dropped += atomic_long_read(&dev->tx_dropped);
-
-	for_each_possible_cpu(i) {
-		const struct pcpu_sw_netstats *percpu_stats;
-		struct pcpu_sw_netstats local_stats;
-		unsigned int start;
-
-		percpu_stats = per_cpu_ptr(dev->tstats, i);
-
-		do {
-			start = u64_stats_fetch_begin_irq(&percpu_stats->syncp);
-			local_stats = *percpu_stats;
-		} while (u64_stats_fetch_retry_irq(&percpu_stats->syncp, start));
-
-		stats->rx_bytes		+= local_stats.rx_bytes;
-		stats->rx_packets	+= local_stats.rx_packets;
-		stats->tx_bytes		+= local_stats.tx_bytes;
-		stats->tx_packets	+= local_stats.tx_packets;
-	}
+	const struct rtnl_link_stats64 *dev_stats;
+	struct rtnl_link_stats64 temp;
+
+	dev_stats = dev_get_stats(vport->dev, &temp);
+	stats->rx_errors  = dev_stats->rx_errors;
+	stats->tx_errors  = dev_stats->tx_errors;
+	stats->tx_dropped = dev_stats->tx_dropped;
+	stats->rx_dropped = dev_stats->rx_dropped;
+
+	stats->rx_bytes	  = dev_stats->rx_bytes;
+	stats->rx_packets = dev_stats->rx_packets;
+	stats->tx_bytes	  = dev_stats->tx_bytes;
+	stats->tx_packets = dev_stats->tx_packets;
 }
 
 /**
-- 
1.7.1

Re: [PATCH net] openvswitch: Fix ovs_vport_get_stats()

[Thomas Graf]

On 10/02/15 at 02:56pm, Pravin B Shelar wrote:
> Not every device has dev->tstats set. So when OVS tries to calculate
> vport stats it causes kernel panic. Following patch fixes it by
> using standard API to get net-device stats.
> 
> ---8<---
> Unable to handle kernel paging request at virtual address 766b4008
> Internal error: Oops: 96000005 [#1] PREEMPT SMP
> Modules linked in: vport_vxlan vxlan ip6_udp_tunnel udp_tunnel tun bridge stp llc openvswitch ipv6
> CPU: 7 PID: 1108 Comm: ovs-vswitchd Not tainted 4.3.0-rc3+ #82
> PC is at ovs_vport_get_stats+0x150/0x1f8 [openvswitch]
> <snip>
> Call trace:
>  [<ffffffbffc0859f8>] ovs_vport_get_stats+0x150/0x1f8 [openvswitch]
>  [<ffffffbffc07cdb0>] ovs_vport_cmd_fill_info+0x140/0x1e0 [openvswitch]
>  [<ffffffbffc07cf0c>] ovs_vport_cmd_dump+0xbc/0x138 [openvswitch]
>  [<ffffffc00045a5ac>] netlink_dump+0xb8/0x258
>  [<ffffffc00045ace0>] __netlink_dump_start+0x120/0x178
>  [<ffffffc00045dd9c>] genl_family_rcv_msg+0x2d4/0x308
>  [<ffffffc00045de58>] genl_rcv_msg+0x88/0xc4
>  [<ffffffc00045cf24>] netlink_rcv_skb+0xd4/0x100
>  [<ffffffc00045dab0>] genl_rcv+0x30/0x48
>  [<ffffffc00045c830>] netlink_unicast+0x154/0x200
>  [<ffffffc00045cc9c>] netlink_sendmsg+0x308/0x364
>  [<ffffffc00041e10c>] sock_sendmsg+0x14/0x2c
>  [<ffffffc000420d58>] SyS_sendto+0xbc/0xf0
> Code: aa1603e1 f94037a4 aa1303e2 aa1703e0 (f9400465)
> 
> Reported-by: Tomasz Sawicki <tomasz.sawicki@objectiveintegration.uk>
> Fixes: 8c876639c98 ("openvswitch: Remove vport stats.")
> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>

Acked-by: Thomas Graf <tgraf@suug.ch>

Re: [PATCH net] openvswitch: Fix ovs_vport_get_stats()

[David Miller]

From: Pravin B Shelar <pshelar@nicira.com>
Date: Fri,  2 Oct 2015 14:56:34 -0700

> Not every device has dev->tstats set. So when OVS tries to calculate
> vport stats it causes kernel panic. Following patch fixes it by
> using standard API to get net-device stats.
 ...
> Reported-by: Tomasz Sawicki <tomasz.sawicki@objectiveintegration.uk>
> Fixes: 8c876639c98 ("openvswitch: Remove vport stats.")
> Signed-off-by: Pravin B Shelar <pshelar@nicira.com>

Applied.

[PATCH 2/2] openvswitch: Allocate memory for ovs internal device stats.

[James Morse]

"openvswitch: Remove vport stats" removed the per-vport statistics, in
order to use the netdev's statistics fields.
"openvswitch: Fix ovs_vport_get_stats()" fixed the export of these stats
to user-space, by using the provided netdev_ops to collate them - but ovs
internal devices still use an unallocated dev->tstats field to count
packets, which are not exported by this api.

Allocate the dev->tstats field for ovs internal devices, and wire up
ndo_get_stats64 with the original implementation of
ovs_vport_get_stats().

On its own, "openvswitch: Fix ovs_vport_get_stats()" fixes the OOPs,
unmasking a full-on panic on arm64:
=============%<==============
[<ffffffbffc00ce4c>] internal_dev_recv+0xa8/0x170 [openvswitch]
[<ffffffbffc0008b4>] do_output.isra.31+0x60/0x19c [openvswitch]
[<ffffffbffc000bf8>] do_execute_actions+0x208/0x11c0 [openvswitch]
[<ffffffbffc001c78>] ovs_execute_actions+0xc8/0x238 [openvswitch]
[<ffffffbffc003dfc>] ovs_packet_cmd_execute+0x21c/0x288 [openvswitch]
[<ffffffc0005e8c5c>] genl_family_rcv_msg+0x1b0/0x310
[<ffffffc0005e8e60>] genl_rcv_msg+0xa4/0xe4
[<ffffffc0005e7ddc>] netlink_rcv_skb+0xb0/0xdc
[<ffffffc0005e8a94>] genl_rcv+0x38/0x50
[<ffffffc0005e76c0>] netlink_unicast+0x164/0x210
[<ffffffc0005e7b70>] netlink_sendmsg+0x304/0x368
[<ffffffc0005a21c0>] sock_sendmsg+0x30/0x4c
[SNIP]
Kernel panic - not syncing: Fatal exception in interrupt
=============%<==============

Signed-off-by: James Morse <james.morse@arm.com>
Fixes: 8c876639c985 ("openvswitch: Remove vport stats.")
---
Hi netdev,

"openvswitch: Fix ovs_vport_get_stats()"  - already applied according to
[0] (where?), is an incomplete fix for the issue in "openvswitch: Remove
vport stats.". Use of an unallocated dev->tstats remains for ovs internal
devices, which causes a panic on arm64. Could this patch and "openvswitch:
Fix ovs_vport_get_stats()" be considered as fixes for v4.3?

Thanks!

James Morse


[0] https://patchwork.ozlabs.org/patch/525827/

 net/openvswitch/vport-internal_dev.c | 43 +++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c
index 388b8a6bf112..758e53cb7a81 100644
--- a/net/openvswitch/vport-internal_dev.c
+++ b/net/openvswitch/vport-internal_dev.c
@@ -106,12 +106,48 @@ static void internal_dev_destructor(struct net_device *dev)
 	free_netdev(dev);
 }
 
+static struct rtnl_link_stats64 *
+internal_get_stats(struct net_device *dev, struct rtnl_link_stats64 *stats)
+{
+	int i;
+
+	memset(stats, 0, sizeof(*stats));
+	stats->rx_errors  = dev->stats.rx_errors;
+	stats->tx_errors  = dev->stats.tx_errors;
+	stats->tx_dropped = dev->stats.tx_dropped;
+	stats->rx_dropped = dev->stats.rx_dropped;
+
+	stats->rx_dropped += atomic_long_read(&dev->rx_dropped);
+	stats->tx_dropped += atomic_long_read(&dev->tx_dropped);
+
+	for_each_possible_cpu(i) {
+		const struct pcpu_sw_netstats *percpu_stats;
+		struct pcpu_sw_netstats local_stats;
+		unsigned int start;
+
+		percpu_stats = per_cpu_ptr(dev->tstats, i);
+
+		do {
+			start = u64_stats_fetch_begin_irq(&percpu_stats->syncp);
+			local_stats = *percpu_stats;
+		} while (u64_stats_fetch_retry_irq(&percpu_stats->syncp, start));
+
+		stats->rx_bytes         += local_stats.rx_bytes;
+		stats->rx_packets       += local_stats.rx_packets;
+		stats->tx_bytes         += local_stats.tx_bytes;
+		stats->tx_packets       += local_stats.tx_packets;
+	}
+
+	return stats;
+}
+
 static const struct net_device_ops internal_dev_netdev_ops = {
 	.ndo_open = internal_dev_open,
 	.ndo_stop = internal_dev_stop,
 	.ndo_start_xmit = internal_dev_xmit,
 	.ndo_set_mac_address = eth_mac_addr,
 	.ndo_change_mtu = internal_dev_change_mtu,
+	.ndo_get_stats64 = internal_get_stats,
 };
 
 static struct rtnl_link_ops internal_dev_link_ops __read_mostly = {
@@ -161,6 +197,11 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
 		err = -ENOMEM;
 		goto error_free_vport;
 	}
+	vport->dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
+	if (!vport->dev->tstats) {
+		err = -ENOMEM;
+		goto error_free_vport;
+	}
 
 	dev_net_set(vport->dev, ovs_dp_get_net(vport->dp));
 	internal_dev = internal_dev_priv(vport->dev);
@@ -198,7 +239,7 @@ static void internal_dev_destroy(struct vport *vport)
 
 	/* unregister_netdevice() waits for an RCU grace period. */
 	unregister_netdevice(vport->dev);
-
+	free_percpu(vport->dev->tstats);
 	rtnl_unlock();
 }
 
-- 
2.1.4

Re: [PATCH 2/2] openvswitch: Allocate memory for ovs internal device stats.

[Pravin Shelar]

On Thu, Oct 15, 2015 at 7:54 AM, James Morse <james.morse@arm.com> wrote:
> "openvswitch: Remove vport stats" removed the per-vport statistics, in
> order to use the netdev's statistics fields.
> "openvswitch: Fix ovs_vport_get_stats()" fixed the export of these stats
> to user-space, by using the provided netdev_ops to collate them - but ovs
> internal devices still use an unallocated dev->tstats field to count
> packets, which are not exported by this api.
>
> Allocate the dev->tstats field for ovs internal devices, and wire up
> ndo_get_stats64 with the original implementation of
> ovs_vport_get_stats().
>
> On its own, "openvswitch: Fix ovs_vport_get_stats()" fixes the OOPs,
> unmasking a full-on panic on arm64:
> =============%<==============
> [<ffffffbffc00ce4c>] internal_dev_recv+0xa8/0x170 [openvswitch]
> [<ffffffbffc0008b4>] do_output.isra.31+0x60/0x19c [openvswitch]
> [<ffffffbffc000bf8>] do_execute_actions+0x208/0x11c0 [openvswitch]
> [<ffffffbffc001c78>] ovs_execute_actions+0xc8/0x238 [openvswitch]
> [<ffffffbffc003dfc>] ovs_packet_cmd_execute+0x21c/0x288 [openvswitch]
> [<ffffffc0005e8c5c>] genl_family_rcv_msg+0x1b0/0x310
> [<ffffffc0005e8e60>] genl_rcv_msg+0xa4/0xe4
> [<ffffffc0005e7ddc>] netlink_rcv_skb+0xb0/0xdc
> [<ffffffc0005e8a94>] genl_rcv+0x38/0x50
> [<ffffffc0005e76c0>] netlink_unicast+0x164/0x210
> [<ffffffc0005e7b70>] netlink_sendmsg+0x304/0x368
> [<ffffffc0005a21c0>] sock_sendmsg+0x30/0x4c
> [SNIP]
> Kernel panic - not syncing: Fatal exception in interrupt
> =============%<==============
>
> Signed-off-by: James Morse <james.morse@arm.com>
> Fixes: 8c876639c985 ("openvswitch: Remove vport stats.")
> ---
> Hi netdev,
>
> "openvswitch: Fix ovs_vport_get_stats()"  - already applied according to
> [0] (where?), is an incomplete fix for the issue in "openvswitch: Remove
> vport stats.". Use of an unallocated dev->tstats remains for ovs internal
> devices, which causes a panic on arm64. Could this patch and "openvswitch:
> Fix ovs_vport_get_stats()" be considered as fixes for v4.3?
>
You need to target this patch for net branch. So that the fix will get
into v4.3.

> Thanks!
>
> James Morse
>
>
> [0] https://patchwork.ozlabs.org/patch/525827/
>
>  net/openvswitch/vport-internal_dev.c | 43 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c
> index 388b8a6bf112..758e53cb7a81 100644
> --- a/net/openvswitch/vport-internal_dev.c
> +++ b/net/openvswitch/vport-internal_dev.c
> @@ -106,12 +106,48 @@ static void internal_dev_destructor(struct net_device *dev)
>         free_netdev(dev);
>  }
>
> +static struct rtnl_link_stats64 *
> +internal_get_stats(struct net_device *dev, struct rtnl_link_stats64 *stats)
> +{
> +       int i;
> +
> +       memset(stats, 0, sizeof(*stats));
> +       stats->rx_errors  = dev->stats.rx_errors;
> +       stats->tx_errors  = dev->stats.tx_errors;
> +       stats->tx_dropped = dev->stats.tx_dropped;
> +       stats->rx_dropped = dev->stats.rx_dropped;
> +
> +       stats->rx_dropped += atomic_long_read(&dev->rx_dropped);
> +       stats->tx_dropped += atomic_long_read(&dev->tx_dropped);
> +
dev_get_stats() already account for dev->rx and tx dropped packets. So
there is no need to add it here.