tun: fix recovery from gup errors

PATCH In review Michael S. Tsirkin 5 participants 7 messages Jun 23, 2013 – Jun 25, 2013

Subsystems (3)

Networking Network drivers TUN/TAP driver

lore

Share & tools

lore

Michael S. Tsirkin

author

2013-06-23 14:18 UTC #0 lore raw

PATCH 1/1 tun: fix recovery from gup errors

get user pages might fail partially in tun zero copy mode. To recover we need to put all pages that we got, but code used a wrong index resulting in double-free errors.

Reported-by Brad Hubbard <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---

I haven't figured out why do we get failures, but recovery is clearly wrong.

This is also -stable material.

 drivers/net/tun.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/net/tun.c +3 -2

index bfa9bb4..c098b1e 100644
@@ -1010,8 +1010,9 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
 			return -EMSGSIZE;
 		num_pages = get_user_pages_fast(base, size, 0, &page[i]);
 		if (num_pages != size) {
-			for (i = 0; i < num_pages; i++)
-				put_page(page[i]);
+			int j;
+			for (j = 0; j < num_pages; j++)
+				put_page(page[i + j]);
 			return -EFAULT;
 		}
 		truesize = size * PAGE_SIZE;

Signature · 1 line

MST

Sergei Shtylyov

2013-06-23 15:36 UTC #1 lore raw

Hello.

Quoted 22 lines · Michael S. Tsirkin

get user pages might fail partially in tun zero copy mode. To recover we need to put all pages that we got, but code used a wrong index resulting in double-free errors.
Reported-by Brad Hubbard <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
I haven't figured out why do we get failures, but recovery is clearly wrong.
This is also -stable material.
  drivers/net/tun.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/tun.c +0 -0
index bfa9bb4..c098b1e 100644

drivers/net/tun.c static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from, · line 1013

Quoted 6 lines

drivers/net/tun.c +1 -2

@@ -1010,8 +1010,9 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
  			return -EMSGSIZE;
  		num_pages = get_user_pages_fast(base, size, 0, &page[i]);
  		if (num_pages != size) {
-			for (i = 0; i < num_pages; i++)
-				put_page(page[i]);
+			int j;

Empty line wouldn't hurt here, after declaration.

Quoted 2 lines

drivers/net/tun.c +2 -0

+			for (j = 0; j < num_pages; j++)
+				put_page(page[i + j]);

Jason Wang

Acked-by on v1 1/1

2013-06-24 03:22 UTC #4 lore raw

In reply to Michael S. Tsirkin

Quoted 5 lines · Michael S. Tsirkin

get user pages might fail partially in tun zero copy mode. To recover we need to put all pages that we got, but code used a wrong index resulting in double-free errors.

Quoted 8 lines

Reported-by Brad Hubbard <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
I haven't figured out why do we get failures, but recovery is clearly wrong.
This is also -stable material.

Acked-by Jason Wang <[email protected]>

Quoted 19 lines

 drivers/net/tun.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/net/tun.c +3 -2

index bfa9bb4..c098b1e 100644
@@ -1010,8 +1010,9 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
 			return -EMSGSIZE;
 		num_pages = get_user_pages_fast(base, size, 0, &page[i]);
 		if (num_pages != size) {
-			for (i = 0; i < num_pages; i++)
-				put_page(page[i]);
+			int j;
+			for (j = 0; j < num_pages; j++)
+				put_page(page[i + j]);
 			return -EFAULT;
 		}
 		truesize = size * PAGE_SIZE;

Neil Horman

Acked-by on v1 1/1

2013-06-24 11:31 UTC #6 lore raw

In reply to Michael S. Tsirkin

Quoted 9 lines · Michael S. Tsirkin

get user pages might fail partially in tun zero copy mode. To recover we need to put all pages that we got, but code used a wrong index resulting in double-free errors.
Reported-by Brad Hubbard <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---

Quoted 8 lines

I haven't figured out why do we get failures, but recovery is clearly wrong.
This is also -stable material.
 drivers/net/tun.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Acked-by Neil Horman <[email protected]>

Michael S. Tsirkin

author

2013-06-24 12:53 UTC #2 lore raw

In reply to Sergei Shtylyov

Quoted 35 lines · Sergei Shtylyov

Hello.
Quoted 22 lines · Michael S. Tsirkin
get user pages might fail partially in tun zero copy mode. To recover we need to put all pages that we got, but code used a wrong index resulting in double-free errors.
Reported-by Brad Hubbard <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
I haven't figured out why do we get failures, but recovery is clearly wrong.
This is also -stable material.
 drivers/net/tun.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/tun.c +0 -0
index bfa9bb4..c098b1e 100644
drivers/net/tun.c static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from, · line 1013
Quoted 6 lines
drivers/net/tun.c +1 -2
@@ -1010,8 +1010,9 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
 			return -EMSGSIZE;
 		num_pages = get_user_pages_fast(base, size, 0, &page[i]);
 		if (num_pages != size) {
-			for (i = 0; i < num_pages; i++)
-				put_page(page[i]);
+			int j;
Empty line wouldn't hurt here, after declaration.

Quoted 2 lines

drivers/net/tun.c static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from, · line 1015
Quoted 2 lines
drivers/net/tun.c +2 -0
+			for (j = 0; j < num_pages; j++)
+				put_page(page[i + j]);

I think it's clearer without: this is the only code within this block, declaration is really part of the loop that comes after it.
An empty line would break it up visually.

David Miller

maintainer

2013-06-24 18:34 UTC #3 lore raw

From: "Michael S. Tsirkin" <[email protected]>
Date: Mon, 24 Jun 2013 15:54:12 +0300

Quoted 36 lines · Sergei Shtylyov

Quoted 35 lines
Hello.
Quoted 22 lines · Michael S. Tsirkin
get user pages might fail partially in tun zero copy mode. To recover we need to put all pages that we got, but code used a wrong index resulting in double-free errors.
Reported-by Brad Hubbard <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---
I haven't figured out why do we get failures, but recovery is clearly wrong.
This is also -stable material.
 drivers/net/tun.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/tun.c +0 -0
index bfa9bb4..c098b1e 100644
drivers/net/tun.c static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from, · line 1013
Quoted 6 lines
drivers/net/tun.c +1 -2
@@ -1010,8 +1010,9 @@ static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
 			return -EMSGSIZE;
 		num_pages = get_user_pages_fast(base, size, 0, &page[i]);
 		if (num_pages != size) {
-			for (i = 0; i < num_pages; i++)
-				put_page(page[i]);
+			int j;
Empty line wouldn't hurt here, after declaration.

Quoted 7 lines

Quoted 2 lines
drivers/net/tun.c static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from, · line 1015
Quoted 2 lines
drivers/net/tun.c +2 -0
+			for (j = 0; j < num_pages; j++)
+				put_page(page[i + j]);
I think it's clearer without: this is the only code within this block, declaration is really part of the loop that comes after it.
An empty line would break it up visually.

No, really, an empty line after local variable declarations please.

David Miller

maintainer

2013-06-25 23:24 UTC #5 lore raw

In reply to Jason Wang

From: Jason Wang <[email protected]>
Date: Mon, 24 Jun 2013 11:22:52 +0800

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines
get user pages might fail partially in tun zero copy mode. To recover we need to put all pages that we got, but code used a wrong index resulting in double-free errors.
Reported-by Brad Hubbard <[email protected]>
Signed-off-by Michael S. Tsirkin <[email protected]>
---

Quoted 6 lines

Quoted 4 lines
I haven't figured out why do we get failures, but recovery is clearly wrong.
This is also -stable material.
Acked-by Jason Wang <[email protected]>

Applied with the missing empty line added, I was tired of waiting for Michael to take care of this himself.