memmove_wd: Allow overlapping memory area
PATCH 1/1 memmove_wd: Allow overlapping memory area
--- common/image.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
common/image.c +8 -3
@@ -456,9 +456,14 @@ void memmove_wd (void *to, void *from, size_t len, ulong chunksz)while (len >0 ) {size_t tail = (len > chunksz) ? chunksz : len;WATCHDOG_RESET (); -memmove (to, from, tail); - to += tail; - from += tail; +if (to <= from) + { +memmove (to, from, tail); + to += tail; + from += tail; + }else { +memmove (to + len - tail, from + len - tail, tail); + } len -= tail; }#else /* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */
Signature · 1 line
1.7.1
PATCH 1/1 memmove_wd: Allow overlapping memory area
Dear Alexander Stein,
Quoted 4 lines · In message <1279701826-20083-1-git-send-email-alexander.stein@systec-electronic…
Signed-off-by Alexander Stein <[email protected]>--- common/image.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
Why would this be needed? Do you have an error scenario?
common/image.c void memmove_wd (void *to, void *from, size_t len, ulong chunksz) · line 465
Quoted 7 lines
common/image.c +7
-0+if (to <= from) + { +memmove (to, from, tail); + to += tail; + from += tail; + }else { +memmove (to + len - tail, from + len - tail, tail);
In which way is this supposed to allow overlapping memory areas?
Best regards,
Wolfgang Denk
Signature · 5 lines
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
I think there's a world market for about five computers.
-- attr. Thomas J. Watson (Chairman of the Board, IBM), 1943PATCH 1/1 memmove_wd: Allow overlapping memory area
Dear Wolfgang,
Am Montag, 9. August 2010, 00:18:19 schrieb Wolfgang Denk:
Quoted 1 line
In message <1279701826-20083-1-git-send-email-alexander.stein@systec-
Quoted 7 lines · electronic.com> you wrote:
Quoted 5 lines
Signed-off-by Alexander Stein <[email protected]>---
common/image.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)Why would this be needed? Do you have an error scenario?
IIRC the problem arose when i downloaded my image to 0x20000000 (SD-RAM) using TFTP and my Linux kernel entry point was at 0x20008000. So when CONFIG_HW_WATCHDOG is defined u-boot starts copying from the beginning thus overriding the source. Finally i got decompression errors upon Linux kernel start.
Without CONFIG_HW_WATCHDOG memmove_wd is simply memmove which handled this cased correctly.
Quoted 9 lines
common/image.c void memmove_wd (void *to, void *from, size_t len, ulong chunksz) · line 465
Quoted 7 lines
common/image.c +7
-0+if (to <= from) + { +memmove (to, from, tail); + to += tail; + from += tail; + }else { +memmove (to + len - tail, from + len - tail, tail);In which way is this supposed to allow overlapping memory areas?
With this change u-boot starts to copy from the end to the beginning thus preventing overriding the source. This change was adopted from memmove (lib/string.c) which handles this case correctly.
Best regards,
Alexander
PATCH 1/1 memmove_wd: Allow overlapping memory area
Dear Alexander Stein,
Quoted 8 lines · In message <[email protected]> you wrote:
Quoted 5 lines
Quoted 3 lines
common/image.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)Why would this be needed? Do you have an error scenario?
IIRC the problem arose when i downloaded my image to
0x20000000(SD-RAM) using TFTP and my Linux kernel entry point was at0x20008000. So when
Don;t do this, then. Such kind of overlap has never been supported. Even if it would work in your case of uncompressed images, it is bound to fail for compressed ones where the uncompressed code grows faster then compressed data get consumed.
Quoted 10 lines
Quoted 9 lines
common/image.c void memmove_wd (void *to, void *from, size_t len, ulong chunksz) · line 465
Quoted 7 lines
common/image.c +7
-0+if (to <= from) + { +memmove (to, from, tail); + to += tail; + from += tail; + }else { +memmove (to + len - tail, from + len - tail, tail);In which way is this supposed to allow overlapping memory areas?
Quoted 3 lines
With this change u-boot starts to copy from the end to the beginning thus preventing overriding the source. This change was adopted from memmove (
lib/string.c) which handles this case correctly.
You are just shifting the problem to another area, but you are not solving it. Without your patch there is a problem when the initial area overlaps, with your problem there is one when the tail overlaps.
Best regards,
Wolfgang Denk
Signature · 5 lines
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 "The two most common things in the universe are hydrogen and stupi- dity."
PATCH 1/1 memmove_wd: Allow overlapping memory area
Dear Wolfgang,
Am Montag, 9. August 2010, 11:26:56 schrieb Wolfgang Denk:
Quoted 7 lines
Quoted 2 lines
IIRC the problem arose when i downloaded my image to
0x20000000(SD-RAM) using TFTP and my Linux kernel entry point was at0x20008000. So whenDon;t do this, then. Such kind of overlap has never been supported. Even if it would work in your case of uncompressed images, it is bound to fail for compressed ones where the uncompressed code grows faster then compressed data get consumed.
Well, that's at least one possibility but it is very annoying that something like memmove that works fine so far suddenly stops working when watchdog support is enabled.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
common/image.c void memmove_wd (void *to, void *from, size_t len, ulong chunksz) · line 465
Quoted 7 lines
common/image.c +7
-0+if (to <= from) + { +memmove (to, from, tail); + to += tail; + from += tail; + }else { +memmove (to + len - tail, from + len - tail, tail);In which way is this supposed to allow overlapping memory areas?
Quoted 7 lines
Quoted 3 lines
With this change u-boot starts to copy from the end to the beginning thus preventing overriding the source. This change was adopted from memmove (
lib/string.c) which handles this case correctly.You are just shifting the problem to another area, but you are not solving it. Without your patch there is a problem when the initial area overlaps, with your problem there is one when the tail overlaps.
Well, why is this situation then handled by memmove?
Best regards,
Alexander
PATCH 1/1 memmove_wd: Allow overlapping memory area
Dear Alexander Stein,
Quoted 8 lines · In message <[email protected]> you wrote:
Quoted 4 lines
Even if it would work in your case of uncompressed images, it is bound to fail for compressed ones where the uncompressed code grows faster then compressed data get consumed.
Well, that's at least one possibility but it is very annoying that something like memmove that works fine so far suddenly stops working when watchdog support is enabled.
Yes, I understand this. It's also annoying when you've been using compressed images just fine for years and suddenly they stop working because the kernel grows beyond some magic (and undefined) limit.
Better do not build on undefined behaviour.
Quoted 5 lines
Quoted 3 lines
You are just shifting the problem to another area, but you are not solving it. Without your patch there is a problem when the initial area overlaps, with your problem there is one when the tail overlaps.
Well, why is this situation then handled by memmove?
memmove() is used in a log of places.
Please note that I'm not against fixing this problem, but if we address it, it should be fixed correctly, i. e. in sich a way that it will always work, not only for certain cases.
Best regards,
Wolfgang Denk
Signature · 4 lines
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 "Free markets select for winning solutions." - Eric S. Raymond
PATCH 1/1 memmove_wd: Allow overlapping memory area
Dear Wolfgang,
August 2010, 13:43:32 schrieb Wolfgang Denk:
Quoted 9 lines
Quoted 3 lines
Well, that's at least one possibility but it is very annoying that something like memmove that works fine so far suddenly stops working when watchdog support is enabled.
Yes, I understand this. It's also annoying when you've been using compressed images just fine for years and suddenly they stop working because the kernel grows beyond some magic (and undefined) limit.
Better do not build on undefined behaviour.
Yes, of course. But it seems difficult to recognise thing like a simple memmove with dest < src as an undefined behavior.
Quoted 3 lines
Please note that I'm not against fixing this problem, but if we address it, it should be fixed correctly, i. e. in sich a way that it will always work, not only for certain cases.
So how to fix this then? I'm totally inexperienced to compressed images.
Best regards,
Alexander
PATCH 1/1 memmove_wd: Allow overlapping memory area
Dear Alexander Stein,
Quoted 5 lines · In message <[email protected]> you wrote:
Quoted 2 lines
Better do not build on undefined behaviour.
Yes, of course. But it seems difficult to recognise thing like a simple memmove with dest < src as an undefined behavior.
See the FAQ: http://www.denx.de/wiki/view/DULG/LinuxUncompressingError
It has been explained before (and often) that the download address of a kernel image must be sufficiently far away from the load address in the image header.
Quoted 5 lines
Quoted 3 lines
Please note that I'm not against fixing this problem, but if we address it, it should be fixed correctly, i. e. in sich a way that it will always work, not only for certain cases.
So how to fix this then? I'm totally inexperienced to compressed images.
Well, I don't an easy fix for the compression case.
Best regards,
Wolfgang Denk
Signature · 5 lines
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 Only a fool fights in a burning house. -- Kank the Klingon, "Day of the Dove", stardate unknown
PATCH 1/1 memmove_wd: Allow overlapping memory area
In reply to Alexander SteinDear Alexander Stein,
Quoted 4 lines · In message <1279701826-20083-1-git-send-email-alexander.stein@systec-electronic…
Signed-off-by Alexander Stein <[email protected]>--- common/image.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
And BTW:
common/image.c void memmove_wd (void *to, void *from, size_t len, ulong chunksz) · line 460
Quoted 2 lines
common/image.c +2
-0+if (to <= from) + {
Incorrect brace style.
Best regards,
Wolfgang Denk
Signature · 4 lines
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 This is now. Later is later.