[U-Boot] [PATCH 0/2] env: Fix "env save" to malformed environment
In case when environment is malformed (e.g. zeroed out), we are not able to save new environment to that location. Let's fix that by not considering the location with "bad CRC" environment as not accessible.
Sam Protsenko (2): env: common: Return specific error code on bad CRC env: Fix saving environment to "bad CRC" location
env/common.c | 4 ++-- env/env.c | 25 +++++++++++++++++++------ 2 files changed, 21 insertions(+), 8 deletions(-)
Callers of env_import*() functions might want to check the case when we have incorrect environment (with bad CRC). For example, when environment location is being defined in env_load(), call chain may look like this:
env_load() -> drv->load() = env_mmc_load() -> env_import()
Return code will be passed from env_import() all way up to env_load(). Right now both env_mmc_load() and env_import() return -EIO error code, so env_load() can't differentiate between two cases: 1. Driver reports the error, because device is not accessible 2. Device is actually accessible, but environment is broken
Let's return -ENOMSG in env_import(), so we can distinguish two cases mentioned above. It will make it possible to continue working with "bad CRC" environment (like doing "env save"), instead of considering it not functional (implemented in subsequent patch).
Signed-off-by: Sam Protsenko semen.protsenko@linaro.org --- env/common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/env/common.c b/env/common.c index d1a6a52860..324502ed82 100644 --- a/env/common.c +++ b/env/common.c @@ -115,7 +115,7 @@ int env_import(const char *buf, int check)
if (crc32(0, ep->data, ENV_SIZE) != crc) { set_default_env("bad CRC", 0); - return -EIO; + return -ENOMSG; /* needed for env_load() */ } }
@@ -169,7 +169,7 @@ int env_import_redund(const char *buf1, int buf1_read_fail,
if (!crc1_ok && !crc2_ok) { set_default_env("bad CRC", 0); - return -EIO; + return -ENOMSG; /* needed for env_load() */ } else if (crc1_ok && !crc2_ok) { gd->env_valid = ENV_VALID; } else if (!crc1_ok && crc2_ok) {
Am 18.01.2019 um 20:19 schrieb Sam Protsenko:
Callers of env_import*() functions might want to check the case when we have incorrect environment (with bad CRC). For example, when environment location is being defined in env_load(), call chain may look like this:
env_load() -> drv->load() = env_mmc_load() -> env_import()Return code will be passed from env_import() all way up to env_load(). Right now both env_mmc_load() and env_import() return -EIO error code, so env_load() can't differentiate between two cases:
- Driver reports the error, because device is not accessible
- Device is actually accessible, but environment is broken
Let's return -ENOMSG in env_import(), so we can distinguish two cases mentioned above. It will make it possible to continue working with "bad CRC" environment (like doing "env save"), instead of considering it not functional (implemented in subsequent patch).
Signed-off-by: Sam Protsenko semen.protsenko@linaro.org
Reviewed-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
env/common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/env/common.c b/env/common.c index d1a6a52860..324502ed82 100644 --- a/env/common.c +++ b/env/common.c @@ -115,7 +115,7 @@ int env_import(const char *buf, int check)
if (crc32(0, ep->data, ENV_SIZE) != crc) { set_default_env("bad CRC", 0);
return -EIO;
return -ENOMSG; /* needed for env_load() */@@ -169,7 +169,7 @@ int env_import_redund(const char *buf1, int buf1_read_fail,
if (!crc1_ok && !crc2_ok) { set_default_env("bad CRC", 0);
return -EIO;
return -ENOMSG; /* needed for env_load() */
On Fri, Jan 18, 2019 at 09:19:03PM +0200, Sam Protsenko wrote:
Callers of env_import*() functions might want to check the case when we have incorrect environment (with bad CRC). For example, when environment location is being defined in env_load(), call chain may look like this:
env_load() -> drv->load() = env_mmc_load() -> env_import()Return code will be passed from env_import() all way up to env_load(). Right now both env_mmc_load() and env_import() return -EIO error code, so env_load() can't differentiate between two cases:
- Driver reports the error, because device is not accessible
- Device is actually accessible, but environment is broken
Let's return -ENOMSG in env_import(), so we can distinguish two cases mentioned above. It will make it possible to continue working with "bad CRC" environment (like doing "env save"), instead of considering it not functional (implemented in subsequent patch).
Signed-off-by: Sam Protsenko semen.protsenko@linaro.org Reviewed-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
Applied to u-boot/master, thanks!
In case when the environment on some location is malformed (CRC isn't matching), there is a chance we won't be able to save the environment to that location. For example, consider the case when we only have the environment on eMMC, but it's zeroed out. In that case, we won't be able to "env save" to it, because of "bad CRC" error. That's happening because in env_load() function we consider malformed environment as incorrect one, and defaulting to the location with highest (0) priority, which can be different from one we are dealing with right now (e.g., highest priority can be ENV_FAT on SD card, which is not inserted, but we want to use ENV_MMC on eMMC, where we were booted from).
This issue began to reproduce after commit d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") on BeagleBone Black, but that commit didn't introduce the wrong logic, it just changed the behavior for default location to use, merely revealing this issue.
To fix that, let's implement next logic in env_load(): 1. Try to find out correct environment; if found -- use it 2. If working environment wasn't found, but we found malformed one (with bad CRC), let's use it for further "env save". But make sure to use malformed environment location with highest priority. 3. If neither correct nor malformed environment was found, let's default to environment location with highest priority (0)
Steps to reproduce mentioned issue on BeagleBone Black (fixed in this patch):
1. Boot from SD card and erase eMMC in U-Boot shell: => mmc dev 1 => mmc erase 0 100000 => gpt write mmc 1 $partitions 2. Write new SPL and U-Boot to eMMC; the rest of eMMC will stay filled with zeroes 3. Boot from eMMC; try to do: => env save 4. Observe the error (incorrect behavior). Correct behavior: environment should be stored correctly on eMMC, in spite of it has "bad CRC"
Fixes: d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") Signed-off-by: Sam Protsenko semen.protsenko@linaro.org --- env/env.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/env/env.c b/env/env.c index 003509d342..4b417b90a2 100644 --- a/env/env.c +++ b/env/env.c @@ -177,6 +177,7 @@ int env_get_char(int index) int env_load(void) { struct env_driver *drv; + int best_prio = -1; int prio;
for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) { @@ -195,20 +196,32 @@ int env_load(void) * one message. */ ret = drv->load(); - if (ret) { - debug("Failed (%d)\n", ret); - } else { + if (!ret) { printf("OK\n"); return 0; + } else if (ret == -ENOMSG) { + /* Handle "bad CRC" case */ + if (best_prio == -1) + best_prio = prio; + } else { + debug("Failed (%d)\n", ret); } }
/* * In case of invalid environment, we set the 'default' env location - * to the highest priority. In this way, next calls to env_save() - * will restore the environment at the right place. + * to the best choice, i.e.: + * 1. Environment location with bad CRC, if such location was found + * 2. Otherwise use the location with highest priority + * + * This way, next calls to env_save() will restore the environment + * at the right place. */ - env_get_location(ENVOP_LOAD, 0); + if (best_prio >= 0) + debug("Selecting environment with bad CRC\n"); + else + best_prio = 0; + env_get_location(ENVOP_LOAD, best_prio);
return -ENODEV; }
Am Fr., 18. Jan. 2019, 20:20 hat Sam Protsenko semen.protsenko@linaro.org geschrieben:
In case when the environment on some location is malformed (CRC isn't matching), there is a chance we won't be able to save the environment to that location. For example, consider the case when we only have the environment on eMMC, but it's zeroed out. In that case, we won't be able to "env save" to it, because of "bad CRC" error. That's happening because in env_load() function we consider malformed environment as incorrect one, and defaulting to the location with highest (0) priority, which can be different from one we are dealing with right now (e.g., highest priority can be ENV_FAT on SD card, which is not inserted, but we want to use ENV_MMC on eMMC, where we were booted from).
This issue began to reproduce after commit d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") on BeagleBone Black, but that commit didn't introduce the wrong logic, it just changed the behavior for default location to use, merely revealing this issue.
In my opinion, this automatism of selecting a driver to save the env can always produce a behaviour that is unexpected to the user.
I wonder if it wouldn't be better to either let the board define some default env driver for saving or the user should specify the driver as argument to 'env save'.
Note that this is not an objection to your oatches, I'm just not content with the current auto-selection...
Regards, Simon
To fix that, let's implement next logic in env_load():
- Try to find out correct environment; if found -- use it
- If working environment wasn't found, but we found malformed one (with bad CRC), let's use it for further "env save". But make sure to use malformed environment location with highest priority.
- If neither correct nor malformed environment was found, let's default to environment location with highest priority (0)
Steps to reproduce mentioned issue on BeagleBone Black (fixed in this patch):
- Boot from SD card and erase eMMC in U-Boot shell: => mmc dev 1 => mmc erase 0 100000 => gpt write mmc 1 $partitions
- Write new SPL and U-Boot to eMMC; the rest of eMMC will stay filled with zeroes
- Boot from eMMC; try to do: => env save
- Observe the error (incorrect behavior). Correct behavior: environment should be stored correctly on eMMC, in spite of it has "bad CRC"
Fixes: d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") Signed-off-by: Sam Protsenko semen.protsenko@linaro.org
env/env.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/env/env.c b/env/env.c index 003509d342..4b417b90a2 100644 --- a/env/env.c +++ b/env/env.c @@ -177,6 +177,7 @@ int env_get_char(int index) int env_load(void) { struct env_driver *drv;
int best_prio = -1; int prio; for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio));prio++) { @@ -195,20 +196,32 @@ int env_load(void) * one message. */ ret = drv->load();
if (ret) {debug("Failed (%d)\n", ret);} else {
if (!ret) { printf("OK\n"); return 0;} else if (ret == -ENOMSG) {/* Handle "bad CRC" case */if (best_prio == -1)best_prio = prio;} else {debug("Failed (%d)\n", ret); } } /* * In case of invalid environment, we set the 'default' envlocation
* to the highest priority. In this way, next calls to env_save()* will restore the environment at the right place.
* to the best choice, i.e.:* 1. Environment location with bad CRC, if such location wasfound
* 2. Otherwise use the location with highest priority** This way, next calls to env_save() will restore the environment* at the right place. */
env_get_location(ENVOP_LOAD, 0);
if (best_prio >= 0)debug("Selecting environment with bad CRC\n");elsebest_prio = 0;env_get_location(ENVOP_LOAD, best_prio); return -ENODEV;}
2.20.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Hi Simon,
On Fri, Jan 18, 2019 at 10:46 PM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
Am Fr., 18. Jan. 2019, 20:20 hat Sam Protsenko semen.protsenko@linaro.org geschrieben:
In case when the environment on some location is malformed (CRC isn't matching), there is a chance we won't be able to save the environment to that location. For example, consider the case when we only have the environment on eMMC, but it's zeroed out. In that case, we won't be able to "env save" to it, because of "bad CRC" error. That's happening because in env_load() function we consider malformed environment as incorrect one, and defaulting to the location with highest (0) priority, which can be different from one we are dealing with right now (e.g., highest priority can be ENV_FAT on SD card, which is not inserted, but we want to use ENV_MMC on eMMC, where we were booted from).
This issue began to reproduce after commit d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") on BeagleBone Black, but that commit didn't introduce the wrong logic, it just changed the behavior for default location to use, merely revealing this issue.
In my opinion, this automatism of selecting a driver to save the env can always produce a behaviour that is unexpected to the user.
Completely agree with you. That's my thoughts exactly, when I ran into this issue.
I wonder if it wouldn't be better to either let the board define some default env driver for saving or the user should specify the driver as argument to 'env save'.
Yeah, if I have some spare time, gonna implement something like this, at least as RFC patch. We can start full-scale discussion in that thread, as there could be more possible ways of fixing that, and we should consider different platforms when deciding on that. But I think we should take this patch anyway, as "env save" without arguments should be left too, for compatibility reasons.
Note that this is not an objection to your oatches, I'm just not content with the current auto-selection...
Yeah, but that's another matter, I hope I will get to that soon. Meanwhile, can you please add your "Reviewed-by" tag, if the code looks ok to you? With this patch we won't have semi-bricked boards, at least.
Regards, Simon
To fix that, let's implement next logic in env_load():
- Try to find out correct environment; if found -- use it
- If working environment wasn't found, but we found malformed one (with bad CRC), let's use it for further "env save". But make sure to use malformed environment location with highest priority.
- If neither correct nor malformed environment was found, let's default to environment location with highest priority (0)
Steps to reproduce mentioned issue on BeagleBone Black (fixed in this patch):
- Boot from SD card and erase eMMC in U-Boot shell: => mmc dev 1 => mmc erase 0 100000 => gpt write mmc 1 $partitions
- Write new SPL and U-Boot to eMMC; the rest of eMMC will stay filled with zeroes
- Boot from eMMC; try to do: => env save
- Observe the error (incorrect behavior). Correct behavior: environment should be stored correctly on eMMC, in spite of it has "bad CRC"
Fixes: d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") Signed-off-by: Sam Protsenko semen.protsenko@linaro.org
env/env.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/env/env.c b/env/env.c index 003509d342..4b417b90a2 100644 --- a/env/env.c +++ b/env/env.c @@ -177,6 +177,7 @@ int env_get_char(int index) int env_load(void) { struct env_driver *drv;
int best_prio = -1; int prio; for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {@@ -195,20 +196,32 @@ int env_load(void) * one message. */ ret = drv->load();
if (ret) {debug("Failed (%d)\n", ret);} else {
if (!ret) { printf("OK\n"); return 0;} else if (ret == -ENOMSG) {/* Handle "bad CRC" case */if (best_prio == -1)best_prio = prio;} else {debug("Failed (%d)\n", ret); } } /* * In case of invalid environment, we set the 'default' env location
* to the highest priority. In this way, next calls to env_save()* will restore the environment at the right place.
* to the best choice, i.e.:* 1. Environment location with bad CRC, if such location was found* 2. Otherwise use the location with highest priority** This way, next calls to env_save() will restore the environment* at the right place. */
env_get_location(ENVOP_LOAD, 0);
if (best_prio >= 0)debug("Selecting environment with bad CRC\n");elsebest_prio = 0;env_get_location(ENVOP_LOAD, best_prio); return -ENODEV;}
2.20.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Am 19.01.2019 um 14:28 schrieb Sam Protsenko:
Hi Simon,
On Fri, Jan 18, 2019 at 10:46 PM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
Am Fr., 18. Jan. 2019, 20:20 hat Sam Protsenko semen.protsenko@linaro.org geschrieben:
In case when the environment on some location is malformed (CRC isn't matching), there is a chance we won't be able to save the environment to that location. For example, consider the case when we only have the environment on eMMC, but it's zeroed out. In that case, we won't be able to "env save" to it, because of "bad CRC" error. That's happening because in env_load() function we consider malformed environment as incorrect one, and defaulting to the location with highest (0) priority, which can be different from one we are dealing with right now (e.g., highest priority can be ENV_FAT on SD card, which is not inserted, but we want to use ENV_MMC on eMMC, where we were booted from).
This issue began to reproduce after commit d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") on BeagleBone Black, but that commit didn't introduce the wrong logic, it just changed the behavior for default location to use, merely revealing this issue.
In my opinion, this automatism of selecting a driver to save the env can always produce a behaviour that is unexpected to the user.
Completely agree with you. That's my thoughts exactly, when I ran into this issue.
I wonder if it wouldn't be better to either let the board define some default env driver for saving or the user should specify the driver as argument to 'env save'.
Yeah, if I have some spare time, gonna implement something like this, at least as RFC patch. We can start full-scale discussion in that thread, as there could be more possible ways of fixing that, and we should consider different platforms when deciding on that. But I think we should take this patch anyway, as "env save" without arguments should be left too, for compatibility reasons.
Note that this is not an objection to your oatches, I'm just not content with the current auto-selection...
Yeah, but that's another matter, I hope I will get to that soon. Meanwhile, can you please add your "Reviewed-by" tag, if the code looks ok to you? With this patch we won't have semi-bricked boards, at least.
Well, I don't know if that's another matter. This multi-env code is pretty new, and it seems to me it hasn't fully worked, yet, for corner cases. Being like that, I'm not totally sure your change is OK for everyone using more than one env source. What if the first device returning an error code was an unexpected error? You'll then save your environment to some other storage without noticing... Is that expected behaviour for multi-env support? Or shouldn't we rather return an error and let the user supply the target device instead?
I just fear your change might break things for others.
I'm not saying your patch is totally wrong, just thinking. I did use multi-env support, but never checked error paths back then...
Regards, Simon
Regards, Simon
To fix that, let's implement next logic in env_load():
- Try to find out correct environment; if found -- use it
- If working environment wasn't found, but we found malformed one (with bad CRC), let's use it for further "env save". But make sure to use malformed environment location with highest priority.
- If neither correct nor malformed environment was found, let's default to environment location with highest priority (0)
Steps to reproduce mentioned issue on BeagleBone Black (fixed in this patch):
- Boot from SD card and erase eMMC in U-Boot shell: => mmc dev 1 => mmc erase 0 100000 => gpt write mmc 1 $partitions
- Write new SPL and U-Boot to eMMC; the rest of eMMC will stay filled with zeroes
- Boot from eMMC; try to do: => env save
- Observe the error (incorrect behavior). Correct behavior: environment should be stored correctly on eMMC, in spite of it has "bad CRC"
Fixes: d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") Signed-off-by: Sam Protsenko semen.protsenko@linaro.org
env/env.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/env/env.c b/env/env.c index 003509d342..4b417b90a2 100644 --- a/env/env.c +++ b/env/env.c @@ -177,6 +177,7 @@ int env_get_char(int index) int env_load(void) { struct env_driver *drv;
int best_prio = -1; int prio; for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {@@ -195,20 +196,32 @@ int env_load(void) * one message. */ ret = drv->load();
if (ret) {debug("Failed (%d)\n", ret);} else {
if (!ret) { printf("OK\n"); return 0;} else if (ret == -ENOMSG) {/* Handle "bad CRC" case */if (best_prio == -1)best_prio = prio;} else {debug("Failed (%d)\n", ret); } } /* * In case of invalid environment, we set the 'default' env location
* to the highest priority. In this way, next calls to env_save()* will restore the environment at the right place.
* to the best choice, i.e.:* 1. Environment location with bad CRC, if such location was found* 2. Otherwise use the location with highest priority** This way, next calls to env_save() will restore the environment* at the right place. */
env_get_location(ENVOP_LOAD, 0);
if (best_prio >= 0)debug("Selecting environment with bad CRC\n");elsebest_prio = 0;env_get_location(ENVOP_LOAD, best_prio); return -ENODEV;}
-- 2.20.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Am 21.01.2019 um 10:18 schrieb Simon Goldschmidt:
Am 19.01.2019 um 14:28 schrieb Sam Protsenko:
Hi Simon,
On Fri, Jan 18, 2019 at 10:46 PM Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
Am Fr., 18. Jan. 2019, 20:20 hat Sam Protsenko semen.protsenko@linaro.org geschrieben:
In case when the environment on some location is malformed (CRC isn't matching), there is a chance we won't be able to save the environment to that location. For example, consider the case when we only have the environment on eMMC, but it's zeroed out. In that case, we won't be able to "env save" to it, because of "bad CRC" error. That's happening because in env_load() function we consider malformed environment as incorrect one, and defaulting to the location with highest (0) priority, which can be different from one we are dealing with right now (e.g., highest priority can be ENV_FAT on SD card, which is not inserted, but we want to use ENV_MMC on eMMC, where we were booted from).
This issue began to reproduce after commit d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") on BeagleBone Black, but that commit didn't introduce the wrong logic, it just changed the behavior for default location to use, merely revealing this issue.
In my opinion, this automatism of selecting a driver to save the env can always produce a behaviour that is unexpected to the user.
Completely agree with you. That's my thoughts exactly, when I ran into this issue.
I wonder if it wouldn't be better to either let the board define some default env driver for saving or the user should specify the driver as argument to 'env save'.
Yeah, if I have some spare time, gonna implement something like this, at least as RFC patch. We can start full-scale discussion in that thread, as there could be more possible ways of fixing that, and we should consider different platforms when deciding on that. But I think we should take this patch anyway, as "env save" without arguments should be left too, for compatibility reasons.
Note that this is not an objection to your oatches, I'm just not content with the current auto-selection...
Yeah, but that's another matter, I hope I will get to that soon. Meanwhile, can you please add your "Reviewed-by" tag, if the code looks ok to you? With this patch we won't have semi-bricked boards, at least.
Well, I don't know if that's another matter. This multi-env code is pretty new, and it seems to me it hasn't fully worked, yet, for corner cases. Being like that, I'm not totally sure your change is OK for everyone using more than one env source. What if the first device returning an error code was an unexpected error? You'll then save your environment to some other storage without noticing... Is that expected behaviour for multi-env support? Or shouldn't we rather return an error and let the user supply the target device instead?
I just fear your change might break things for others.
I'm not saying your patch is totally wrong, just thinking. I did use multi-env support, but never checked error paths back then...
Thinking about this again, we're probably better off with this patch than without, so:
Reviewed-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
Regards, Simon
Regards, Simon
To fix that, let's implement next logic in env_load(): 1. Try to find out correct environment; if found -- use it 2. If working environment wasn't found, but we found malformed one (with bad CRC), let's use it for further "env save". But make sure to use malformed environment location with highest priority. 3. If neither correct nor malformed environment was found, let's default to environment location with highest priority (0)
Steps to reproduce mentioned issue on BeagleBone Black (fixed in this patch):
- Boot from SD card and erase eMMC in U-Boot shell: => mmc dev 1 => mmc erase 0 100000 => gpt write mmc 1 $partitions
- Write new SPL and U-Boot to eMMC; the rest of eMMC will stay filled with zeroes
- Boot from eMMC; try to do: => env save
- Observe the error (incorrect behavior). Correct behavior: environment should be stored correctly on eMMC, in spite of it has "bad CRC"
Fixes: d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") Signed-off-by: Sam Protsenko semen.protsenko@linaro.org
env/env.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/env/env.c b/env/env.c index 003509d342..4b417b90a2 100644 --- a/env/env.c +++ b/env/env.c @@ -177,6 +177,7 @@ int env_get_char(int index) int env_load(void) { struct env_driver *drv;
int best_prio = -1; int prio; for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {@@ -195,20 +196,32 @@ int env_load(void) * one message. */ ret = drv->load();
if (ret) {debug("Failed (%d)\n", ret);} else {
if (!ret) { printf("OK\n"); return 0;} else if (ret == -ENOMSG) {/* Handle "bad CRC" case */if (best_prio == -1)best_prio = prio;} else {debug("Failed (%d)\n", ret); } } /* * In case of invalid environment, we set the 'default' env location
* to the highest priority. In this way, next calls to env_save()* will restore the environment at the right place.
* to the best choice, i.e.:* 1. Environment location with bad CRC, if such location was found* 2. Otherwise use the location with highest priority** This way, next calls to env_save() will restore the environment* at the right place. */
env_get_location(ENVOP_LOAD, 0);
if (best_prio >= 0)debug("Selecting environment with bad CRC\n");elsebest_prio = 0;env_get_location(ENVOP_LOAD, best_prio); return -ENODEV;}
-- 2.20.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Hi,
i ran also in same problem, after resize environment (4k to 8k), because my 4k is no more enough for my environment.
i did not understand my CRC have to be checked if i want to save (and the checked part will be overridden). It makes sense to check the CRC of current environment-block which will be written but not the existing existing on mmc.
imho save should verify generated block before save (memory) and after (on storage). So this case should not happen, that saving is impossible because of CRC on mmc is invalid before save...
currently i look on http://www.denx.de/wiki/view/DULG/UBootCmdGroupMMC to find the right params to clear my environment at 0x100000 byte-offset, i guess there are blocks of 512byte so mmc erase should be at block 0x800 with length 0x10 (8k), am i right?
regards Frank
Gesendet: Freitag, 18. Januar 2019 um 20:19 Uhr Von: "Sam Protsenko" semen.protsenko@linaro.org An: u-boot@lists.denx.de Cc: "Tom Rini" trini@konsulko.com, "Nicholas Faustini" nicholas.faustini@azcomtech.com, "Maxime Ripard" maxime.ripard@free-electrons.com Betreff: [U-Boot] [PATCH 2/2] env: Fix saving environment to "bad CRC" location
In case when the environment on some location is malformed (CRC isn't matching), there is a chance we won't be able to save the environment to that location. For example, consider the case when we only have the environment on eMMC, but it's zeroed out. In that case, we won't be able to "env save" to it, because of "bad CRC" error. That's happening because in env_load() function we consider malformed environment as incorrect one, and defaulting to the location with highest (0) priority, which can be different from one we are dealing with right now (e.g., highest priority can be ENV_FAT on SD card, which is not inserted, but we want to use ENV_MMC on eMMC, where we were booted from).
This issue began to reproduce after commit d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") on BeagleBone Black, but that commit didn't introduce the wrong logic, it just changed the behavior for default location to use, merely revealing this issue.
To fix that, let's implement next logic in env_load():
- Try to find out correct environment; if found -- use it
- If working environment wasn't found, but we found malformed one (with bad CRC), let's use it for further "env save". But make sure to use malformed environment location with highest priority.
- If neither correct nor malformed environment was found, let's default to environment location with highest priority (0)
Steps to reproduce mentioned issue on BeagleBone Black (fixed in this patch):
- Boot from SD card and erase eMMC in U-Boot shell: => mmc dev 1 => mmc erase 0 100000 => gpt write mmc 1 $partitions
- Write new SPL and U-Boot to eMMC; the rest of eMMC will stay filled with zeroes
- Boot from eMMC; try to do: => env save
- Observe the error (incorrect behavior). Correct behavior: environment should be stored correctly on eMMC, in spite of it has "bad CRC"
Fixes: d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") Signed-off-by: Sam Protsenko semen.protsenko@linaro.org
env/env.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/env/env.c b/env/env.c index 003509d342..4b417b90a2 100644 --- a/env/env.c +++ b/env/env.c @@ -177,6 +177,7 @@ int env_get_char(int index) int env_load(void) { struct env_driver *drv;
int best_prio = -1; int prio;
for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {
@@ -195,20 +196,32 @@ int env_load(void) * one message. */ ret = drv->load();
if (ret) {debug("Failed (%d)\n", ret);} else {
if (!ret) { printf("OK\n"); return 0;} else if (ret == -ENOMSG) {/* Handle "bad CRC" case */if (best_prio == -1)best_prio = prio;} else {debug("Failed (%d)\n", ret);} }
/*
- In case of invalid environment, we set the 'default' env location
* to the highest priority. In this way, next calls to env_save()* will restore the environment at the right place.
* to the best choice, i.e.:* 1. Environment location with bad CRC, if such location was found* 2. Otherwise use the location with highest priority** This way, next calls to env_save() will restore the environment* at the right place.
- env_get_location(ENVOP_LOAD, 0);
if (best_prio >= 0)
debug("Selecting environment with bad CRC\n");else
best_prio = 0;env_get_location(ENVOP_LOAD, best_prio);
return -ENODEV;
}
2.20.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Thu, Jan 24, 2019 at 4:11 PM Frank Wunderlich frank-w@public-files.de wrote:
Hi,
i ran also in same problem, after resize environment (4k to 8k), because my 4k is no more enough for my environment.
i did not understand my CRC have to be checked if i want to save (and the checked part will be overridden). It makes sense to check the CRC of current environment-block which will be written but not the existing existing on mmc.
imho save should verify generated block before save (memory) and after (on storage). So this case should not happen, that saving is impossible because of CRC on mmc is invalid before save...
currently i look on http://www.denx.de/wiki/view/DULG/UBootCmdGroupMMC to find the right params to clear my environment at 0x100000 byte-offset, i guess there are blocks of 512byte so mmc erase should be at block 0x800 with length 0x10 (8k), am i right?
regards Frank
Hi Frank,
As I understand, your case is different from one I'm fixing in my patch. My patch is fixing the case when CRC is wrong during environment loading, not saving. So in case when environment was zeroed on your eMMC, you won't be able to do "env save" anymore, which in my case as bad as a bricked board. So I still think we need to merge this as is.
Thanks.
Gesendet: Freitag, 18. Januar 2019 um 20:19 Uhr Von: "Sam Protsenko" semen.protsenko@linaro.org An: u-boot@lists.denx.de Cc: "Tom Rini" trini@konsulko.com, "Nicholas Faustini" nicholas.faustini@azcomtech.com, "Maxime Ripard" maxime.ripard@free-electrons.com Betreff: [U-Boot] [PATCH 2/2] env: Fix saving environment to "bad CRC" location
In case when the environment on some location is malformed (CRC isn't matching), there is a chance we won't be able to save the environment to that location. For example, consider the case when we only have the environment on eMMC, but it's zeroed out. In that case, we won't be able to "env save" to it, because of "bad CRC" error. That's happening because in env_load() function we consider malformed environment as incorrect one, and defaulting to the location with highest (0) priority, which can be different from one we are dealing with right now (e.g., highest priority can be ENV_FAT on SD card, which is not inserted, but we want to use ENV_MMC on eMMC, where we were booted from).
This issue began to reproduce after commit d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") on BeagleBone Black, but that commit didn't introduce the wrong logic, it just changed the behavior for default location to use, merely revealing this issue.
To fix that, let's implement next logic in env_load():
- Try to find out correct environment; if found -- use it
- If working environment wasn't found, but we found malformed one (with bad CRC), let's use it for further "env save". But make sure to use malformed environment location with highest priority.
- If neither correct nor malformed environment was found, let's default to environment location with highest priority (0)
Steps to reproduce mentioned issue on BeagleBone Black (fixed in this patch):
- Boot from SD card and erase eMMC in U-Boot shell: => mmc dev 1 => mmc erase 0 100000 => gpt write mmc 1 $partitions
- Write new SPL and U-Boot to eMMC; the rest of eMMC will stay filled with zeroes
- Boot from eMMC; try to do: => env save
- Observe the error (incorrect behavior). Correct behavior: environment should be stored correctly on eMMC, in spite of it has "bad CRC"
Fixes: d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") Signed-off-by: Sam Protsenko semen.protsenko@linaro.org
env/env.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/env/env.c b/env/env.c index 003509d342..4b417b90a2 100644 --- a/env/env.c +++ b/env/env.c @@ -177,6 +177,7 @@ int env_get_char(int index) int env_load(void) { struct env_driver *drv;
int best_prio = -1; int prio; for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {@@ -195,20 +196,32 @@ int env_load(void) * one message. */ ret = drv->load();
if (ret) {debug("Failed (%d)\n", ret);} else {
if (!ret) { printf("OK\n"); return 0;} else if (ret == -ENOMSG) {/* Handle "bad CRC" case */if (best_prio == -1)best_prio = prio;} else {debug("Failed (%d)\n", ret); } } /* * In case of invalid environment, we set the 'default' env location
* to the highest priority. In this way, next calls to env_save()* will restore the environment at the right place.
* to the best choice, i.e.:* 1. Environment location with bad CRC, if such location was found* 2. Otherwise use the location with highest priority** This way, next calls to env_save() will restore the environment* at the right place. */
env_get_location(ENVOP_LOAD, 0);
if (best_prio >= 0)debug("Selecting environment with bad CRC\n");elsebest_prio = 0;env_get_location(ENVOP_LOAD, best_prio); return -ENODEV;}
2.20.1
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Fri, Jan 18, 2019 at 09:19:04PM +0200, Sam Protsenko wrote:
In case when the environment on some location is malformed (CRC isn't matching), there is a chance we won't be able to save the environment to that location. For example, consider the case when we only have the environment on eMMC, but it's zeroed out. In that case, we won't be able to "env save" to it, because of "bad CRC" error. That's happening because in env_load() function we consider malformed environment as incorrect one, and defaulting to the location with highest (0) priority, which can be different from one we are dealing with right now (e.g., highest priority can be ENV_FAT on SD card, which is not inserted, but we want to use ENV_MMC on eMMC, where we were booted from).
This issue began to reproduce after commit d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") on BeagleBone Black, but that commit didn't introduce the wrong logic, it just changed the behavior for default location to use, merely revealing this issue.
To fix that, let's implement next logic in env_load():
- Try to find out correct environment; if found -- use it
- If working environment wasn't found, but we found malformed one (with bad CRC), let's use it for further "env save". But make sure to use malformed environment location with highest priority.
- If neither correct nor malformed environment was found, let's default to environment location with highest priority (0)
Steps to reproduce mentioned issue on BeagleBone Black (fixed in this patch):
- Boot from SD card and erase eMMC in U-Boot shell: => mmc dev 1 => mmc erase 0 100000 => gpt write mmc 1 $partitions
- Write new SPL and U-Boot to eMMC; the rest of eMMC will stay filled with zeroes
- Boot from eMMC; try to do: => env save
- Observe the error (incorrect behavior). Correct behavior: environment should be stored correctly on eMMC, in spite of it has "bad CRC"
Fixes: d30ba2315ae3 ("u-boot: remove driver lookup loop from env_save()") Signed-off-by: Sam Protsenko semen.protsenko@linaro.org Reviewed-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
Applied to u-boot/master, thanks!
participants (4)
-
Frank Wunderlich -
Sam Protsenko -
Simon Goldschmidt -
Tom Rini