Skip to contentSkip to conversation

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
#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
#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]);
author
#2 lore raw
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.

maintainer
#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.

Acked-by on v1 1/1
#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;
maintainer
#5 lore raw

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.

Acked-by on v1 1/1
#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]>