[PATCHv2 0/3 net] xen-netback: fix ethtool stats and memory leak

[PATCHv2 0/3 net] xen-netback: fix ethtool stats and memory leak

[David Vrabel]

A couple of bug fixes for netback:
- make ethool stats to report the correct values.
- don't leak 1 MiB every time a VIF is destroyed.

Changes in v2:
- Split 2nd patch into leak fix and refactor patches

David

[PATCHv2 1/3] xen-netback: return correct ethtool stats

[David Vrabel]

Use correct pointer arithmetic to get the pointer to each stat.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/net/xen-netback/interface.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index f38227a..3aa8648 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -340,12 +340,11 @@ static void xenvif_get_ethtool_stats(struct net_device *dev,
 	unsigned int num_queues = vif->num_queues;
 	int i;
 	unsigned int queue_index;
-	struct xenvif_stats *vif_stats;
 
 	for (i = 0; i < ARRAY_SIZE(xenvif_stats); i++) {
 		unsigned long accum = 0;
 		for (queue_index = 0; queue_index < num_queues; ++queue_index) {
-			vif_stats = &vif->queues[queue_index].stats;
+			void *vif_stats = &vif->queues[queue_index].stats;
 			accum += *(unsigned long *)(vif_stats + xenvif_stats[i].offset);
 		}
 		data[i] = accum;
-- 
1.7.10.4

[PATCHv2 1/3] xen-netback: return correct ethtool stats

[David Vrabel]

Use correct pointer arithmetic to get the pointer to each stat.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/net/xen-netback/interface.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index f38227a..3aa8648 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -340,12 +340,11 @@ static void xenvif_get_ethtool_stats(struct net_device *dev,
 	unsigned int num_queues = vif->num_queues;
 	int i;
 	unsigned int queue_index;
-	struct xenvif_stats *vif_stats;
 
 	for (i = 0; i < ARRAY_SIZE(xenvif_stats); i++) {
 		unsigned long accum = 0;
 		for (queue_index = 0; queue_index < num_queues; ++queue_index) {
-			vif_stats = &vif->queues[queue_index].stats;
+			void *vif_stats = &vif->queues[queue_index].stats;
 			accum += *(unsigned long *)(vif_stats + xenvif_stats[i].offset);
 		}
 		data[i] = accum;
-- 
1.7.10.4

[PATCHv2 2/3] xen-netback: unref frags when handling a from-guest skb with a frag list

[David Vrabel]

Every time a VIF is destroyed up to 256 pages may be leaked if packets
with more than MAX_SKB_FRAGS frags were transmitted from the guest.
Even worse, if another user of ballooned pages allocated one of these
ballooned pages it would not handle the unexpectedly >1 page count
(e.g., gntdev would deadlock when unmapping a grant because the page
count would never reach 1).

When handling a from-guest skb with a frag list, unref the frags
before releasing them so they are freed correctly when the VIF is
destroyed.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/net/xen-netback/netback.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index f7a31d2..f5560cd 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1343,7 +1343,7 @@ static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *s
 {
 	unsigned int offset = skb_headlen(skb);
 	skb_frag_t frags[MAX_SKB_FRAGS];
-	int i;
+	int i, f;
 	struct ubuf_info *uarg;
 	struct sk_buff *nskb = skb_shinfo(skb)->frag_list;
 
@@ -1383,6 +1383,11 @@ static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *s
 		frags[i].page_offset = 0;
 		skb_frag_size_set(&frags[i], len);
 	}
+
+	/* Release all the original (foreign) frags. */
+	for (f = 0; f < skb_shinfo(skb)->nr_frags; f++)
+		skb_frag_unref(skb, f);
+
 	/* swap out with old one */
 	memcpy(skb_shinfo(skb)->frags,
 	       frags,
-- 
1.7.10.4

[PATCHv2 2/3] xen-netback: unref frags when handling a from-guest skb with a frag list

[David Vrabel]

Every time a VIF is destroyed up to 256 pages may be leaked if packets
with more than MAX_SKB_FRAGS frags were transmitted from the guest.
Even worse, if another user of ballooned pages allocated one of these
ballooned pages it would not handle the unexpectedly >1 page count
(e.g., gntdev would deadlock when unmapping a grant because the page
count would never reach 1).

When handling a from-guest skb with a frag list, unref the frags
before releasing them so they are freed correctly when the VIF is
destroyed.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/net/xen-netback/netback.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index f7a31d2..f5560cd 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1343,7 +1343,7 @@ static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *s
 {
 	unsigned int offset = skb_headlen(skb);
 	skb_frag_t frags[MAX_SKB_FRAGS];
-	int i;
+	int i, f;
 	struct ubuf_info *uarg;
 	struct sk_buff *nskb = skb_shinfo(skb)->frag_list;
 
@@ -1383,6 +1383,11 @@ static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *s
 		frags[i].page_offset = 0;
 		skb_frag_size_set(&frags[i], len);
 	}
+
+	/* Release all the original (foreign) frags. */
+	for (f = 0; f < skb_shinfo(skb)->nr_frags; f++)
+		skb_frag_unref(skb, f);
+
 	/* swap out with old one */
 	memcpy(skb_shinfo(skb)->frags,
 	       frags,
-- 
1.7.10.4

[PATCHv2 3/3] xen-netback: refactor xenvif_handle_frag_list()

[David Vrabel]

When handling a from-guest frag list, xenvif_handle_frag_list()
replaces the frags before calling the destructor to clean up the
original (foreign) frags.  Whilst this is safe (the destructor doesn't
actually use the frags), it looks odd.

Reorder the function to be less confusing.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/net/xen-netback/netback.c |   21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index f5560cd..56c4a81 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1384,27 +1384,24 @@ static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *s
 		skb_frag_size_set(&frags[i], len);
 	}
 
+	/* Copied all the bits from the frag list -- free it. */
+	skb_frag_list_init(skb);
+	xenvif_skb_zerocopy_prepare(queue, nskb);
+	kfree_skb(nskb);
+
 	/* Release all the original (foreign) frags. */
 	for (f = 0; f < skb_shinfo(skb)->nr_frags; f++)
 		skb_frag_unref(skb, f);
-
-	/* swap out with old one */
-	memcpy(skb_shinfo(skb)->frags,
-	       frags,
-	       i * sizeof(skb_frag_t));
-	skb_shinfo(skb)->nr_frags = i;
-	skb->truesize += i * PAGE_SIZE;
-
-	/* remove traces of mapped pages and frag_list */
-	skb_frag_list_init(skb);
 	uarg = skb_shinfo(skb)->destructor_arg;
 	/* increase inflight counter to offset decrement in callback */
 	atomic_inc(&queue->inflight_packets);
 	uarg->callback(uarg, true);
 	skb_shinfo(skb)->destructor_arg = NULL;
 
-	xenvif_skb_zerocopy_prepare(queue, nskb);
-	kfree_skb(nskb);
+	/* Fill the skb with the new (local) frags. */
+	memcpy(skb_shinfo(skb)->frags, frags, i * sizeof(skb_frag_t));
+	skb_shinfo(skb)->nr_frags = i;
+	skb->truesize += i * PAGE_SIZE;
 
 	return 0;
 }
-- 
1.7.10.4

[PATCHv2 3/3] xen-netback: refactor xenvif_handle_frag_list()

[David Vrabel]

When handling a from-guest frag list, xenvif_handle_frag_list()
replaces the frags before calling the destructor to clean up the
original (foreign) frags.  Whilst this is safe (the destructor doesn't
actually use the frags), it looks odd.

Reorder the function to be less confusing.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 drivers/net/xen-netback/netback.c |   21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index f5560cd..56c4a81 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1384,27 +1384,24 @@ static int xenvif_handle_frag_list(struct xenvif_queue *queue, struct sk_buff *s
 		skb_frag_size_set(&frags[i], len);
 	}
 
+	/* Copied all the bits from the frag list -- free it. */
+	skb_frag_list_init(skb);
+	xenvif_skb_zerocopy_prepare(queue, nskb);
+	kfree_skb(nskb);
+
 	/* Release all the original (foreign) frags. */
 	for (f = 0; f < skb_shinfo(skb)->nr_frags; f++)
 		skb_frag_unref(skb, f);
-
-	/* swap out with old one */
-	memcpy(skb_shinfo(skb)->frags,
-	       frags,
-	       i * sizeof(skb_frag_t));
-	skb_shinfo(skb)->nr_frags = i;
-	skb->truesize += i * PAGE_SIZE;
-
-	/* remove traces of mapped pages and frag_list */
-	skb_frag_list_init(skb);
 	uarg = skb_shinfo(skb)->destructor_arg;
 	/* increase inflight counter to offset decrement in callback */
 	atomic_inc(&queue->inflight_packets);
 	uarg->callback(uarg, true);
 	skb_shinfo(skb)->destructor_arg = NULL;
 
-	xenvif_skb_zerocopy_prepare(queue, nskb);
-	kfree_skb(nskb);
+	/* Fill the skb with the new (local) frags. */
+	memcpy(skb_shinfo(skb)->frags, frags, i * sizeof(skb_frag_t));
+	skb_shinfo(skb)->nr_frags = i;
+	skb->truesize += i * PAGE_SIZE;
 
 	return 0;
 }
-- 
1.7.10.4

Re: [PATCHv2 0/3 net] xen-netback: fix ethtool stats and memory leak

[Ian Campbell]

On Wed, 2015-03-04 at 11:14 +0000, David Vrabel wrote:

All three patches:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCHv2 0/3 net] xen-netback: fix ethtool stats and memory leak

[Ian Campbell]

On Wed, 2015-03-04 at 11:14 +0000, David Vrabel wrote:

All three patches:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCHv2 0/3 net] xen-netback: fix ethtool stats and memory leak

[David Miller]

From: David Vrabel <david.vrabel@citrix.com>
Date: Wed, 4 Mar 2015 11:14:45 +0000

> A couple of bug fixes for netback:
> - make ethool stats to report the correct values.
> - don't leak 1 MiB every time a VIF is destroyed.
> 
> Changes in v2:
> - Split 2nd patch into leak fix and refactor patches

Series applied, thanks.

Re: [PATCHv2 0/3 net] xen-netback: fix ethtool stats and memory leak

[David Miller]

From: David Vrabel <david.vrabel@citrix.com>
Date: Wed, 4 Mar 2015 11:14:45 +0000

> A couple of bug fixes for netback:
> - make ethool stats to report the correct values.
> - don't leak 1 MiB every time a VIF is destroyed.
> 
> Changes in v2:
> - Split 2nd patch into leak fix and refactor patches

Series applied, thanks.

Re: [PATCHv2 1/3] xen-netback: return correct ethtool stats

[Ian Campbell]

Control: fixed -1 4.0-1~exp1

On Wed, 2015-03-04 at 11:14 +0000, David Vrabel wrote:
> Use correct pointer arithmetic to get the pointer to each stat.

I think this incorrect arithmetic was also responsible for the crash
reported in http://bugs.debian.org/786936 which was using the resulting
stray pointer.

I'll add the fix to our kernel but: David (Miller) could we also have it
queued for stable please?

Thanks.

Reasoning:

IP: [<ffffffffa06802a0>] xenvif_get_ethtool_stats+0x50/0x80 [xen_netback]

(gdb) disas xenvif_get_ethtool_stats+0x50
Dump of assembler code for function xenvif_get_ethtool_stats:
   0x0000000000005280 <+0>:	callq  0x5285 <xenvif_get_ethtool_stats+5>
   0x0000000000005285 <+5>:	mov    0x900(%rdi),%r9d
   0x000000000000528c <+12>:	mov    $0x0,%r8
   0x0000000000005293 <+19>:	lea    -0x1(%r9),%r10d
   0x0000000000005297 <+23>:	imul   $0x36258,%r10,%r10
   0x000000000000529e <+30>:	xchg   %ax,%ax
   0x00000000000052a0 <+32>:	test   %r9d,%r9d
   0x00000000000052a3 <+35>:	je     0x52f8 <xenvif_get_ethtool_stats+120>
   0x00000000000052a5 <+37>:	movzwl (%r8),%esi
   0x00000000000052a9 <+41>:	mov    0x8f8(%rdi),%rcx
   0x00000000000052b0 <+48>:	lea    0x0(,%rsi,8),%rax
   0x00000000000052b8 <+56>:	shl    $0x6,%rsi
   0x00000000000052bc <+60>:	sub    %rax,%rsi
   0x00000000000052bf <+63>:	lea    (%rcx,%rsi,1),%rax
   0x00000000000052c3 <+67>:	lea    0x36258(%rcx,%r10,1),%rcx
   0x00000000000052cb <+75>:	add    %rcx,%rsi
   0x00000000000052ce <+78>:	xor    %ecx,%ecx
   0x00000000000052d0 <+80>:	add    0x36220(%rax),%rcx
   0x00000000000052d7 <+87>:	add    $0x36258,%rax
   0x00000000000052dd <+93>:	cmp    %rsi,%rax
   0x00000000000052e0 <+96>:	jne    0x52d0 <xenvif_get_ethtool_stats+80>
   0x00000000000052e2 <+98>:	add    $0x22,%r8
   0x00000000000052e6 <+102>:	mov    %rcx,(%rdx)
   0x00000000000052e9 <+105>:	add    $0x8,%rdx
   0x00000000000052ed <+109>:	cmp    $0x0,%r8
   0x00000000000052f4 <+116>:	jne    0x52a0 <xenvif_get_ethtool_stats+32>
   0x00000000000052f6 <+118>:	repz retq 
   0x00000000000052f8 <+120>:	xor    %ecx,%ecx
   0x00000000000052fa <+122>:	jmp    0x52e2 <xenvif_get_ethtool_stats+98>
End of assembler dump.
(gdb) list *xenvif_get_ethtool_stats+0x50
0x52d0 is in xenvif_get_ethtool_stats (/build/linux-RGM_Ed/linux-3.16.7-ckt9/drivers/net/xen-netback/interface.c:349).

... and in the Debian kernel interface.c:349 is the accum += line from
the patch.

Ian.

> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> ---
>  drivers/net/xen-netback/interface.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
> index f38227a..3aa8648 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -340,12 +340,11 @@ static void xenvif_get_ethtool_stats(struct net_device *dev,
>  	unsigned int num_queues = vif->num_queues;
>  	int i;
>  	unsigned int queue_index;
> -	struct xenvif_stats *vif_stats;
>  
>  	for (i = 0; i < ARRAY_SIZE(xenvif_stats); i++) {
>  		unsigned long accum = 0;
>  		for (queue_index = 0; queue_index < num_queues; ++queue_index) {
> -			vif_stats = &vif->queues[queue_index].stats;
> +			void *vif_stats = &vif->queues[queue_index].stats;
>  			accum += *(unsigned long *)(vif_stats + xenvif_stats[i].offset);
>  		}
>  		data[i] = accum;