[U-Boot] Bug in IP/UDP defragment?
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
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 at 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
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
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
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
participants (3)
-
Alessandro Rubini -
Fillod Stephane -
Wolfgang Denk