[U-Boot] Bug in IP/UDP defragment?

[U-Boot] Bug in IP/UDP defragment?

[Fillod Stephane]

Hi,

I'm encountering a problem when tftp'ing a file of size 1747851 bytes
with 
CONFIG_IP_DEFRAG defined and CONFIG_TFTP_BLOCKSIZE set to 4096. U-Boot's
tftp 
times out on the last chunk. Undefining
CONFIG_IP_DEFRAG/CONFIG_TFTP_BLOCKSIZE
or downloading with a Linux tftp client are all well, so it does not
look like
a server problem. 

In fact, a file size between 1747845 and 1747851 bytes (incl.) makes the
tftp
client to timeout. File sizes slightly below or higher than the before 
mentioned range are fine.

tcpdump trace of the last chunk yielding a timeout:

16:58:32.289427 IP 10.12.48.32.3285 > 10.12.48.10.33088: UDP, length 4
16:58:32.289449 IP 10.12.48.10.33088 > 10.12.48.32.3285: UDP, length
4100
16:58:32.289452 IP 10.12.48.10 > 10.12.48.32: udp
16:58:32.289454 IP 10.12.48.10 > 10.12.48.32: udp
16:58:32.290386 IP 10.12.48.32.3285 > 10.12.48.10.33088: UDP, length 4
16:58:32.290407 IP 10.12.48.10.33088 > 10.12.48.32.3285: UDP, length
2959
16:58:32.290410 IP 10.12.48.10 > 10.12.48.32: udp
16:58:32.290412 IP 10.12.48.10 > 10.12.48.32: udp


The patch below appears to solve my problem. WARNING: since I haven't
rolled 
my mind yet around the NetDefragment IP/UDP stack, I don't know yet what
are
the side effects of my patch.

--- a/net/net.c	31 Mar 2010 21:54:39
+++ b/net/net.c	4 Jun 2010 15:09:08
@@ -1201,7 +1201,7 @@ static IP_t *__NetDefragment(IP_t *ip, i
 		h = payload + h->next_hole;
 	}

-	if (offset8 + (len / 8) <= h - payload) {
+	if (offset8 + (len / 8) < h - payload) {
 		/* no overlap with holes (dup fragment?) */
 		return NULL;
 	}

The same problem should exist for NFS fragments.
The arch is powerpc, U-Boot version 2010.03 (no fix seen in 2010.06-rc).

Best Regards
-- 
Stephane

[U-Boot] Bug in IP/UDP defragment?

[Alessandro Rubini]

Hello.
Please forgive my delay.

> 16:58:32.290407 IP 10.12.48.10.33088 > 10.12.48.32.3285: UDP, length 2959
> 16:58:32.290410 IP 10.12.48.10 > 10.12.48.32: udp
> 16:58:32.290412 IP 10.12.48.10 > 10.12.48.32: udp

The third fragment here is less than 8 bytes of payload, and this
triggers the bug.

> I don't know yet what are the side effects of my patch.

Ok, I restudied the flow.

> -	if (offset8 + (len / 8) <= h - payload) {
> +	if (offset8 + (len / 8) < h - payload) {
>  		/* no overlap with holes (dup fragment?) */
>  		return NULL;

offset8 is the start of this fragment, as 8-byte-items, and h is the
current hole, the one that might be relevant. Since everything before
"h" is already filled, a fragment that ends where this hole starts
is duplicate, thus "<=". 

Here len/8 is 0, so you force accepting this trailing fragment, but
you'll also accept duplicate packets.  Looking@how following code
behaves in this case (it assumes some hole or part of is filled),
the "overlaps with initial part of the hole" will trigger, and the
hole will be rewritten unchanged.

So I don't see bad side effects in your fix, but if anyone will re-read
the code (looking for another bug, for example) after your change it will
be hard to understand (they'll suggest reverting the change for example).

I'd suggest to still discard duplicate fragments, by turning

	if (offset8 + (len / 8) <= h - payload)

to
	/* last fragment may be 1..7 bytes, the "+7" forces acceptance */
	if (offset8 + ((len + 7) / 8) <= h - payload)

or something along these lines. Will you please post a patch?

Thanks a lof for your report and fix

/alessandro

[U-Boot] Bug in IP/UDP defragment?

[Wolfgang Denk]

Dear "Fillod Stephane",

In message <0B45E93C5FF65740AEAE690BF3848B7A02087B60@rennsmail04.eu.thmulti.com> you wrote:
> 
> I'm encountering a problem when tftp'ing a file of size 1747851 bytes
> with 
> CONFIG_IP_DEFRAG defined and CONFIG_TFTP_BLOCKSIZE set to 4096. U-Boot's
> tftp 
> times out on the last chunk. Undefining
> CONFIG_IP_DEFRAG/CONFIG_TFTP_BLOCKSIZE
> or downloading with a Linux tftp client are all well, so it does not
> look like
> a server problem. 
>... 
> The patch below appears to solve my problem. WARNING: since I haven't
> rolled 
> my mind yet around the NetDefragment IP/UDP stack, I don't know yet what
> are
> the side effects of my patch.
> 
> --- a/net/net.c	31 Mar 2010 21:54:39
> +++ b/net/net.c	4 Jun 2010 15:09:08
> @@ -1201,7 +1201,7 @@ static IP_t *__NetDefragment(IP_t *ip, i
>  		h = payload + h->next_hole;
>  	}
> 
> -	if (offset8 + (len / 8) <= h - payload) {
> +	if (offset8 + (len / 8) < h - payload) {
>  		/* no overlap with holes (dup fragment?) */
>  		return NULL;
>  	}

Can you please resubmit this (eventually integrating Alessandro's
comments) and add your Signed-off-by: line, so we can integrate this
fix into mainline?

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
What about WRITING it first and rationalizing it afterwords?  :-)
                       - Larry Wall in <8162@jpl-devvax.JPL.NASA.GOV>

[U-Boot] Bug in IP/UDP defragment?

[Alessandro Rubini]

>> The patch below appears to solve my problem.

> Can you please resubmit this (eventually integrating Alessandro's
> comments) and add your Signed-off-by: line, so we can integrate this
> fix into mainline?

This already happened, it's upstream as e397e59e861aa818cda12a23206dde06f7e9f660
(v2010.06-rc2-36-ge397e59) and it went in through Ben Warren.

/alessandro

[U-Boot] Bug in IP/UDP defragment?

[Wolfgang Denk]

Dear Alessandro Rubini,

In message <20100808093940.GA24183@morgana.i.gnudd.com> you wrote:
> >> The patch below appears to solve my problem.
> 
> > Can you please resubmit this (eventually integrating Alessandro's
> > comments) and add your Signed-off-by: line, so we can integrate this
> > fix into mainline?
> 
> This already happened, it's upstream as e397e59e861aa818cda12a23206dde06f7e9f660
> (v2010.06-rc2-36-ge397e59) and it went in through Ben Warren.

Ah, thanks. I missed that.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
In C we had to code our own bugs, in C++ we can inherit them.