* [PATCH] pktgen: fix transmission headers with frags=0 @ 2011-03-14 13:22 Daniel Turull 2011-03-14 13:56 ` Eric Dumazet 0 siblings, 1 reply; 7+ messages in thread From: Daniel Turull @ 2011-03-14 13:22 UTC (permalink / raw) To: netdev; +Cc: Robert Olsson, Jens Laas, Voravit Tanyingyong, 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. Signed-off-by: Daniel Turull <daniel.turull@gmail.com> --- 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 --- diff --git a/net/core/pktgen.c b/net/core/pktgen.c index f0aec6c..5baa9d9 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -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; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pktgen: fix transmission headers with frags=0 2011-03-14 13:22 [PATCH] pktgen: fix transmission headers with frags=0 Daniel Turull @ 2011-03-14 13:56 ` Eric Dumazet 2011-03-14 14:13 ` Daniel Turull 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2011-03-14 13:56 UTC (permalink / raw) To: Daniel Turull; +Cc: netdev, Robert Olsson, Jens Laas, Voravit Tanyingyong Le lundi 14 mars 2011 à 14:22 +0100, Daniel Turull a écrit : > 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 <daniel.turull@gmail.com> > --- > 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)) > 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 > > --- > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index f0aec6c..5baa9d9 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -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; > -- 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 : diff --git a/net/core/pktgen.c b/net/core/pktgen.c index f0aec6c..f727c83 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -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); ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] pktgen: fix transmission headers with frags=0 2011-03-14 13:56 ` Eric Dumazet @ 2011-03-14 14:13 ` Daniel Turull 2011-03-14 14:18 ` Eric Dumazet 0 siblings, 1 reply; 7+ messages in thread From: Daniel Turull @ 2011-03-14 14:13 UTC (permalink / raw) To: Eric Dumazet; +Cc: netdev, Robert Olsson, Jens Laas, Voravit Tanyingyong On 03/14/2011 02:56 PM, Eric Dumazet wrote: > Le lundi 14 mars 2011 à 14:22 +0100, Daniel Turull a écrit : >> 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 <daniel.turull@gmail.com> >> --- >> 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. > >> 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 >> >> --- >> diff --git a/net/core/pktgen.c b/net/core/pktgen.c >> index f0aec6c..5baa9d9 100644 >> --- a/net/core/pktgen.c >> +++ b/net/core/pktgen.c >> @@ -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; >> -- > > 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. > 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 : > > diff --git a/net/core/pktgen.c b/net/core/pktgen.c > index f0aec6c..f727c83 100644 > --- a/net/core/pktgen.c > +++ b/net/core/pktgen.c > @@ -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); > I think that the second is better and more compact. Do I resend the new patch? > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] pktgen: fix transmission headers with frags=0 2011-03-14 14:13 ` Daniel Turull @ 2011-03-14 14:18 ` Eric Dumazet 2011-03-14 14:31 ` [PATCH net-next-2.6] pktgen: bug fix in " Daniel Turull 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2011-03-14 14:18 UTC (permalink / raw) To: Daniel Turull; +Cc: netdev, Robert Olsson, Jens Laas, Voravit Tanyingyong Le lundi 14 mars 2011 à 15:13 +0100, Daniel Turull a écrit : > On 03/14/2011 02:56 PM, Eric Dumazet wrote: > > 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...) > I think that the second is better and more compact. > > Do I resend the new patch? Yes please ;) ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next-2.6] pktgen: bug fix in transmission headers with frags=0 2011-03-14 14:18 ` Eric Dumazet @ 2011-03-14 14:31 ` Daniel Turull 2011-03-14 14:35 ` Eric Dumazet 0 siblings, 1 reply; 7+ messages in thread From: Daniel Turull @ 2011-03-14 14:31 UTC (permalink / raw) To: Eric Dumazet; +Cc: netdev, Robert Olsson, Jens Laas, Voravit Tanyingyong (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 <daniel.turull@gmail.com> --- 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 --- diff --git a/net/core/pktgen.c b/net/core/pktgen.c index f0aec6c..0c55eaa 100644 --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -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; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next-2.6] pktgen: bug fix in transmission headers with frags=0 2011-03-14 14:31 ` [PATCH net-next-2.6] pktgen: bug fix in " Daniel Turull @ 2011-03-14 14:35 ` Eric Dumazet 2011-03-14 20:47 ` David Miller 0 siblings, 1 reply; 7+ messages in thread From: Eric Dumazet @ 2011-03-14 14:35 UTC (permalink / raw) To: Daniel Turull; +Cc: netdev, Robert Olsson, Jens Laas, Voravit Tanyingyong Le lundi 14 mars 2011 à 15:31 +0100, Daniel Turull a écrit : > (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 <daniel.turull@gmail.com> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Thanks ! ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next-2.6] pktgen: bug fix in transmission headers with frags=0 2011-03-14 14:35 ` Eric Dumazet @ 2011-03-14 20:47 ` David Miller 0 siblings, 0 replies; 7+ messages in thread From: David Miller @ 2011-03-14 20:47 UTC (permalink / raw) To: eric.dumazet; +Cc: daniel.turull, netdev, robert, jens.laas, voravit From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 14 Mar 2011 15:35:50 +0100 > Le lundi 14 mars 2011 à 15:31 +0100, Daniel Turull a écrit : >> (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 <daniel.turull@gmail.com> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Applied, thanks everyone. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-03-14 20:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-14 13:22 [PATCH] pktgen: fix transmission headers with frags=0 Daniel Turull 2011-03-14 13:56 ` Eric Dumazet 2011-03-14 14:13 ` Daniel Turull 2011-03-14 14:18 ` Eric Dumazet 2011-03-14 14:31 ` [PATCH net-next-2.6] pktgen: bug fix in " Daniel Turull 2011-03-14 14:35 ` Eric Dumazet 2011-03-14 20:47 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).