| Message ID | 1479292229-17256-8-git-send-email-bernhard.nortmann@web.de |
|---|---|
| State | RFC |
| Delegated to: | Joe Hershberger |
| Headers | show |
Hi Bernhard, On 16 November 2016 at 03:30, Bernhard Nortmann <bernhard.nortmann@web.de> wrote: > This is an attempt to prevent such information from ending up > in exported environment data, especially when doing "saveenv". > (http://lists.denx.de/pipermail/u-boot/2015-September/227611.html) > > The patch makes use of the new setenv_transient() helper for > environment variables that get updated via network configuration: > BOOTP/DHCP (netboot_update_env), CDP (cdp_update_env) and > link-local protocol (do_link_local). > > Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de> > --- > > Changes in v2: None > > cmd/net.c | 34 +++++++++++++++++----------------- > 1 file changed, 17 insertions(+), 17 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org> How would you make these variables non-transient? Does the transient nature show up in 'printenv'? Regards, Simon
Hi Simon! Am 19.11.2016 um 14:47 schrieb Simon Glass: > Hi Bernhard, > > On 16 November 2016 at 03:30, Bernhard Nortmann > <bernhard.nortmann@web.de> wrote: >> This is an attempt to prevent such information from ending up >> in exported environment data, especially when doing "saveenv". >> (http://lists.denx.de/pipermail/u-boot/2015-September/227611.html) >> >> The patch makes use of the new setenv_transient() helper for >> environment variables that get updated via network configuration: >> BOOTP/DHCP (netboot_update_env), CDP (cdp_update_env) and >> link-local protocol (do_link_local). >> >> Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de> >> --- >> >> Changes in v2: None >> >> cmd/net.c | 34 +++++++++++++++++----------------- >> 1 file changed, 17 insertions(+), 17 deletions(-) > Reviewed-by: Simon Glass <sjg@chromium.org> > > How would you make these variables non-transient? Does the transient > nature show up in 'printenv'? > > Regards, > Simon I think it's best to consider a practical example. I'm firing up my sunxi-based device with these patches applied on top of U-Boot v2016.11, and interrupt the autoboot. The standard way to examine variables that have non-default flags is the "env flags" command. In my case this outputs (discarding the explanations near the top): Static flags: Variable Name Variable Type Variable Access ------------- ------------- --------------- eth\d?addr MAC address write-once ipaddr IP address any gatewayip IP address any netmask IP address any serverip IP address any nvlan decimal any vlan decimal any dnsip IP address any serial# string write-once fel_booted boolean system fel_scriptaddr hexadecimal system Active flags: Variable Name Variable Type Variable Access ------------- ------------- --------------- serial# string write-once ethaddr MAC address write-once fel_booted boolean system As you can see, patch 5/7 has sneaked in two new "system" flags and since I've booted using the FEL mechanism, the "fel_booted" one is in effect ("active"). Okay, now I use the command "setenv autoload no; dhcp" to auto-configure networking. Let's check "env flags" again. The "Static flags" section is unchanged, but at the bottom there's now: Active flags: Variable Name Variable Type Variable Access ------------- ------------- --------------- ipaddr IP address transient serial# string write-once hostname string transient ethaddr MAC address write-once netmask IP address transient serverip IP address transient fel_booted boolean system dnsip IP address transient gatewayip IP address transient >Does the transient nature show up in 'printenv'? No, it doesn't. "printenv" will give output that looks as usual / perfectly normal. You're also not restricted in modifying "transient" variables, for example I can do "ipaddr 10.11.12.13" with no problems. But: The differences will become apparent when trying to export the environment with something like "saveenv" or "env export": => env export 0x40000000 ## Don't export "ipaddr" ## Don't export "hostname" ## Don't export "netmask" ## Don't export "serverip" ## Don't export "fel_booted" ## Don't export "dnsip" ## Don't export "gatewayip" U-Boot refuses to store the 'volatile' information for those variables that have been flagged accordingly, and informs the user about it. This is checked individually per variable, i.e. the other ones will get exported normally. At this point I decide that I want to set up a *static* IP configuration and make it persistent with "saveenv". Houston, we have a problem! >How would you make these variables non-transient? Well, again there's a standard U-Boot mechanism for this. The README mentions it in the description of CONFIG_ENV_FLAGS_LIST_STATIC: "To override a setting in the static list, simply add an entry for the same variable name to the '.flags' variable.". So we could do "setenv .flags ipaddr:i,netmask:i,gatewayip.i" to reset the variables to access level "any", meaning they'll get exported again. Admittedly, there's a slight inconsistency here. Due to the fact that setenv_transient() only sets a temporary flag, and U-Boot will rebuild the access flags list (from the "static" default one) when assigning ".flags", the "transient" flags for the networking variables will get cleared anyway - even when deleting ".flags". The alternative would be to mark these variables "transient" from the beginning (in the static flags), but I considered that more awkward - when taking into account that users might want to setup (and "saveenv") the network manually right from the start, and are less likely to override the values retrieved via auto-configuration (DHCP etc.). Regards, B. Nortmann
Hi Bernhard, On 22 November 2016 at 15:30, Bernhard Nortmann <bernhard.nortmann@web.de> wrote: > Hi Simon! > > > Am 19.11.2016 um 14:47 schrieb Simon Glass: >> >> Hi Bernhard, >> >> On 16 November 2016 at 03:30, Bernhard Nortmann >> <bernhard.nortmann@web.de> wrote: >>> >>> This is an attempt to prevent such information from ending up >>> in exported environment data, especially when doing "saveenv". >>> (http://lists.denx.de/pipermail/u-boot/2015-September/227611.html) >>> >>> The patch makes use of the new setenv_transient() helper for >>> environment variables that get updated via network configuration: >>> BOOTP/DHCP (netboot_update_env), CDP (cdp_update_env) and >>> link-local protocol (do_link_local). >>> >>> Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de> >>> --- >>> >>> Changes in v2: None >>> >>> cmd/net.c | 34 +++++++++++++++++----------------- >>> 1 file changed, 17 insertions(+), 17 deletions(-) >> >> Reviewed-by: Simon Glass <sjg@chromium.org> >> >> How would you make these variables non-transient? Does the transient >> nature show up in 'printenv'? >> >> Regards, >> Simon > > > I think it's best to consider a practical example. I'm firing up my > sunxi-based > device with these patches applied on top of U-Boot v2016.11, and interrupt > the > autoboot. > > The standard way to examine variables that have non-default flags is the > "env > flags" command. In my case this outputs (discarding the explanations near > the > top): > > Static flags: > Variable Name Variable Type Variable Access > ------------- ------------- --------------- > eth\d?addr MAC address write-once > ipaddr IP address any > gatewayip IP address any > netmask IP address any > serverip IP address any > nvlan decimal any > vlan decimal any > dnsip IP address any > serial# string write-once > fel_booted boolean system > fel_scriptaddr hexadecimal system > > Active flags: > Variable Name Variable Type Variable Access > ------------- ------------- --------------- > serial# string write-once > ethaddr MAC address write-once > fel_booted boolean system > > As you can see, patch 5/7 has sneaked in two new "system" flags and since > I've > booted using the FEL mechanism, the "fel_booted" one is in effect > ("active"). > > Okay, now I use the command "setenv autoload no; dhcp" to auto-configure > networking. Let's check "env flags" again. The "Static flags" section is > unchanged, but at the bottom there's now: > > Active flags: > Variable Name Variable Type Variable Access > ------------- ------------- --------------- > ipaddr IP address transient > serial# string write-once > hostname string transient > ethaddr MAC address write-once > netmask IP address transient > serverip IP address transient > fel_booted boolean system > dnsip IP address transient > gatewayip IP address transient > >>Does the transient nature show up in 'printenv'? > > No, it doesn't. "printenv" will give output that looks as usual / perfectly > normal. You're also not restricted in modifying "transient" variables, for > example I can do "ipaddr 10.11.12.13" with no problems. > > But: The differences will become apparent when trying to export the > environment > with something like "saveenv" or "env export": > => env export 0x40000000 > ## Don't export "ipaddr" > ## Don't export "hostname" > ## Don't export "netmask" > ## Don't export "serverip" > ## Don't export "fel_booted" > ## Don't export "dnsip" > ## Don't export "gatewayip" > > U-Boot refuses to store the 'volatile' information for those variables that > have been flagged accordingly, and informs the user about it. This is > checked > individually per variable, i.e. the other ones will get exported normally. > > At this point I decide that I want to set up a *static* IP configuration and > make it persistent with "saveenv". Houston, we have a problem! > >>How would you make these variables non-transient? > > Well, again there's a standard U-Boot mechanism for this. The README > mentions > it in the description of CONFIG_ENV_FLAGS_LIST_STATIC: "To override a > setting > in the static list, simply add an entry for the same variable name to the > '.flags' variable.". > > So we could do "setenv .flags ipaddr:i,netmask:i,gatewayip.i" to reset the > variables to access level "any", meaning they'll get exported again. > > Admittedly, there's a slight inconsistency here. Due to the fact that > setenv_transient() only sets a temporary flag, and U-Boot will rebuild the > access flags list (from the "static" default one) when assigning ".flags", > the "transient" flags for the networking variables will get cleared anyway - > even when deleting ".flags". > > The alternative would be to mark these variables "transient" from the > beginning > (in the static flags), but I considered that more awkward - when taking into > account that users might want to setup (and "saveenv") the network manually > right from the start, and are less likely to override the values retrieved > via > auto-configuration (DHCP etc.). Thanks for the explanation. It looks fine to me. Regards, Simon
diff --git a/cmd/net.c b/cmd/net.c index bed76e4..7ad2540 100644 --- a/cmd/net.c +++ b/cmd/net.c @@ -116,23 +116,23 @@ static void netboot_update_env(void) if (net_gateway.s_addr) { ip_to_string(net_gateway, tmp); - setenv("gatewayip", tmp); + setenv_transient("gatewayip", tmp); } if (net_netmask.s_addr) { ip_to_string(net_netmask, tmp); - setenv("netmask", tmp); + setenv_transient("netmask", tmp); } if (net_hostname[0]) - setenv("hostname", net_hostname); + setenv_transient("hostname", net_hostname); if (net_root_path[0]) - setenv("rootpath", net_root_path); + setenv_transient("rootpath", net_root_path); if (net_ip.s_addr) { ip_to_string(net_ip, tmp); - setenv("ipaddr", tmp); + setenv_transient("ipaddr", tmp); } #if !defined(CONFIG_BOOTP_SERVERIP) /* @@ -141,32 +141,32 @@ static void netboot_update_env(void) */ if (net_server_ip.s_addr) { ip_to_string(net_server_ip, tmp); - setenv("serverip", tmp); + setenv_transient("serverip", tmp); } #endif if (net_dns_server.s_addr) { ip_to_string(net_dns_server, tmp); - setenv("dnsip", tmp); + setenv_transient("dnsip", tmp); } #if defined(CONFIG_BOOTP_DNS2) if (net_dns_server2.s_addr) { ip_to_string(net_dns_server2, tmp); - setenv("dnsip2", tmp); + setenv_transient("dnsip2", tmp); } #endif if (net_nis_domain[0]) - setenv("domain", net_nis_domain); + setenv_transient("domain", net_nis_domain); #if defined(CONFIG_CMD_SNTP) && defined(CONFIG_BOOTP_TIMEOFFSET) if (net_ntp_time_offset) { sprintf(tmp, "%d", net_ntp_time_offset); - setenv("timeoffset", tmp); + setenv_transient("timeoffset", tmp); } #endif #if defined(CONFIG_CMD_SNTP) && defined(CONFIG_BOOTP_NTPSERVER) if (net_ntp_server.s_addr) { ip_to_string(net_ntp_server, tmp); - setenv("ntpserverip", tmp); + setenv_transient("ntpserverip", tmp); } #endif } @@ -291,14 +291,14 @@ static void cdp_update_env(void) printf("CDP offered appliance VLAN %d\n", ntohs(cdp_appliance_vlan)); vlan_to_string(cdp_appliance_vlan, tmp); - setenv("vlan", tmp); + setenv_transient("vlan", tmp); net_our_vlan = cdp_appliance_vlan; } if (cdp_native_vlan != htons(-1)) { printf("CDP offered native VLAN %d\n", ntohs(cdp_native_vlan)); vlan_to_string(cdp_native_vlan, tmp); - setenv("nvlan", tmp); + setenv_transient("nvlan", tmp); net_native_vlan = cdp_native_vlan; } } @@ -423,14 +423,14 @@ static int do_link_local(cmd_tbl_t *cmdtp, int flag, int argc, net_gateway.s_addr = 0; ip_to_string(net_gateway, tmp); - setenv("gatewayip", tmp); + setenv_transient("gatewayip", tmp); ip_to_string(net_netmask, tmp); - setenv("netmask", tmp); + setenv_transient("netmask", tmp); ip_to_string(net_ip, tmp); - setenv("ipaddr", tmp); - setenv("llipaddr", tmp); /* store this for next time */ + setenv_transient("ipaddr", tmp); + setenv_transient("llipaddr", tmp); /* store this for next time */ return CMD_RET_SUCCESS; }
This is an attempt to prevent such information from ending up in exported environment data, especially when doing "saveenv". (http://lists.denx.de/pipermail/u-boot/2015-September/227611.html) The patch makes use of the new setenv_transient() helper for environment variables that get updated via network configuration: BOOTP/DHCP (netboot_update_env), CDP (cdp_update_env) and link-local protocol (do_link_local). Signed-off-by: Bernhard Nortmann <bernhard.nortmann@web.de> --- Changes in v2: None cmd/net.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)