netdev - Re: [PATCH] pktgen: fix transmission headers with frags=0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4D7E22A5.1000708@gmail.com>
Date:	Mon, 14 Mar 2011 15:13:57 +0100
From:	Daniel Turull <daniel.turull@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	netdev@...r.kernel.org, Robert Olsson <robert@...julf.net>,
	Jens Laas <jens.laas@....uu.se>,
	Voravit Tanyingyong <voravit@....se>
Subject: Re: [PATCH] pktgen: fix transmission headers with frags=0

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@...il.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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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