pktgen: fix transmission headers with frags=0

PATCH In review Daniel Turull 3 participants 7 messages Mar 14, 2011

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Daniel Turull

author

2011-03-14 13:22 UTC #0 lore raw

PATCH 1/1 pktgen: fix transmission headers with frags=0

The headers of pktgen were incorrectly added in a pktgen packet without frags (frags=0). There was an offset in the pktgen headers.

The cause was in reusing the pgh variable as a return variable in skb_put when adding the payload to the skb.

A rename of the variable is done.

Signed-off-by Daniel Turull <[email protected]>

---
The PKTGEN magic (be9b e955) now starts in the correct offset. Before the patch, it was starting at the end of the packet (be9b)

Capture from tcpdump:

before patch:
14:57:37.854812 IP 10.0.0.2.discard > 10.254.254.84.discard: UDP, length 18

	0x0000:  001b 2157 ed84 001b 215d 01d0 0800 4500
	0x0010:  002e 0004 0000 2011 8767 0a00 0002 0afe
	0x0020:  fe54 0009 0009 001a 0000 0000 0000 b072
	0x0030:  9102 00ea ffff 0010 0000 be9b

after patch:
14:44:32.896048 IP 10.0.0.2.discard > 10.217.234.56.discard: UDP, length 18

	0x0000:  001b 2157 ed84 001b 215d 01d0 0800 4500
	0x0010:  002e 0000 0000 2011 9bac 0a00 0002 0ad9
	0x0020:  ea38 0009 0009 001a 0000 be9b e955 0000
	0x0030:  0001 4d7e 1b09 0005 5f23 af00

---

net/core/pktgen.c +3 -2

index f0aec6c..5baa9d9 100644
@@ -2615,13 +2615,14 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 {
 	struct timeval timestamp;
 	struct pktgen_hdr *pgh;
+	void *data;
 
 	pgh = (struct pktgen_hdr *)skb_put(skb, sizeof(*pgh));
 	datalen -= sizeof(*pgh);
 
 	if (pkt_dev->nfrags <= 0) {
-		pgh = (struct pktgen_hdr *)skb_put(skb, datalen);
-		memset(pgh + 1, 0, datalen);
+		data = skb_put(skb, datalen);
+		memset(data + 1, 0, datalen);
 	} else {
 		int frags = pkt_dev->nfrags;
 		int i, len;

Eric Dumazet

2011-03-14 13:56 UTC #1 lore raw

Quoted 6 lines · Daniel Turull

The headers of pktgen were incorrectly added in a pktgen packet without frags (frags=0). There was an offset in the pktgen headers.
The cause was in reusing the pgh variable as a return variable in skb_put when adding the payload to the skb.

Quoted 7 lines

A rename of the variable is done.
Signed-off-by Daniel Turull <[email protected]>
---
The PKTGEN magic (be9b e955) now starts in the correct offset. Before the patch, it was starting at the end of the packet (be9b)

not exactly, but offseted +16 bytes
(sizeof(struct pktgen_hdr))

Quoted 29 lines

Capture from tcpdump:

before patch:
14:57:37.854812 IP 10.0.0.2.discard > 10.254.254.84.discard: UDP, length 18

	0x0000:  001b 2157 ed84 001b 215d 01d0 0800 4500
	0x0010:  002e 0004 0000 2011 8767 0a00 0002 0afe
	0x0020:  fe54 0009 0009 001a 0000 0000 0000 b072
	0x0030:  9102 00ea ffff 0010 0000 be9b

after patch:
14:44:32.896048 IP 10.0.0.2.discard > 10.217.234.56.discard: UDP, length 18

	0x0000:  001b 2157 ed84 001b 215d 01d0 0800 4500
	0x0010:  002e 0000 0000 2011 9bac 0a00 0002 0ad9
	0x0020:  ea38 0009 0009 001a 0000 be9b e955 0000
	0x0030:  0001 4d7e 1b09 0005 5f23 af00

---

net/core/pktgen.c +1 -0

index f0aec6c..5baa9d9 100644
@@ -2615,13 +2615,14 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 {
 	struct timeval timestamp;
 	struct pktgen_hdr *pgh;
+	void *data;
 
 	pgh = (struct pktgen_hdr *)skb_put(skb, sizeof(*pgh));
 	datalen -= sizeof(*pgh);

net/core/pktgen.c static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,

Quoted 10 lines

net/core/pktgen.c +2 -3

 
 	if (pkt_dev->nfrags <= 0) {
-		pgh = (struct pktgen_hdr *)skb_put(skb, datalen);
-		memset(pgh + 1, 0, datalen);
+		data = skb_put(skb, datalen);
+		memset(data + 1, 0, datalen);
 	} else {
 		int frags = pkt_dev->nfrags;
 		int i, len;
--

Good catch !

Hmm this patch is not correct, why memset(data + 1, ...) ?

Also, this patch is needed for net-next-2.6 only (bug introduced by commit 26ad787962ef84677a48c560 (pktgen: speedup fragmented skbs)

I would avoid the "void *data;" declaration and just use following :

net/core/pktgen.c +1 -1

index f0aec6c..f727c83 100644
@@ -2620,7 +2620,7 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 	datalen -= sizeof(*pgh);
 
 	if (pkt_dev->nfrags <= 0) {
-		pgh = (struct pktgen_hdr *)skb_put(skb, datalen);
+		skb_put(skb, datalen);
 		memset(pgh + 1, 0, datalen);
 	} else {
 		int frags = pkt_dev->nfrags;

or even :

		memset(skb_put(skb, datalen), 0, datalen);

Daniel Turull

author

2011-03-14 14:14 UTC #2 lore raw

Quoted 9 lines · Eric Dumazet

Quoted 8 lines · Daniel Turull
The headers of pktgen were incorrectly added in a pktgen packet without frags (frags=0). There was an offset in the pktgen headers.
The cause was in reusing the pgh variable as a return variable in skb_put when adding the payload to the skb.
A rename of the variable is done.

Quoted 8 lines

Quoted 5 lines
Signed-off-by Daniel Turull <[email protected]>
---
The PKTGEN magic (be9b e955) now starts in the correct offset. Before the patch, it was starting at the end of the packet (be9b)
not exactly, but offseted +16 bytes
(sizeof(struct pktgen_hdr))

True, I shouldn't say end.

Quoted 43 lines

Quoted 30 lines

Capture from tcpdump:

before patch:
14:57:37.854812 IP 10.0.0.2.discard > 10.254.254.84.discard: UDP, length 18

	0x0000:  001b 2157 ed84 001b 215d 01d0 0800 4500
	0x0010:  002e 0004 0000 2011 8767 0a00 0002 0afe
	0x0020:  fe54 0009 0009 001a 0000 0000 0000 b072
	0x0030:  9102 00ea ffff 0010 0000 be9b

after patch:
14:44:32.896048 IP 10.0.0.2.discard > 10.217.234.56.discard: UDP, length 18

	0x0000:  001b 2157 ed84 001b 215d 01d0 0800 4500
	0x0010:  002e 0000 0000 2011 9bac 0a00 0002 0ad9
	0x0020:  ea38 0009 0009 001a 0000 be9b e955 0000
	0x0030:  0001 4d7e 1b09 0005 5f23 af00

---

net/core/pktgen.c +1 -0

index f0aec6c..5baa9d9 100644
@@ -2615,13 +2615,14 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 {
 	struct timeval timestamp;
 	struct pktgen_hdr *pgh;
+	void *data;
 
 	pgh = (struct pktgen_hdr *)skb_put(skb, sizeof(*pgh));
 	datalen -= sizeof(*pgh);

net/core/pktgen.c static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,

Quoted 10 lines

net/core/pktgen.c +2 -3

 
 	if (pkt_dev->nfrags <= 0) {
-		pgh = (struct pktgen_hdr *)skb_put(skb, datalen);
-		memset(pgh + 1, 0, datalen);
+		data = skb_put(skb, datalen);
+		memset(data + 1, 0, datalen);
 	} else {
 		int frags = pkt_dev->nfrags;
 		int i, len;
--

Good catch !

Quoted 1 line

Hmm this patch is not correct, why memset(data + 1, ...) ?

I kept the +1 as it was in the original code, but I supposed it can be avoided.

Quoted 22 lines

Also, this patch is needed for net-next-2.6 only (bug introduced by commit 26ad787962ef84677a48c560 (pktgen: speedup fragmented skbs)
I would avoid the "void *data;" declaration and just use following :
net/core/pktgen.c +1 -1
index f0aec6c..f727c83 100644
@@ -2620,7 +2620,7 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 	datalen -= sizeof(*pgh);
 
 	if (pkt_dev->nfrags <= 0) {
-		pgh = (struct pktgen_hdr *)skb_put(skb, datalen);
+		skb_put(skb, datalen);
 		memset(pgh + 1, 0, datalen);
 	} else {
 		int frags = pkt_dev->nfrags;

Quoted 4 lines

or even :

		memset(skb_put(skb, datalen), 0, datalen);

I think that the second is better and more compact.

Do I resend the new patch?

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

2011-03-14 14:19 UTC #3 lore raw

Quoted 8 lines · Daniel Turull

Quoted 3 lines · Eric Dumazet
Good catch !
Hmm this patch is not correct, why memset(data + 1, ...) ?
I kept the +1 as it was in the original code, but I supposed it can be avoided.

original code was assuming pgh was at right location.

pgh + 1 : really pointed to first byte after pktgen header.

data + 1 : points one byte off (so we leak one byte of kernel memory, security guys would complain a lot...)

Quoted 3 lines

I think that the second is better and more compact.
Do I resend the new patch?

Yes please ;)

Daniel Turull

author

2011-03-14 14:31 UTC #4 lore raw

PATCH 1/1 pktgen: bug fix in transmission headers with frags=0

(bug introduced by commit 26ad787962ef84677a48c560 (pktgen: speedup fragmented skbs)

The headers of pktgen were incorrectly added in a pktgen packet without frags (frags=0). There was an offset in the pktgen headers.

The cause was in reusing the pgh variable as a return variable in skb_put when adding the payload to the skb.

Signed-off-by Daniel Turull <[email protected]>

---
The PKTGEN magic (be9b e955) now starts in the correct offset. Before the patch, it was starting with an offset of +16 bytes (sizeof(struct pktgen_hdr)) (be9b)

Capture from tcpdump:

before patch:
14:57:37.854812 IP 10.0.0.2.discard > 10.254.254.84.discard: UDP, length 18

	0x0000:  001b 2157 ed84 001b 215d 01d0 0800 4500
	0x0010:  002e 0004 0000 2011 8767 0a00 0002 0afe
	0x0020:  fe54 0009 0009 001a 0000 0000 0000 b072
	0x0030:  9102 00ea ffff 0010 0000 be9b

after patch:
14:44:32.896048 IP 10.0.0.2.discard > 10.217.234.56.discard: UDP, length 18

	0x0000:  001b 2157 ed84 001b 215d 01d0 0800 4500
	0x0010:  002e 0000 0000 2011 9bac 0a00 0002 0ad9
	0x0020:  ea38 0009 0009 001a 0000 be9b e955 0000
	0x0030:  0001 4d7e 1b09 0005 5f23 af00

---

net/core/pktgen.c +1 -2

index f0aec6c..0c55eaa 100644
@@ -2620,8 +2620,7 @@ static void pktgen_finalize_skb(struct pktgen_dev *pkt_dev, struct sk_buff *skb,
 	datalen -= sizeof(*pgh);
 
 	if (pkt_dev->nfrags <= 0) {
-		pgh = (struct pktgen_hdr *)skb_put(skb, datalen);
-		memset(pgh + 1, 0, datalen);
+		memset(skb_put(skb, datalen), 0, datalen);
 	} else {
 		int frags = pkt_dev->nfrags;
 		int i, len;

Eric Dumazet

2011-03-14 14:35 UTC #5 lore raw

Re: [PATCH net-next-2.6] pktgen: bug fix in transmission headers with frags=0

Quoted 10 lines · Daniel Turull

(bug introduced by commit 26ad787962ef84677a48c560 (pktgen: speedup fragmented skbs)
The headers of pktgen were incorrectly added in a pktgen packet without frags (frags=0). There was an offset in the pktgen headers.
The cause was in reusing the pgh variable as a return variable in skb_put when adding the payload to the skb.
Signed-off-by Daniel Turull <[email protected]>

Signed-off-by Eric Dumazet <[email protected]>

Thanks !

David Miller

maintainer Applied

2011-03-14 20:47 UTC #6 lore raw

Re: [PATCH net-next-2.6] pktgen: bug fix in transmission headers with frags=0

From: Eric Dumazet <[email protected]>
Date: Mon, 14 Mar 2011 15:35:50 +0100

Quoted 7 lines · Daniel Turull

Quoted 6 lines
(bug introduced by commit 26ad787962ef84677a48c560 (pktgen: speedup fragmented skbs)
The headers of pktgen were incorrectly added in a pktgen packet without frags (frags=0). There was an offset in the pktgen headers.

Quoted 6 lines

Quoted 4 lines
The cause was in reusing the pgh variable as a return variable in skb_put when adding the payload to the skb.
Signed-off-by Daniel Turull <[email protected]>
Signed-off-by Eric Dumazet <[email protected]>

Applied, thanks everyone.