[PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE

[Marek Vasut]

On 2/1/21 8:31 PM, Tom Rini wrote:
> On Fri, Jan 29, 2021 at 12:03:52AM +0100, Marek Vasut wrote:
>> On 1/28/21 8:26 PM, Tom Rini wrote:
>>> On Thu, Jan 28, 2021 at 08:07:54PM +0100, Marek Vasut wrote:
>>>> On 1/11/21 11:27 AM, Martin Fuzzey wrote:
>>>>> Since commit 0f036bf4b87e ("env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set")
>>>>> a warning message is displayed when setenv -f is used WITHOUT
>>>>> CONFIG_ENV_ACCESS_IGNORE_FORCE, but the variable is set anyway, resulting
>>>>> in lots of log pollution.
>>>>>
>>>>> env_flags_validate() returns 0 if the access is accepted, or non zero
>>>>> if it is refused.
>>>>>
>>>>> So the original code
>>>>> 	#ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
>>>>> 		if (flag & H_FORCE)
>>>>> 			return 0;
>>>>> 	#endif
>>>>>
>>>>> was correct, it returns 0 (accepts the modification) if forced UNLESS
>>>>> IGNORE_FORCE is set (in which case access checks in the following code
>>>>> are applied). The broken patch just added a printf to the force accepted
>>>>> case.
>>>>>
>>>>> To obtain the intent of the patch we need this:
>>>>> 	if (flag & H_FORCE) {
>>>>> 	#ifdef CONFIG_ENV_ACCESS_IGNORE_FORCE
>>>>> 		printf("## Error: Can't force access to \"%s\"\n", name);
>>>>> 	#else
>>>>> 		return 0;
>>>>> 	#endif
>>>>> 	}
>>>>>
>>>>> Fixes: 0f036bf4b87e ("env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set")
>>>>>
>>>>> Signed-off-by: Martin Fuzzey <martin.fuzzey at flowbird.group>
>>>>> ---
>>>>>     env/flags.c | 5 +++--
>>>>>     1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/env/flags.c b/env/flags.c
>>>>> index df4aed2..e3e833c 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) {
>>>>> +#ifdef CONFIG_ENV_ACCESS_IGNORE_FORCE
>>>>>     		printf("## Error: Can't force access to \"%s\"\n", name);
>>>>> +#else
>>>>>     		return 0;
>>>>> -	}
>>>>>     #endif
>>>>
>>>> Based on env/Kconfig  description of this option:
>>>>
>>>> config ENV_ACCESS_IGNORE_FORCE
>>>>           bool "Block forced environment operations"
>>>>           default n
>>>>           help
>>>>             If defined, don't allow the -f switch to env set override variable
>>>>             access flags.
>>>>
>>>> I would think the code should look like this:
>>>>
>>>> #ifdef CONFIG_ENV_ACCESS_IGNORE_FORCE
>>>>           if (flag & H_FORCE) {
>>>>                   printf("## Error: Can't force access to \"%s\"\n", name);
>>>>                   return 1;
>>>>           }
>>>> #else
>>>>           if (flag & H_FORCE)
>>>> 		return 0;
>>>> #endif
>>>
>>> So, prior to 0f036bf4b87e we had what you're suggesting, and that lead
>>> to 8a5cdf601f8d (which is the commit I was trying to think of) which
>>> Heinrich did not like, but was what was needed to get things to function
>>> again.  Wouldn't what you're proposing break the use case you had in the
>>> first place?
>>
>> No, the idea is to completely block the -f flag if
>> CONFIG_ENV_ACCESS_IGNORE_FORCE is set from setting anything in the
>> environment. That's how I understand the Kconfig entry help text.
> 
> So was this all a "by inspection" bug then and not something you ran in
> to in use?  I'm a bit worried that "how it's implemented" is relied upon
> more than "how it's documented in the help", esp since the former is
> probably older than the latter.

The usecase of the writeable list is to completely block the -f flag.
Maybe some further cleanup of the env config options is required, 
because it is poorly documented it seems.