net/ipv6: addr_gen_mode fixes
Subsystems (4)
v2 cover net/ipv6: addr_gen_mode fixes
This series fixes bugs in handling of the addr_gen_mode option, mainly related to the sysctl. A minor netlink issue was also present in the initial commit introducing the option on a per-netdevice basis.
v2: add patch 4, requested by David Ahern during review of v1
add patch 5, missing documentation for the sysctl patches 1, 2, 3 are unchanged
Sabrina Dubroca (5):
net/ipv6: fix addrconf_sysctl_addr_gen_mode
net/ipv6: don't reinitialize ndev->cnf.addr_gen_mode on new inet6_dev net/ipv6: reserve room for IFLA_INET6_ADDR_GEN_MODE net/ipv6: propagate net.ipv6.conf.all.addr_gen_mode
Documentation: ip-sysctl.txt: document addr_gen_mode
Documentation/networking/ip-sysctl.txt | 10 ++++++++ net/ipv6/addrconf.c | 45 ++++++++++++++++++++++------------ 2 files changed, 39 insertions(+), 16 deletions(-)
Signature · 1 line
2.18.0
PATCH v2 1/5 net/ipv6: fix addrconf_sysctl_addr_gen_mode
addrconf_sysctl_addr_gen_mode() has multiple problems. First, it ignores the errors returned by proc_dointvec().
addrconf_sysctl_addr_gen_mode() calls proc_dointvec() directly, which writes the value to memory, and then checks if it's valid and may return EINVAL. If a bad value is given, the value displayed when reading net.ipv6.conf.foo.addr_gen_mode next time will be invalid. In case the value provided by the user was valid, addrconf_dev_config() won't be called since idev->cnf.addr_gen_mode has already been updated.
Fix this in the usual way we deal with values that need to be checked after the proc_do*() helper has returned: define a local ctl_table and storage, call proc_dointvec() on that temporary area, then check and store.
addrconf_sysctl_addr_gen_mode() also writes the new value to the global ipv6_devconf_dflt, when we're writing to some netns's default, so that new netns will inherit the value that was set by the change occuring in any netns. That doesn't make any sense, so let's drop this assignment.
Finally, since addr_gen_mode is a __u32, switch to proc_douintvec().
--- net/ipv6/addrconf.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)
net/ipv6/addrconf.c +14 -13
@@ -5892,32 +5892,31 @@ static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write, loff_t *ppos) {int ret =0 ; -int new_val; +u32 new_val;struct inet6_dev *idev = (struct inet6_dev *)ctl->extra1;struct net *net = (struct net *)ctl->extra2; +struct ctl_table tmp = { + .data = &new_val, + .maxlen =sizeof (new_val), + .mode = ctl->mode, + };if (!rtnl_trylock ())return restart_syscall (); - ret =proc_dointvec (ctl, write, buffer, lenp, ppos); + new_val = *((u32 *)ctl->data); -if (write) { - new_val = *((int *)ctl->data); + ret =proc_douintvec (&tmp, write, buffer, lenp, ppos); +if (ret !=0 ) + goto out; +if (write) {if (check_addr_gen_mode (new_val) <0 ) { ret = -EINVAL ; goto out; } -/* request for default */ -if (&net->ipv6.devconf_dflt->addr_gen_mode == ctl->data) { - ipv6_devconf_dflt.addr_gen_mode = new_val; - -/* request for individual net device */ - }else { -if (!idev) - goto out; - +if (idev) {if (check_stable_privacy (idev, net, new_val) <0 ) { ret = -EINVAL ; goto out;
@@ -5928,6 +5927,8 @@ static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write,addrconf_dev_config (idev->dev); } } + + *((u32 *)ctl->data) = new_val; } out:
Signature · 1 line
2.18.0
Re: [PATCH net v2 1/5] net/ipv6: fix addrconf_sysctl_addr_gen_mode
Quoted 20 lines · Sabrina Dubroca
addrconf_sysctl_addr_gen_mode()has multiple problems. First, it ignores the errors returned byproc_dointvec().
addrconf_sysctl_addr_gen_mode()callsproc_dointvec()directly, which writes the value to memory, and then checks if it's valid and may return EINVAL. If a bad value is given, the value displayed when reading net.ipv6.conf.foo.addr_gen_mode next time will be invalid. In case the value provided by the user was valid,addrconf_dev_config()won't be called sinceidev->cnf.addr_gen_modehas already been updated.Fix this in the usual way we deal with values that need to be checked after the proc_do*() helper has returned: define a local
ctl_tableand storage, callproc_dointvec()on that temporary area, then check and store.
addrconf_sysctl_addr_gen_mode()also writes the new value to the globalipv6_devconf_dflt, when we're writing to some netns's default, so that new netns will inherit the value that was set by the change occuring in any netns. That doesn't make any sense, so let's drop this assignment.
Quoted 8 lines
Finally, since
addr_gen_modeis a__u32, switch toproc_douintvec().Fixes d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")Signed-off-by Sabrina Dubroca <[email protected]>--- net/ipv6/addrconf.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)
PATCH v2 2/5 net/ipv6: don't reinitialize ndev->cnf.addr_gen_mode on new inet6_dev
In reply to Sabrina DubrocaThe value has already been copied from this netns's devconf_dflt, it shouldn't be reset to the global kernel default.
--- net/ipv6/addrconf.c | 2 -- 1 file changed, 2 deletions(-)
net/ipv6/addrconf.c +0 -2
@@ -385,8 +385,6 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)if (ndev->cnf.stable_secret.initialized) ndev->cnf.addr_gen_mode =IN6_ADDR_GEN_MODE_STABLE_PRIVACY ; -else - ndev->cnf.addr_gen_mode = ipv6_devconf_dflt.addr_gen_mode; ndev->cnf.mtu6 = dev->mtu; ndev->nd_parms =neigh_parms_alloc (dev, &nd_tbl);
Signature · 1 line
2.18.0
PATCH v2 3/5 net/ipv6: reserve room for IFLA_INET6_ADDR_GEN_MODE
In reply to Sabrina Dubrocainet6_ifla6_size() is called to check how much space is needed by inet6_fill_link_af() and inet6_fill_ifinfo(), both of which include the IFLA_INET6_ADDR_GEN_MODE attribute. Reserve some room for it.
--- net/ipv6/addrconf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
net/ipv6/addrconf.c +3 -1
@@ -5208,7 +5208,9 @@ static inline size_t inet6_ifla6_size(void) +nla_total_size (DEVCONF_MAX *4 )/* IFLA_INET6_CONF */ +nla_total_size (IPSTATS_MIB_MAX *8 )/* IFLA_INET6_STATS */ +nla_total_size (ICMP6_MIB_MAX *8 )/* IFLA_INET6_ICMP6STATS */ - +nla_total_size (sizeof (struct in6_addr ));/* IFLA_INET6_TOKEN */ + +nla_total_size (sizeof (struct in6_addr ))/* IFLA_INET6_TOKEN */ + +nla_total_size (1 )/* IFLA_INET6_ADDR_GEN_MODE */ + +0 ; }static inline size_t inet6_if_nlmsg_size (void )
Signature · 1 line
2.18.0
PATCH v2 4/5 net/ipv6: propagate net.ipv6.conf.all.addr_gen_mode to devices
In reply to Sabrina DubrocaThis aligns the addr_gen_mode sysctl with the expected behavior of the "all" variant.
--- net/ipv6/addrconf.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
net/ipv6/addrconf.c +12 -0
@@ -5926,6 +5926,18 @@ static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write, idev->cnf.addr_gen_mode = new_val;addrconf_dev_config (idev->dev); } + }else if (&net->ipv6.devconf_all->addr_gen_mode == ctl->data) { +struct net_device *dev; + + net->ipv6.devconf_dflt->addr_gen_mode = new_val; +for_each_netdev (net, dev) { + idev =__in6_dev_get (dev); +if (idev && + idev->cnf.addr_gen_mode != new_val) { + idev->cnf.addr_gen_mode = new_val; +addrconf_dev_config (idev->dev); + } + } } *((u32 *)ctl->data) = new_val;
Signature · 1 line
2.18.0
Re: [PATCH net v2 4/5] net/ipv6: propagate net.ipv6.conf.all.addr_gen_mode to devices
Quoted 18 lines · Sabrina Dubroca
This aligns the
addr_gen_modesysctl with the expected behavior of the "all" variant.Fixes d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")Suggested-by David Ahern <[email protected]>Signed-off-by Sabrina Dubroca <[email protected]>--- net/ipv6/addrconf.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)net/ipv6/addrconf.c +0
-0@@ -5926,6 +5926,18 @@ static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write, idev->cnf.addr_gen_mode = new_val;addrconf_dev_config (idev->dev); }
net/ipv6/addrconf.c static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write, · line 5938
Quoted 10 lines
net/ipv6/addrconf.c +10
-0+ }else if (&net->ipv6.devconf_all->addr_gen_mode == ctl->data) { +struct net_device *dev; + + net->ipv6.devconf_dflt->addr_gen_mode = new_val; +for_each_netdev (net, dev) { + idev =__in6_dev_get (dev); +if (idev && + idev->cnf.addr_gen_mode != new_val) { + idev->cnf.addr_gen_mode = new_val; +addrconf_dev_config (idev->dev);
This call is adding a new LL address without removing the previous one:
# ip -6 addr sh dev eth2
4: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000
inet6 2001:db8:2::4/64 scope global
valid_lft forever preferred_lft forever
inet6 fe80::e0:f9ff:fe45:6480/64 scope link
valid_lft forever preferred_lft forever# sysctl -w net.ipv6.conf.eth2.addr_gen_mode=3 net.ipv6.conf.eth2.addr_gen_mode = 3
# ip -6 addr sh dev eth2
4: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000
inet6 2001:db8:2::4/64 scope global
valid_lft forever preferred_lft forever
inet6 fe80::bc31:8009:270d:e019/64 scope link stable-privacy
valid_lft forever preferred_lft forever
inet6 fe80::e0:f9ff:fe45:6480/64 scope link
valid_lft forever preferred_lft foreverQuoted 6 lines
net/ipv6/addrconf.c +2
-0+ } + } } *((u32 *)ctl->data) = new_val;
Re: [PATCH net v2 4/5] net/ipv6: propagate net.ipv6.conf.all.addr_gen_mode to devices
Quoted 42 lines · David Ahern
Quoted 18 lines · Sabrina Dubroca
This aligns the
addr_gen_modesysctl with the expected behavior of the "all" variant.Fixes d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")Suggested-by David Ahern <[email protected]>Signed-off-by Sabrina Dubroca <[email protected]>--- net/ipv6/addrconf.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)net/ipv6/addrconf.c +0
-0@@ -5926,6 +5926,18 @@ static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write, idev->cnf.addr_gen_mode = new_val;addrconf_dev_config (idev->dev); }net/ipv6/addrconf.c static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write, · line 5938
Quoted 10 lines
net/ipv6/addrconf.c +10
-0+ }else if (&net->ipv6.devconf_all->addr_gen_mode == ctl->data) { +struct net_device *dev; + + net->ipv6.devconf_dflt->addr_gen_mode = new_val; +for_each_netdev (net, dev) { + idev =__in6_dev_get (dev); +if (idev && + idev->cnf.addr_gen_mode != new_val) { + idev->cnf.addr_gen_mode = new_val; +addrconf_dev_config (idev->dev);This call is adding a new LL address without removing the previous one:
# ip -6 addr sh dev eth2 4: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000 inet6 2001:db8:2::4/64 scope global valid_lft forever preferred_lft forever inet6 fe80::e0:f9ff:fe45:6480/64 scope link valid_lft forever preferred_lft forever# sysctl -w net.ipv6.conf.eth2.addr_gen_mode=3 net.ipv6.conf.eth2.addr_gen_mode = 3
Quoted 8 lines
# ip -6 addr sh dev eth2
4: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000
inet6 2001:db8:2::4/64 scope global
valid_lft forever preferred_lft forever
inet6 fe80::bc31:8009:270d:e019/64 scope link stable-privacy
valid_lft forever preferred_lft forever
inet6 fe80::e0:f9ff:fe45:6480/64 scope link
valid_lft forever preferred_lft foreverYes. That's also what will happen with global addresses, once the next RA is received: a new address corresponding to the new generation mode will be added, and the old one isn't removed.
I think that was the expected behavior of d35a00b8e33d, but since it never actually worked... OTOH, the netlink attribute only sets idev->cnf.addr_gen_mode and doesn't add the new LL address (not until a DOWN/UP cycle), which I personally find surprising. If I set the mode to random or stable_secret, I would expect the privacy address to show up without having to take the device down and then up.
I think removing the previous address immediately would break things (and the user wouldn't expect an address to disappear that way, since they're not explicitly asking for it to be removed), but I guess we could play games with the lifetimes (reduce the lifetime of the old address from forever to some limit). That limit would need to be configurable I think, and I would rather target that change for net-next.
Signature · 1 line
Sabrina
Re: [PATCH net v2 4/5] net/ipv6: propagate net.ipv6.conf.all.addr_gen_mode to devices
Quoted 52 lines · Sabrina Dubroca
Quoted 42 lines · David Ahern
Quoted 18 lines · Sabrina Dubroca
This aligns the
addr_gen_modesysctl with the expected behavior of the "all" variant.Fixes d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")Suggested-by David Ahern <[email protected]>Signed-off-by Sabrina Dubroca <[email protected]>--- net/ipv6/addrconf.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)net/ipv6/addrconf.c +0
-0@@ -5926,6 +5926,18 @@ static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write, idev->cnf.addr_gen_mode = new_val;addrconf_dev_config (idev->dev); }net/ipv6/addrconf.c static int addrconf_sysctl_addr_gen_mode(struct ctl_table *ctl, int write, · line 5938
Quoted 10 lines
net/ipv6/addrconf.c +10
-0+ }else if (&net->ipv6.devconf_all->addr_gen_mode == ctl->data) { +struct net_device *dev; + + net->ipv6.devconf_dflt->addr_gen_mode = new_val; +for_each_netdev (net, dev) { + idev =__in6_dev_get (dev); +if (idev && + idev->cnf.addr_gen_mode != new_val) { + idev->cnf.addr_gen_mode = new_val; +addrconf_dev_config (idev->dev);This call is adding a new LL address without removing the previous one:
# ip -6 addr sh dev eth2 4: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000 inet6 2001:db8:2::4/64 scope global valid_lft forever preferred_lft forever inet6 fe80::e0:f9ff:fe45:6480/64 scope link valid_lft forever preferred_lft forever# sysctl -w net.ipv6.conf.eth2.addr_gen_mode=3 net.ipv6.conf.eth2.addr_gen_mode = 3Quoted 8 lines
# ip -6 addr sh dev eth2 4: eth2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 state UP qlen 1000 inet6 2001:db8:2::4/64 scope global valid_lft forever preferred_lft forever inet6 fe80::bc31:8009:270d:e019/64 scope link stable-privacy valid_lft forever preferred_lft forever inet6 fe80::e0:f9ff:fe45:6480/64 scope link valid_lft forever preferred_lft forever
Quoted 3 lines
Yes. That's also what will happen with global addresses, once the next RA is received: a new address corresponding to the new generation mode will be added, and the old one isn't removed.
Interesting.
Quoted 8 lines
I think that was the expected behavior of
d35a00b8e33d, but since it never actually worked... OTOH, the netlink attribute only setsidev->cnf.addr_gen_modeand doesn't add the new LL address (not until a DOWN/UP cycle), which I personally find surprising. If I set the mode to random orstable_secret, I would expect the privacy address to show up without having to take the device down and then up.
Quoted 8 lines
I think removing the previous address immediately would break things (and the user wouldn't expect an address to disappear that way, since they're not explicitly asking for it to be removed), but I guess we could play games with the lifetimes (reduce the lifetime of the old address from forever to some limit). That limit would need to be configurable I think, and I would rather target that change for net-next.
The setting is address generation mode. Addresses are generated on admin up only - I believe this is well established behavior. If the documentation contains a note that changing the mode requires an admin down/up, then it should not be a surprise to users.
I can't imagine this setting is changed on the fly and expected to work immediately. Rather I would expect this is set at boot via the sysctl files or by an interface manager before interfaces are brought up.
I'll ask around, but I am traveling today so will be a delay getting back on this.
PATCH v2 5/5 Documentation: ip-sysctl.txt: document addr_gen_mode
In reply to Sabrina Dubrocaaddr_gen_mode was introduced in without documentation, add it now.
--- Documentation/networking/ip-sysctl.txt | 9 +++++++++ 1 file changed, 9 insertions(+)
Documentation/networking/ip-sysctl.txt +10 -0
@@ -1834,6 +1834,16 @@ stable_secret - IPv6 address By default the stable secret is unset. +addr_gen_mode - INTEGER + Defines how link-local and autoconf addresses are generated. + + 0: generate address based on EUI64 (default) + 1: do no generate a link-local address, use EUI64 for addresses generated + from autoconf + 2: generate stable privacy addresses, using the secret from + stable_secret (RFC7217) + 3: generate stable privacy addresses, using a random secret if unset + drop_unicast_in_l2_multicast - BOOLEAN Drop any unicast IPv6 packets that are received in link-layer multicast (or broadcast) frames.
Re: [PATCH net v2 5/5] Documentation: ip-sysctl.txt: document addr_gen_mode
Quoted 8 lines · Sabrina Dubroca
addr_gen_modewas introduced in without documentation, add it now.Fixes d35a00b8e33d ("net/ipv6: allow sysctl to change link-local address generation mode")Signed-off-by Sabrina Dubroca <[email protected]>--- Documentation/networking/ip-sysctl.txt | 9 +++++++++ 1 file changed, 9 insertions(+)
David Miller
maintainerFrom: Sabrina Dubroca <[email protected]>
Date: Mon, 9 Jul 2018 12:25:13 +0200
Quoted 7 lines
This series fixes bugs in handling of the
addr_gen_modeoption, mainly related to the sysctl. A minor netlink issue was also present in the initial commit introducing the option on a per-netdevice basis.v2: add patch 4, requested by David Ahern during review of v1
add patch 5, missing documentation for the sysctl patches 1, 2, 3 are unchanged
I know there is still some discussion going on about sysctl semantics, but I'll aply this for now and any further refinements can be submitted on top.
Thanks.