[PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set

[PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set

[Marek Vasut]

If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable
cannot be force-set if such attempt happens.

Signed-off-by: Marek Vasut <marex@denx.de>
---
V2: No change
---
 env/flags.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/env/flags.c b/env/flags.c
index b88fe7ba9c..f7a53775c4 100644
--- a/env/flags.c
+++ b/env/flags.c
@@ -524,8 +524,10 @@ int env_flags_validate(const struct env_entry *item, const char *newval,
 
 	/* check for access permission */
 #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
-	if (flag & H_FORCE)
+	if (flag & H_FORCE) {
+		printf("## Error: Can't force access to \"%s\"\n", name);
 		return 0;
+	}
 #endif
 	switch (op) {
 	case env_op_delete:
-- 
2.27.0

[PATCH V2 2/7] env: Add H_DEFAULT flag

[Marek Vasut]

Add another internal environment flag which indicates that the operation
is resetting the environment to the default one. This allows the env code
to discern between import of external environment and reset to default.

Signed-off-by: Marek Vasut <marex@denx.de>
---
V2: No change
---
 env/common.c     | 3 ++-
 include/search.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/env/common.c b/env/common.c
index 088b2aebb4..0db56e610a 100644
--- a/env/common.c
+++ b/env/common.c
@@ -81,6 +81,7 @@ void env_set_default(const char *s, int flags)
 		debug("Using default environment\n");
 	}
 
+	flags |= H_DEFAULT;
 	if (himport_r(&env_htab, (char *)default_environment,
 			sizeof(default_environment), '\0', flags, 0,
 			0, NULL) == 0)
@@ -99,7 +100,7 @@ int env_set_default_vars(int nvars, char * const vars[], int flags)
 	 * Special use-case: import from default environment
 	 * (and use \0 as a separator)
 	 */
-	flags |= H_NOCLEAR;
+	flags |= H_NOCLEAR | H_DEFAULT;
 	return himport_r(&env_htab, (const char *)default_environment,
 				sizeof(default_environment), '\0',
 				flags, 0, nvars, vars);
diff --git a/include/search.h b/include/search.h
index bca36d3abc..c4b50c9630 100644
--- a/include/search.h
+++ b/include/search.h
@@ -112,5 +112,6 @@ int hwalk_r(struct hsearch_data *htab,
 #define H_MATCH_METHOD	(H_MATCH_IDENT | H_MATCH_SUBSTR | H_MATCH_REGEX)
 #define H_PROGRAMMATIC	(1 << 9) /* indicate that an import is from env_set() */
 #define H_ORIGIN_FLAGS	(H_INTERACTIVE | H_PROGRAMMATIC)
+#define H_DEFAULT	(1 << 10) /* indicate that an import is default env */
 
 #endif /* _SEARCH_H_ */
-- 
2.27.0

[PATCH V2 2/7] env: Add H_DEFAULT flag

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:34PM +0200, Marek Vasut wrote:

> Add another internal environment flag which indicates that the operation
> is resetting the environment to the default one. This allows the env code
> to discern between import of external environment and reset to default.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200724/9ad4c791/attachment.sig>

[PATCH V2 2/7] env: Add H_DEFAULT flag

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:34PM +0200, Marek Vasut wrote:

> Add another internal environment flag which indicates that the operation
> is resetting the environment to the default one. This allows the env code
> to discern between import of external environment and reset to default.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200731/808fc9c1/attachment.sig>

[PATCH V2 3/7] env: Discern environment coming from external storage

[Marek Vasut]

Add another custom environment flag which discerns environment coming
from external storage from environment set by U-Boot itself.

Signed-off-by: Marek Vasut <marex@denx.de>
---
V2: New patch
---
 env/common.c     | 13 +++++++------
 env/eeprom.c     |  2 +-
 env/ext4.c       |  2 +-
 env/fat.c        |  2 +-
 env/flash.c      |  2 +-
 env/mmc.c        |  4 ++--
 env/nand.c       |  4 ++--
 env/nvram.c      |  2 +-
 env/onenand.c    |  2 +-
 env/remote.c     |  2 +-
 env/sata.c       |  2 +-
 env/sf.c         |  4 ++--
 env/ubi.c        |  4 ++--
 include/env.h    |  7 +++++--
 include/search.h |  1 +
 15 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/env/common.c b/env/common.c
index 0db56e610a..ed18378000 100644
--- a/env/common.c
+++ b/env/common.c
@@ -110,7 +110,7 @@ int env_set_default_vars(int nvars, char * const vars[], int flags)
  * Check if CRC is valid and (if yes) import the environment.
  * Note that "buf" may or may not be aligned.
  */
-int env_import(const char *buf, int check)
+int env_import(const char *buf, int check, int flags)
 {
 	env_t *ep = (env_t *)buf;
 
@@ -125,7 +125,7 @@ int env_import(const char *buf, int check)
 		}
 	}
 
-	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, 0,
+	if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', flags, 0,
 			0, NULL)) {
 		gd->flags |= GD_FLG_ENV_READY;
 		return 0;
@@ -142,7 +142,8 @@ int env_import(const char *buf, int check)
 static unsigned char env_flags;
 
 int env_import_redund(const char *buf1, int buf1_read_fail,
-		      const char *buf2, int buf2_read_fail)
+		      const char *buf2, int buf2_read_fail,
+		      int flags)
 {
 	int crc1_ok, crc2_ok;
 	env_t *ep, *tmp_env1, *tmp_env2;
@@ -162,10 +163,10 @@ int env_import_redund(const char *buf1, int buf1_read_fail,
 		return -EIO;
 	} else if (!buf1_read_fail && buf2_read_fail) {
 		gd->env_valid = ENV_VALID;
-		return env_import((char *)tmp_env1, 1);
+		return env_import((char *)tmp_env1, 1, flags);
 	} else if (buf1_read_fail && !buf2_read_fail) {
 		gd->env_valid = ENV_REDUND;
-		return env_import((char *)tmp_env2, 1);
+		return env_import((char *)tmp_env2, 1, flags);
 	}
 
 	crc1_ok = crc32(0, tmp_env1->data, ENV_SIZE) ==
@@ -200,7 +201,7 @@ int env_import_redund(const char *buf1, int buf1_read_fail,
 		ep = tmp_env2;
 
 	env_flags = ep->flags;
-	return env_import((char *)ep, 0);
+	return env_import((char *)ep, 0, flags);
 }
 #endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
 
diff --git a/env/eeprom.c b/env/eeprom.c
index e8126cfe39..e300470ad0 100644
--- a/env/eeprom.c
+++ b/env/eeprom.c
@@ -188,7 +188,7 @@ static int env_eeprom_load(void)
 	eeprom_bus_read(CONFIG_SYS_DEF_EEPROM_ADDR,
 		off, (uchar *)buf_env, CONFIG_ENV_SIZE);
 
-	return env_import(buf_env, 1);
+	return env_import(buf_env, 1, H_EXTERNAL);
 }
 
 static int env_eeprom_save(void)
diff --git a/env/ext4.c b/env/ext4.c
index 8e90bb71b7..b6d38324d1 100644
--- a/env/ext4.c
+++ b/env/ext4.c
@@ -124,7 +124,7 @@ static int env_ext4_load(void)
 		goto err_env_relocate;
 	}
 
-	return env_import(buf, 1);
+	return env_import(buf, 1, H_EXTERNAL);
 
 err_env_relocate:
 	env_set_default(NULL, 0);
diff --git a/env/fat.c b/env/fat.c
index 35a1955e63..9f66a6d642 100644
--- a/env/fat.c
+++ b/env/fat.c
@@ -116,7 +116,7 @@ static int env_fat_load(void)
 		goto err_env_relocate;
 	}
 
-	return env_import(buf, 1);
+	return env_import(buf, 1, H_EXTERNAL);
 
 err_env_relocate:
 	env_set_default(NULL, 0);
diff --git a/env/flash.c b/env/flash.c
index 3198147c38..722d5adf8b 100644
--- a/env/flash.c
+++ b/env/flash.c
@@ -351,7 +351,7 @@ static int env_flash_load(void)
 		     "reading environment; recovered successfully\n\n");
 #endif /* CONFIG_ENV_ADDR_REDUND */
 
-	return env_import((char *)flash_addr, 1);
+	return env_import((char *)flash_addr, 1, H_EXTERNAL);
 }
 #endif /* LOADENV */
 
diff --git a/env/mmc.c b/env/mmc.c
index a8b661db80..d8fb51ffe6 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -332,7 +332,7 @@ static int env_mmc_load(void)
 	read2_fail = read_env(mmc, CONFIG_ENV_SIZE, offset2, tmp_env2);
 
 	ret = env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2,
-				read2_fail);
+				read2_fail, H_EXTERNAL);
 
 fini:
 	fini_mmc_for_env(mmc);
@@ -374,7 +374,7 @@ static int env_mmc_load(void)
 		goto fini;
 	}
 
-	ret = env_import(buf, 1);
+	ret = env_import(buf, 1, H_EXTERNAL);
 	if (!ret) {
 		ep = (env_t *)buf;
 		gd->env_addr = (ulong)&ep->data;
diff --git a/env/nand.c b/env/nand.c
index 8b0027d304..0d7ee19bc2 100644
--- a/env/nand.c
+++ b/env/nand.c
@@ -331,7 +331,7 @@ static int env_nand_load(void)
 	read2_fail = readenv(CONFIG_ENV_OFFSET_REDUND, (u_char *) tmp_env2);
 
 	ret = env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2,
-				read2_fail);
+				read2_fail, H_EXTERNAL);
 
 done:
 	free(tmp_env1);
@@ -372,7 +372,7 @@ static int env_nand_load(void)
 		return -EIO;
 	}
 
-	return env_import(buf, 1);
+	return env_import(buf, 1, H_EXTERNAL);
 #endif /* ! ENV_IS_EMBEDDED */
 
 	return 0;
diff --git a/env/nvram.c b/env/nvram.c
index 1a9fcf1c06..7c8ea26f96 100644
--- a/env/nvram.c
+++ b/env/nvram.c
@@ -64,7 +64,7 @@ static int env_nvram_load(void)
 #else
 	memcpy(buf, (void *)CONFIG_ENV_ADDR, CONFIG_ENV_SIZE);
 #endif
-	return env_import(buf, 1);
+	return env_import(buf, 1, H_EXTERNAL);
 }
 
 static int env_nvram_save(void)
diff --git a/env/onenand.c b/env/onenand.c
index dfd4e939f8..a2477cef9b 100644
--- a/env/onenand.c
+++ b/env/onenand.c
@@ -55,7 +55,7 @@ static int env_onenand_load(void)
 		mtd->writesize = MAX_ONENAND_PAGESIZE;
 #endif /* !ENV_IS_EMBEDDED */
 
-	rc = env_import(buf, 1);
+	rc = env_import(buf, 1, H_EXTERNAL);
 	if (!rc)
 		gd->env_valid = ENV_VALID;
 
diff --git a/env/remote.c b/env/remote.c
index e3f0608b16..d93a137376 100644
--- a/env/remote.c
+++ b/env/remote.c
@@ -45,7 +45,7 @@ static int env_remote_save(void)
 static int env_remote_load(void)
 {
 #ifndef ENV_IS_EMBEDDED
-	return env_import((char *)env_ptr, 1);
+	return env_import((char *)env_ptr, 1, H_EXTERNAL);
 #endif
 
 	return 0;
diff --git a/env/sata.c b/env/sata.c
index 8bfcc94306..9442cfcaf3 100644
--- a/env/sata.c
+++ b/env/sata.c
@@ -111,7 +111,7 @@ static void env_sata_load(void)
 		return -EIO;
 	}
 
-	return env_import(buf, 1);
+	return env_import(buf, 1, H_EXTERNAL);
 }
 
 U_BOOT_ENV_LOCATION(sata) = {
diff --git a/env/sf.c b/env/sf.c
index 3e524f2947..81d6e74127 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -172,7 +172,7 @@ static int env_sf_load(void)
 				    CONFIG_ENV_SIZE, tmp_env2);
 
 	ret = env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2,
-				read2_fail);
+				read2_fail, H_EXTERNAL);
 
 	spi_flash_free(env_flash);
 	env_flash = NULL;
@@ -265,7 +265,7 @@ static int env_sf_load(void)
 		goto err_read;
 	}
 
-	ret = env_import(buf, 1);
+	ret = env_import(buf, 1, H_EXTERNAL);
 	if (!ret)
 		gd->env_valid = ENV_VALID;
 
diff --git a/env/ubi.c b/env/ubi.c
index 08aac47df2..5502efe28b 100644
--- a/env/ubi.c
+++ b/env/ubi.c
@@ -141,7 +141,7 @@ static int env_ubi_load(void)
 		       CONFIG_ENV_UBI_PART, CONFIG_ENV_UBI_VOLUME_REDUND);
 
 	return env_import_redund((char *)tmp_env1, read1_fail, (char *)tmp_env2,
-							 read2_fail);
+				 read2_fail, H_EXTERNAL);
 }
 #else /* ! CONFIG_SYS_REDUNDAND_ENVIRONMENT */
 static int env_ubi_load(void)
@@ -172,7 +172,7 @@ static int env_ubi_load(void)
 		return -EIO;
 	}
 
-	return env_import(buf, 1);
+	return env_import(buf, 1, H_EXTERNAL);
 }
 #endif /* CONFIG_SYS_REDUNDAND_ENVIRONMENT */
 
diff --git a/include/env.h b/include/env.h
index d6c2d751d6..8cc70fd752 100644
--- a/include/env.h
+++ b/include/env.h
@@ -288,10 +288,11 @@ int env_erase(void);
  * @buf: Buffer containing the environment (struct environemnt_s *)
  * @check: non-zero to check the CRC at the start of the environment, 0 to
  *	ignore it
+ * @flags: Flags controlling matching (H_... - see search.h)
  * @return 0 if imported successfully, -ENOMSG if the CRC was bad, -EIO if
  *	something else went wrong
  */
-int env_import(const char *buf, int check);
+int env_import(const char *buf, int check, int flags);
 
 /**
  * env_export() - Export the environment to a buffer
@@ -310,10 +311,12 @@ int env_export(struct environment_s *env_out);
  * @buf1_read_fail: 0 if buf1 is valid, non-zero if invalid
  * @buf2: Second environment (struct environemnt_s *)
  * @buf2_read_fail: 0 if buf2 is valid, non-zero if invalid
+ * @flags: Flags controlling matching (H_... - see search.h)
  * @return 0 if OK, -EIO if no environment is valid, -ENOMSG if the CRC was bad
  */
 int env_import_redund(const char *buf1, int buf1_read_fail,
-		      const char *buf2, int buf2_read_fail);
+		      const char *buf2, int buf2_read_fail,
+		      int flags);
 
 /**
  * env_get_default() - Look up a variable from the default environment
diff --git a/include/search.h b/include/search.h
index c4b50c9630..e56843c26f 100644
--- a/include/search.h
+++ b/include/search.h
@@ -113,5 +113,6 @@ int hwalk_r(struct hsearch_data *htab,
 #define H_PROGRAMMATIC	(1 << 9) /* indicate that an import is from env_set() */
 #define H_ORIGIN_FLAGS	(H_INTERACTIVE | H_PROGRAMMATIC)
 #define H_DEFAULT	(1 << 10) /* indicate that an import is default env */
+#define H_EXTERNAL	(1 << 11) /* indicate that an import is external env */
 
 #endif /* _SEARCH_H_ */
-- 
2.27.0

[PATCH V2 3/7] env: Discern environment coming from external storage

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:35PM +0200, Marek Vasut wrote:

> Add another custom environment flag which discerns environment coming
> from external storage from environment set by U-Boot itself.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200724/b95b1de2/attachment.sig>

[PATCH V2 3/7] env: Discern environment coming from external storage

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:35PM +0200, Marek Vasut wrote:

> Add another custom environment flag which discerns environment coming
> from external storage from environment set by U-Boot itself.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200731/c01b51f0/attachment.sig>

[PATCH V2 4/7] env: Fix invalid env handling in env_init()

[Marek Vasut]

This fixes the case where there are multiple environment drivers, one of
them is the default environment one, and it is followed by an environment
driver which does not implement .init() callback. The default environment
driver sets gd->env_valid to ENV_INVALID and returns 0 from its .init()
callback implementation, which is valid behavior for default environment.

Since the subsequent environment driver does not implement .init(), it
also does not modify the $ret variable in the loop. Therefore, the loop
is exited with gd->env_valid=ENV_INVALID and ret=0, which means that the
code further down in env_init() will not reset the environment to the
default one, which is incorrect.

This patch sets the $ret variable back to -ENOENT in case the env_valid
is set to ENV_INVALID by an environment driver, so that the environment
would be correctly reset back to default one, unless a subsequent driver
loads a valid environment.

Signed-off-by: Marek Vasut <marex@denx.de>
---
V2: Reword commit message
---
 env/env.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/env/env.c b/env/env.c
index dcc25c030b..024d36fdbe 100644
--- a/env/env.c
+++ b/env/env.c
@@ -300,6 +300,9 @@ int env_init(void)
 
 		debug("%s: Environment %s init done (ret=%d)\n", __func__,
 		      drv->name, ret);
+
+		if (gd->env_valid == ENV_INVALID)
+			ret = -ENOENT;
 	}
 
 	if (!prio)
-- 
2.27.0

[PATCH V2 4/7] env: Fix invalid env handling in env_init()

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:36PM +0200, Marek Vasut wrote:

> This fixes the case where there are multiple environment drivers, one of
> them is the default environment one, and it is followed by an environment
> driver which does not implement .init() callback. The default environment
> driver sets gd->env_valid to ENV_INVALID and returns 0 from its .init()
> callback implementation, which is valid behavior for default environment.
> 
> Since the subsequent environment driver does not implement .init(), it
> also does not modify the $ret variable in the loop. Therefore, the loop
> is exited with gd->env_valid=ENV_INVALID and ret=0, which means that the
> code further down in env_init() will not reset the environment to the
> default one, which is incorrect.
> 
> This patch sets the $ret variable back to -ENOENT in case the env_valid
> is set to ENV_INVALID by an environment driver, so that the environment
> would be correctly reset back to default one, unless a subsequent driver
> loads a valid environment.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> V2: Reword commit message
> ---
>  env/env.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/env/env.c b/env/env.c
> index dcc25c030b..024d36fdbe 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -300,6 +300,9 @@ int env_init(void)
>  
>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>  		      drv->name, ret);
> +
> +		if (gd->env_valid == ENV_INVALID)
> +			ret = -ENOENT;
>  	}
>  
>  	if (!prio)

So, I am not sure about this change.  Given the whole thread that ends
in https://lists.denx.de/pipermail/u-boot/2020-June/417433.html this
particular part of the function is being too clever.  I think it's
intentionally not doing what you're adding right here for some use
cases.  I think that to cleanly achieve the goals of your series we need
to stop letting drv->init be optional so that we then stop doing the
particular we're doing with "ENOENT means runs this common code path for
many envs".  We may also need to make sure the link order in
env/Makefile has nowhere first in all cases, rather than just most cases
like it does now, with a big comment on why.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200724/3e3cfdfd/attachment.sig>

[PATCH V2 4/7] env: Fix invalid env handling in env_init()

[Marek Vasut]

On 7/24/20 4:56 PM, Tom Rini wrote:
> On Tue, Jul 07, 2020 at 08:51:36PM +0200, Marek Vasut wrote:
> 
>> This fixes the case where there are multiple environment drivers, one of
>> them is the default environment one, and it is followed by an environment
>> driver which does not implement .init() callback. The default environment
>> driver sets gd->env_valid to ENV_INVALID and returns 0 from its .init()
>> callback implementation, which is valid behavior for default environment.
>>
>> Since the subsequent environment driver does not implement .init(), it
>> also does not modify the $ret variable in the loop. Therefore, the loop
>> is exited with gd->env_valid=ENV_INVALID and ret=0, which means that the
>> code further down in env_init() will not reset the environment to the
>> default one, which is incorrect.
>>
>> This patch sets the $ret variable back to -ENOENT in case the env_valid
>> is set to ENV_INVALID by an environment driver, so that the environment
>> would be correctly reset back to default one, unless a subsequent driver
>> loads a valid environment.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> V2: Reword commit message
>> ---
>>  env/env.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/env/env.c b/env/env.c
>> index dcc25c030b..024d36fdbe 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -300,6 +300,9 @@ int env_init(void)
>>  
>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>  		      drv->name, ret);
>> +
>> +		if (gd->env_valid == ENV_INVALID)
>> +			ret = -ENOENT;
>>  	}
>>  
>>  	if (!prio)
> 
> So, I am not sure about this change.  Given the whole thread that ends
> in https://lists.denx.de/pipermail/u-boot/2020-June/417433.html this
> particular part of the function is being too clever.  I think it's
> intentionally not doing what you're adding right here for some use
> cases.

Which use-cases ?

> I think that to cleanly achieve the goals of your series we need
> to stop letting drv->init be optional so that we then stop doing the
> particular we're doing with "ENOENT means runs this common code path for
> many envs".  We may also need to make sure the link order in
> env/Makefile has nowhere first in all cases, rather than just most cases
> like it does now, with a big comment on why.

So, would it make sense to apply the rest and revisit this patch
separately (likely with ST on CC) so the writeable list won't miss this
release ?

[PATCH V2 4/7] env: Fix invalid env handling in env_init()

[Tom Rini]

On Tue, Jul 28, 2020 at 09:28:17AM +0200, Marek Vasut wrote:
> On 7/24/20 4:56 PM, Tom Rini wrote:
> > On Tue, Jul 07, 2020 at 08:51:36PM +0200, Marek Vasut wrote:
> > 
> >> This fixes the case where there are multiple environment drivers, one of
> >> them is the default environment one, and it is followed by an environment
> >> driver which does not implement .init() callback. The default environment
> >> driver sets gd->env_valid to ENV_INVALID and returns 0 from its .init()
> >> callback implementation, which is valid behavior for default environment.
> >>
> >> Since the subsequent environment driver does not implement .init(), it
> >> also does not modify the $ret variable in the loop. Therefore, the loop
> >> is exited with gd->env_valid=ENV_INVALID and ret=0, which means that the
> >> code further down in env_init() will not reset the environment to the
> >> default one, which is incorrect.
> >>
> >> This patch sets the $ret variable back to -ENOENT in case the env_valid
> >> is set to ENV_INVALID by an environment driver, so that the environment
> >> would be correctly reset back to default one, unless a subsequent driver
> >> loads a valid environment.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> ---
> >> V2: Reword commit message
> >> ---
> >>  env/env.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/env/env.c b/env/env.c
> >> index dcc25c030b..024d36fdbe 100644
> >> --- a/env/env.c
> >> +++ b/env/env.c
> >> @@ -300,6 +300,9 @@ int env_init(void)
> >>  
> >>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
> >>  		      drv->name, ret);
> >> +
> >> +		if (gd->env_valid == ENV_INVALID)
> >> +			ret = -ENOENT;
> >>  	}
> >>  
> >>  	if (!prio)
> > 
> > So, I am not sure about this change.  Given the whole thread that ends
> > in https://lists.denx.de/pipermail/u-boot/2020-June/417433.html this
> > particular part of the function is being too clever.  I think it's
> > intentionally not doing what you're adding right here for some use
> > cases.
> 
> Which use-cases ?

flash specifically sets env_valid to ENV_INVALID and returns 0, and uses
the default environment location, when env is invalid.  NAND, when
embedded, sets addr to 0 when invalid, ENV_INVALID and returns 0.
NOWHERE is the real funny case, it says ENV_INVALID and
default_environment and returns 0.

Which all brings me back to my "too clever" statement.  Only REMOTE
ever returns non-zero in it's init function.

> > I think that to cleanly achieve the goals of your series we need
> > to stop letting drv->init be optional so that we then stop doing the
> > particular we're doing with "ENOENT means runs this common code path for
> > many envs".  We may also need to make sure the link order in
> > env/Makefile has nowhere first in all cases, rather than just most cases
> > like it does now, with a big comment on why.
> 
> So, would it make sense to apply the rest and revisit this patch
> separately (likely with ST on CC) so the writeable list won't miss this
> release ?

If you're OK with that, yes.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200728/4804817f/attachment.sig>

[PATCH V2 4/7] env: Fix invalid env handling in env_init()

[Marek Vasut]

On 7/28/20 2:39 PM, Tom Rini wrote:
[...]
>>> So, I am not sure about this change.  Given the whole thread that ends
>>> in https://lists.denx.de/pipermail/u-boot/2020-June/417433.html this
>>> particular part of the function is being too clever.  I think it's
>>> intentionally not doing what you're adding right here for some use
>>> cases.
>>
>> Which use-cases ?
> 
> flash specifically sets env_valid to ENV_INVALID and returns 0, and uses
> the default environment location, when env is invalid.  NAND, when
> embedded, sets addr to 0 when invalid, ENV_INVALID and returns 0.
> NOWHERE is the real funny case, it says ENV_INVALID and
> default_environment and returns 0.
> 
> Which all brings me back to my "too clever" statement.  Only REMOTE
> ever returns non-zero in it's init function.

Which makes me think there is probably no good way out which would not
break one usecase or the other.

>>> I think that to cleanly achieve the goals of your series we need
>>> to stop letting drv->init be optional so that we then stop doing the
>>> particular we're doing with "ENOENT means runs this common code path for
>>> many envs".  We may also need to make sure the link order in
>>> env/Makefile has nowhere first in all cases, rather than just most cases
>>> like it does now, with a big comment on why.
>>
>> So, would it make sense to apply the rest and revisit this patch
>> separately (likely with ST on CC) so the writeable list won't miss this
>> release ?
> 
> If you're OK with that, yes.

Yes, fine.

[PATCH V2 5/7] env: nowhere: Implement .load callback

[Marek Vasut]

Add .load callback for the 'nowhere' environment driver. This is useful
for when the 'nowhere' driver is used in combination with other drivers
and should be used to load the default environment.

Signed-off-by: Marek Vasut <marex@denx.de>
---
V2: No change
---
 env/nowhere.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/env/nowhere.c b/env/nowhere.c
index f5b0a17652..417a636f83 100644
--- a/env/nowhere.c
+++ b/env/nowhere.c
@@ -15,6 +15,12 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+static int env_nowhere_load(void)
+{
+	env_set_default(NULL, 0);
+
+	return 0;
+}
 /*
  * Because we only ever have the default environment available we must mark
  * it as invalid.
@@ -30,5 +36,6 @@ static int env_nowhere_init(void)
 U_BOOT_ENV_LOCATION(nowhere) = {
 	.location	= ENVL_NOWHERE,
 	.init		= env_nowhere_init,
+	.load		= env_nowhere_load,
 	ENV_NAME("nowhere")
 };
-- 
2.27.0

[PATCH V2 5/7] env: nowhere: Implement .load callback

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:37PM +0200, Marek Vasut wrote:

> Add .load callback for the 'nowhere' environment driver. This is useful
> for when the 'nowhere' driver is used in combination with other drivers
> and should be used to load the default environment.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200724/b97de8a1/attachment.sig>

[PATCH V2 5/7] env: nowhere: Implement .load callback

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:37PM +0200, Marek Vasut wrote:

> Add .load callback for the 'nowhere' environment driver. This is useful
> for when the 'nowhere' driver is used in combination with other drivers
> and should be used to load the default environment.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>

I'm deferring this version for
http://patchwork.ozlabs.org/project/uboot/patch/20200728095128.2363-6-patrick.delaunay at st.com/
which address the problem of this change bringing in more code in SPL
otherwise.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200731/93e81f31/attachment.sig>

[PATCH V2 6/7] env: Add option to only ever append environment

[Marek Vasut]

Add configuration option which prevents the environment hash table to be
ever cleared and reloaded with different content. This is useful in case
the first environment loaded into the hash table contains e.g. sensitive
content which must not be dropped or reloaded.

Signed-off-by: Marek Vasut <marex@denx.de>
---
V2: No change
---
 env/Kconfig     | 9 +++++++++
 env/env.c       | 2 ++
 lib/hashtable.c | 4 ++++
 3 files changed, 15 insertions(+)

diff --git a/env/Kconfig b/env/Kconfig
index 38e7fadbb9..9f7eff4f69 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -604,6 +604,15 @@ config DELAY_ENVIRONMENT
 	  later by U-Boot code. With CONFIG_OF_CONTROL this is instead
 	  controlled by the value of /config/load-environment.
 
+config ENV_APPEND
+	bool "Always append the environment with new data"
+	default n
+	help
+	  If defined, the environment hash table is only ever appended with new
+	  data, but the existing hash table can never be dropped and reloaded
+	  with newly imported data. This may be used in combination with static
+	  flags to e.g. to protect variables which must not be modified.
+
 config ENV_ACCESS_IGNORE_FORCE
 	bool "Block forced environment operations"
 	default n
diff --git a/env/env.c b/env/env.c
index 024d36fdbe..967a9d36d7 100644
--- a/env/env.c
+++ b/env/env.c
@@ -204,7 +204,9 @@ int env_load(void)
 		ret = drv->load();
 		if (!ret) {
 			printf("OK\n");
+#if !CONFIG_IS_ENABLED(ENV_APPEND)
 			return 0;
+#endif
 		} else if (ret == -ENOMSG) {
 			/* Handle "bad CRC" case */
 			if (best_prio == -1)
diff --git a/lib/hashtable.c b/lib/hashtable.c
index 7b6781bc35..ef834badc5 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -826,6 +826,10 @@ int himport_r(struct hsearch_data *htab,
 	if (nvars)
 		memcpy(localvars, vars, sizeof(vars[0]) * nvars);
 
+#if CONFIG_IS_ENABLED(ENV_APPEND)
+	flag |= H_NOCLEAR;
+#endif
+
 	if ((flag & H_NOCLEAR) == 0 && !nvars) {
 		/* Destroy old hash table if one exists */
 		debug("Destroy Hash Table: %p table = %p\n", htab,
-- 
2.27.0

[PATCH V2 6/7] env: Add option to only ever append environment

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:38PM +0200, Marek Vasut wrote:

> Add configuration option which prevents the environment hash table to be
> ever cleared and reloaded with different content. This is useful in case
> the first environment loaded into the hash table contains e.g. sensitive
> content which must not be dropped or reloaded.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200724/6e652b39/attachment.sig>

[PATCH V2 6/7] env: Add option to only ever append environment

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:38PM +0200, Marek Vasut wrote:

> Add configuration option which prevents the environment hash table to be
> ever cleared and reloaded with different content. This is useful in case
> the first environment loaded into the hash table contains e.g. sensitive
> content which must not be dropped or reloaded.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200731/bd8ac51d/attachment.sig>

[PATCH V2 7/7] env: Add support for explicit write access list

[Marek Vasut]

This option marks any U-Boot variable which does not have explicit 'w'
writeable flag set as read-only. This way the environment can be locked
down and only variables explicitly configured to be writeable can ever
be changed by either 'env import', 'env set' or loading user environment
from environment storage.

Signed-off-by: Marek Vasut <marex@denx.de>
---
V2: - Only apply the filtering on external env, U-Boot can set variables
      from U-Boot shell and so on.
    - Switch from if CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST) to
      ifdef CONFIG_ENV_WRITEABLE_LIST
---
 env/Kconfig         |  8 ++++++
 env/flags.c         | 62 +++++++++++++++++++++++++++++++++++++--------
 include/env_flags.h |  6 ++++-
 lib/hashtable.c     |  5 +++-
 4 files changed, 68 insertions(+), 13 deletions(-)

diff --git a/env/Kconfig b/env/Kconfig
index 9f7eff4f69..44698d863d 100644
--- a/env/Kconfig
+++ b/env/Kconfig
@@ -613,6 +613,14 @@ config ENV_APPEND
 	  with newly imported data. This may be used in combination with static
 	  flags to e.g. to protect variables which must not be modified.
 
+config ENV_WRITEABLE_LIST
+	bool "Permit write access only to listed variables"
+	default n
+	help
+	  If defined, only environment variables which explicitly set the 'w'
+	  writeable flag can be written and modified at runtime. No variables
+	  can be otherwise created, written or imported into the environment.
+
 config ENV_ACCESS_IGNORE_FORCE
 	bool "Block forced environment operations"
 	default n
diff --git a/env/flags.c b/env/flags.c
index f7a53775c4..df4aed26b2 100644
--- a/env/flags.c
+++ b/env/flags.c
@@ -28,8 +28,15 @@
 #define ENV_FLAGS_NET_VARTYPE_REPS ""
 #endif
 
+#ifdef CONFIG_ENV_WRITEABLE_LIST
+#define ENV_FLAGS_WRITEABLE_VARACCESS_REPS "w"
+#else
+#define ENV_FLAGS_WRITEABLE_VARACCESS_REPS ""
+#endif
+
 static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS;
-static const char env_flags_varaccess_rep[] = "aroc";
+static const char env_flags_varaccess_rep[] =
+	"aroc" ENV_FLAGS_WRITEABLE_VARACCESS_REPS;
 static const int env_flags_varaccess_mask[] = {
 	0,
 	ENV_FLAGS_VARACCESS_PREVENT_DELETE |
@@ -38,7 +45,11 @@ static const int env_flags_varaccess_mask[] = {
 	ENV_FLAGS_VARACCESS_PREVENT_DELETE |
 		ENV_FLAGS_VARACCESS_PREVENT_OVERWR,
 	ENV_FLAGS_VARACCESS_PREVENT_DELETE |
-		ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR};
+		ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR,
+#ifdef CONFIG_ENV_WRITEABLE_LIST
+	ENV_FLAGS_VARACCESS_WRITEABLE,
+#endif
+	};
 
 #ifdef CONFIG_CMD_ENV_FLAGS
 static const char * const env_flags_vartype_names[] = {
@@ -56,6 +67,9 @@ static const char * const env_flags_varaccess_names[] = {
 	"read-only",
 	"write-once",
 	"change-default",
+#ifdef CONFIG_ENV_WRITEABLE_LIST
+	"writeable",
+#endif
 };
 
 /*
@@ -130,21 +144,25 @@ enum env_flags_vartype env_flags_parse_vartype(const char *flags)
  */
 enum env_flags_varaccess env_flags_parse_varaccess(const char *flags)
 {
+	enum env_flags_varaccess va_default = env_flags_varaccess_any;
+	enum env_flags_varaccess va;
 	char *access;
 
 	if (strlen(flags) <= ENV_FLAGS_VARACCESS_LOC)
-		return env_flags_varaccess_any;
+		return va_default;
 
 	access = strchr(env_flags_varaccess_rep,
 		flags[ENV_FLAGS_VARACCESS_LOC]);
 
-	if (access != NULL)
-		return (enum env_flags_varaccess)
+	if (access != NULL) {
+		va = (enum env_flags_varaccess)
 			(access - &env_flags_varaccess_rep[0]);
+		return va;
+	}
 
 	printf("## Warning: Unknown environment variable access method '%c'\n",
 		flags[ENV_FLAGS_VARACCESS_LOC]);
-	return env_flags_varaccess_any;
+	return va_default;
 }
 
 /*
@@ -152,17 +170,21 @@ enum env_flags_varaccess env_flags_parse_varaccess(const char *flags)
  */
 enum env_flags_varaccess env_flags_parse_varaccess_from_binflags(int binflags)
 {
+	enum env_flags_varaccess va_default = env_flags_varaccess_any;
+	enum env_flags_varaccess va;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(env_flags_varaccess_mask); i++)
 		if (env_flags_varaccess_mask[i] ==
-		    (binflags & ENV_FLAGS_VARACCESS_BIN_MASK))
-			return (enum env_flags_varaccess)i;
+		    (binflags & ENV_FLAGS_VARACCESS_BIN_MASK)) {
+			va = (enum env_flags_varaccess)i;
+			return va;
+	}
 
 	printf("Warning: Non-standard access flags. (0x%x)\n",
 		binflags & ENV_FLAGS_VARACCESS_BIN_MASK);
 
-	return env_flags_varaccess_any;
+	return va_default;
 }
 
 static inline int is_hex_prefix(const char *value)
@@ -326,13 +348,14 @@ enum env_flags_vartype env_flags_get_type(const char *name)
 enum env_flags_varaccess env_flags_get_varaccess(const char *name)
 {
 	const char *flags_list = env_get(ENV_FLAGS_VAR);
+	enum env_flags_varaccess va_default = env_flags_varaccess_any;
 	char flags[ENV_FLAGS_ATTR_MAX_LEN + 1];
 
 	if (env_flags_lookup(flags_list, name, flags))
-		return env_flags_varaccess_any;
+		return va_default;
 
 	if (strlen(flags) <= ENV_FLAGS_VARACCESS_LOC)
-		return env_flags_varaccess_any;
+		return va_default;
 
 	return env_flags_parse_varaccess(flags);
 }
@@ -426,7 +449,11 @@ void env_flags_init(struct env_entry *var_entry)
 	int ret = 1;
 
 	if (first_call) {
+#ifdef CONFIG_ENV_WRITEABLE_LIST
+		flags_list = ENV_FLAGS_LIST_STATIC;
+#else
 		flags_list = env_get(ENV_FLAGS_VAR);
+#endif
 		first_call = 0;
 	}
 	/* look in the ".flags" and static for a reference to this variable */
@@ -523,6 +550,19 @@ int env_flags_validate(const struct env_entry *item, const char *newval,
 	}
 
 	/* check for access permission */
+#ifdef CONFIG_ENV_WRITEABLE_LIST
+	if (flag & H_DEFAULT)
+		return 0;	/* Default env is always OK */
+
+	/*
+	 * External writeable variables can be overwritten by external env,
+	 * anything else can not be overwritten by external env.
+	 */
+	if ((flag & H_EXTERNAL) &&
+	    !(item->flags & ENV_FLAGS_VARACCESS_WRITEABLE))
+		return 1;
+#endif
+
 #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
 	if (flag & H_FORCE) {
 		printf("## Error: Can't force access to \"%s\"\n", name);
diff --git a/include/env_flags.h b/include/env_flags.h
index 725841a891..313cb8c49a 100644
--- a/include/env_flags.h
+++ b/include/env_flags.h
@@ -24,6 +24,9 @@ enum env_flags_varaccess {
 	env_flags_varaccess_readonly,
 	env_flags_varaccess_writeonce,
 	env_flags_varaccess_changedefault,
+#ifdef CONFIG_ENV_WRITEABLE_LIST
+	env_flags_varaccess_writeable,
+#endif
 	env_flags_varaccess_end
 };
 
@@ -173,6 +176,7 @@ int env_flags_validate(const struct env_entry *item, const char *newval,
 #define ENV_FLAGS_VARACCESS_PREVENT_CREATE		0x00000010
 #define ENV_FLAGS_VARACCESS_PREVENT_OVERWR		0x00000020
 #define ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR	0x00000040
-#define ENV_FLAGS_VARACCESS_BIN_MASK			0x00000078
+#define ENV_FLAGS_VARACCESS_WRITEABLE			0x00000080
+#define ENV_FLAGS_VARACCESS_BIN_MASK			0x000000f8
 
 #endif /* __ENV_FLAGS_H__ */
diff --git a/lib/hashtable.c b/lib/hashtable.c
index ef834badc5..4a8c50b4b8 100644
--- a/lib/hashtable.c
+++ b/lib/hashtable.c
@@ -950,9 +950,12 @@ int himport_r(struct hsearch_data *htab,
 		e.data = value;
 
 		hsearch_r(e, ENV_ENTER, &rv, htab, flag);
-		if (rv == NULL)
+#if !CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)
+		if (rv == NULL) {
 			printf("himport_r: can't insert \"%s=%s\" into hash table\n",
 				name, value);
+		}
+#endif
 
 		debug("INSERT: table %p, filled %d/%d rv %p ==> name=\"%s\" value=\"%s\"\n",
 			htab, htab->filled, htab->size,
-- 
2.27.0

[PATCH V2 7/7] env: Add support for explicit write access list

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:39PM +0200, Marek Vasut wrote:

> This option marks any U-Boot variable which does not have explicit 'w'
> writeable flag set as read-only. This way the environment can be locked
> down and only variables explicitly configured to be writeable can ever
> be changed by either 'env import', 'env set' or loading user environment
> from environment storage.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200724/199d429c/attachment.sig>

[PATCH V2 7/7] env: Add support for explicit write access list

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:39PM +0200, Marek Vasut wrote:

> This option marks any U-Boot variable which does not have explicit 'w'
> writeable flag set as read-only. This way the environment can be locked
> down and only variables explicitly configured to be writeable can ever
> be changed by either 'env import', 'env set' or loading user environment
> from environment storage.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200731/e7737652/attachment.sig>

[PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:33PM +0200, Marek Vasut wrote:

> If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable
> cannot be force-set if such attempt happens.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200724/1dd918a4/attachment.sig>

[PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set

[Tom Rini]

On Tue, Jul 07, 2020 at 08:51:33PM +0200, Marek Vasut wrote:

> If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable
> cannot be force-set if such attempt happens.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Reviewed-by: Tom Rini <trini@konsulko.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200731/b55a068e/attachment.sig>

[PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set

[Simon Goldschmidt]

Am 31.07.2020 um 23:40 schrieb Tom Rini:
> On Tue, Jul 07, 2020 at 08:51:33PM +0200, Marek Vasut wrote:
> 
>> If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable
>> cannot be force-set if such attempt happens.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Reviewed-by: Tom Rini <trini@konsulko.com>
> 
> Applied to u-boot/master, thanks!
> 

Sorry I haven't followed this and wasn't able to report this earlier,
but I think this is wrong: This warning is issued when 0 is returned,
which means the change is accepted, not rejected.

Regards,
Simon

[PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set

[Marek Vasut]

On 10/23/20 10:58 AM, Simon Goldschmidt wrote:
> Am 31.07.2020 um 23:40 schrieb Tom Rini:
>> On Tue, Jul 07, 2020 at 08:51:33PM +0200, Marek Vasut wrote:
>>
>>> If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable
>>> cannot be force-set if such attempt happens.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Reviewed-by: Tom Rini <trini@konsulko.com>
>>
>> Applied to u-boot/master, thanks!
>>
> 
> Sorry I haven't followed this and wasn't able to report this earlier,
> but I think this is wrong: This warning is issued when 0 is returned,
> which means the change is accepted, not rejected.

I think there was a subsequent discussion on this topic in the ML,
[PATCH] Revert "env: Warn on force access if ENV_ACCESS_IGNORE_FORCE
set", I think we reached a conclusion this patch was OK. But if you did
more digging and found a problem, please send a patch / provide details.

[PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set

[Simon Goldschmidt]

Am 23.10.2020 um 11:52 schrieb Marek Vasut:
> On 10/23/20 10:58 AM, Simon Goldschmidt wrote:
>> Am 31.07.2020 um 23:40 schrieb Tom Rini:
>>> On Tue, Jul 07, 2020 at 08:51:33PM +0200, Marek Vasut wrote:
>>>
>>>> If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable
>>>> cannot be force-set if such attempt happens.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Reviewed-by: Tom Rini <trini@konsulko.com>
>>>
>>> Applied to u-boot/master, thanks!
>>>
>>
>> Sorry I haven't followed this and wasn't able to report this earlier,
>> but I think this is wrong: This warning is issued when 0 is returned,
>> which means the change is accepted, not rejected.
> 
> I think there was a subsequent discussion on this topic in the ML,
> [PATCH] Revert "env: Warn on force access if ENV_ACCESS_IGNORE_FORCE
> set", I think we reached a conclusion this patch was OK. But if you did
> more digging and found a problem, please send a patch / provide details.
> 

I use a script that reads ethaddrs from external storage and then use
"env set -f ethaddr <addr>". With v2020.10, I now get a warning that
this can't be written, but I still see the value later with 'env print'.

I think this should just be reverted. I'll try to find the thread
discussing the revert patch you mentioned.

Regards,
Simon

[PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set

[Alex Kiernan]

On Tue, Jul 7, 2020 at 7:52 PM Marek Vasut <marex@denx.de> wrote:
>
> If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable
> cannot be force-set if such attempt happens.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> V2: No change
> ---
>  env/flags.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/env/flags.c b/env/flags.c
> index b88fe7ba9c..f7a53775c4 100644
> --- a/env/flags.c
> +++ b/env/flags.c
> @@ -524,8 +524,10 @@ int env_flags_validate(const struct env_entry *item, const char *newval,
>
>         /* check for access permission */
>  #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
> -       if (flag & H_FORCE)
> +       if (flag & H_FORCE) {
> +               printf("## Error: Can't force access to \"%s\"\n", name);
>                 return 0;
> +       }
>  #endif
>         switch (op) {
>         case env_op_delete:

AFAICT this is wrong - you get the message when you have
CONFIG_ENV_ACCESS_IGNORE_FORCE disabled and use force:

      => env print ethaddr
      ethaddr=00:1C:2B:08:AF:65
      => env set ethaddr 00:00:00:00:00:00
      ## Error: Can't overwrite "ethaddr"
      ## Error inserting "ethaddr" variable, errno=1
      => env print ethaddr
      ethaddr=00:1C:2B:08:AF:65
      => env set -f ethaddr 00:00:00:00:00:00
      ## Error: Can't force access to "ethaddr"
      => env print ethaddr
      ethaddr=00:00:00:00:00:00

Just staring at the code, I don't see a good way to capture this
behaviour, other than wiring it into each of the branches of the
switch - I started off with something like this:

diff --git a/env/flags.c b/env/flags.c
index df4aed26b2c6..70621dff4434 100644
--- a/env/flags.c
+++ b/env/flags.c
@@ -563,12 +563,13 @@ int env_flags_validate(const struct env_entry
*item, const char *newval,
                return 1;
 #endif

-#ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
        if (flag & H_FORCE) {
-               printf("## Error: Can't force access to \"%s\"\n", name);
-               return 0;
+               if (CONFIG_IS_ENABLED(ENV_ACCESS_IGNORE_FORCE))
+                       printf("## Error: Can't force access to
\"%s\"\n", name);
+               else
+                       return 0;
        }
-#endif
+
        switch (op) {
        case env_op_delete:
                if (item->flags & ENV_FLAGS_VARACCESS_PREVENT_DELETE) {

But I think with that you'll get the message for variables which can
be overwritten, so still not what's intended.

--
Alex Kiernan