| Message ID | 20221219010619.1826599-2-Thomas.Winter@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ip/ip6_gre: Fix GRE tunnels not generating IPv6 link local addresses | expand |
On Mon, 2022-12-19 at 14:06 +1300, Thomas Winter wrote: > Commit e5dd729460ca changed the code path so that GRE tunnels > generate an IPv6 address based on the tunnel source address. > It also changed the code path so GRE tunnels don't call addrconf_addr_gen > in addrconf_dev_config which is called by addrconf_sysctl_addr_gen_mode > when the IN6_ADDR_GEN_MODE is changed. > > This patch aims to fix this issue by moving the code in addrconf_notify > which calls the addr gen for GRE and SIT into a separate function > and calling it in the places that expect the IPv6 address to be > generated. > > The previous addrconf_dev_config is renamed to addrconf_eth_config > since it only expected eth type interfaces and follows the > addrconf_gre/sit_config format. > > Fixes: e5dd729460ca ("ip/ip6_gre: use the same logic as SIT interfaces when computing v6LL address") > Signed-off-by: Thomas Winter <Thomas.Winter@alliedtelesis.co.nz> > --- > net/ipv6/addrconf.c | 47 +++++++++++++++++++++++++-------------------- > 1 file changed, 26 insertions(+), 21 deletions(-) > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 6dcf034835ec..e9d7ec03316d 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -3355,7 +3355,7 @@ static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route) > } > } > > -static void addrconf_dev_config(struct net_device *dev) > +static void addrconf_eth_config(struct net_device *dev) You are creating a new function with the name of an existing one, while renaming the latter. IMHO this makes the patch hard to review as there are some existing call side for the old name, which we likelly want to explicitly see here. > { > struct inet6_dev *idev; > > @@ -3447,6 +3447,30 @@ static void addrconf_gre_config(struct net_device *dev) > } > #endif > > +static void addrconf_dev_config(struct net_device *dev) > +{ > + switch (dev->type) { > +#if IS_ENABLED(CONFIG_IPV6_SIT) > + case ARPHRD_SIT: > + addrconf_sit_config(dev); > + break; > +#endif > +#if IS_ENABLED(CONFIG_NET_IPGRE) || IS_ENABLED(CONFIG_IPV6_GRE) > + case ARPHRD_IP6GRE: > + case ARPHRD_IPGRE: > + addrconf_gre_config(dev); > + break; > +#endif > + case ARPHRD_LOOPBACK: > + init_loopback(dev); > + break; If I read correctly, this change will cause unneeded attempt to re-add the loopback address on the loopback device when the lo.addr_gen_mode sysfs entry is touched. I think such side effect should be avoided. Thanks, Paolo
On Tue, 2022-12-20 at 14:16 +0100, Paolo Abeni wrote: > On Mon, 2022-12-19 at 14:06 +1300, Thomas Winter wrote: > > Commit e5dd729460ca changed the code path so that GRE tunnels > > generate an IPv6 address based on the tunnel source address. > > It also changed the code path so GRE tunnels don't call > > addrconf_addr_gen > > in addrconf_dev_config which is called by > > addrconf_sysctl_addr_gen_mode > > when the IN6_ADDR_GEN_MODE is changed. > > > > This patch aims to fix this issue by moving the code in > > addrconf_notify > > which calls the addr gen for GRE and SIT into a separate function > > and calling it in the places that expect the IPv6 address to be > > generated. > > > > The previous addrconf_dev_config is renamed to addrconf_eth_config > > since it only expected eth type interfaces and follows the > > addrconf_gre/sit_config format. > > > > Fixes: e5dd729460ca ("ip/ip6_gre: use the same logic as SIT > > interfaces when computing v6LL address") > > Signed-off-by: Thomas Winter <Thomas.Winter@alliedtelesis.co.nz> > > --- > > net/ipv6/addrconf.c | 47 +++++++++++++++++++++++++-------------- > > ------ > > 1 file changed, 26 insertions(+), 21 deletions(-) > > > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > > index 6dcf034835ec..e9d7ec03316d 100644 > > --- a/net/ipv6/addrconf.c > > +++ b/net/ipv6/addrconf.c > > @@ -3355,7 +3355,7 @@ static void addrconf_addr_gen(struct > > inet6_dev *idev, bool prefix_route) > > } > > } > > > > -static void addrconf_dev_config(struct net_device *dev) > > +static void addrconf_eth_config(struct net_device *dev) > > You are creating a new function with the name of an existing one, > while > renaming the latter. IMHO this makes the patch hard to review as > there > are some existing call side for the old name, which we likelly want > to > explicitly see here. > Perhaps these functions can be named something like "addrconfig_init_auto_addrs". > > { > > struct inet6_dev *idev; > > > > @@ -3447,6 +3447,30 @@ static void addrconf_gre_config(struct > > net_device *dev) > > } > > #endif > > > > +static void addrconf_dev_config(struct net_device *dev) > > +{ > > + switch (dev->type) { > > +#if IS_ENABLED(CONFIG_IPV6_SIT) > > + case ARPHRD_SIT: > > + addrconf_sit_config(dev); > > + break; > > +#endif > > +#if IS_ENABLED(CONFIG_NET_IPGRE) || IS_ENABLED(CONFIG_IPV6_GRE) > > + case ARPHRD_IP6GRE: > > + case ARPHRD_IPGRE: > > + addrconf_gre_config(dev); > > + break; > > +#endif > > + case ARPHRD_LOOPBACK: > > + init_loopback(dev); > > + break; > > If I read correctly, this change will cause unneeded attempt to re- > add > the loopback address on the loopback device when the lo.addr_gen_mode > sysfs entry is touched. I think such side effect should be avoided. My understanding is that these addresses should always exist. It doesn't look like any problem will happen if the loopback address already exists and this called. > > Thanks, > > Paolo >
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c index 6dcf034835ec..e9d7ec03316d 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -3355,7 +3355,7 @@ static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route) } } -static void addrconf_dev_config(struct net_device *dev) +static void addrconf_eth_config(struct net_device *dev) { struct inet6_dev *idev; @@ -3447,6 +3447,30 @@ static void addrconf_gre_config(struct net_device *dev) } #endif +static void addrconf_dev_config(struct net_device *dev) +{ + switch (dev->type) { +#if IS_ENABLED(CONFIG_IPV6_SIT) + case ARPHRD_SIT: + addrconf_sit_config(dev); + break; +#endif +#if IS_ENABLED(CONFIG_NET_IPGRE) || IS_ENABLED(CONFIG_IPV6_GRE) + case ARPHRD_IP6GRE: + case ARPHRD_IPGRE: + addrconf_gre_config(dev); + break; +#endif + case ARPHRD_LOOPBACK: + init_loopback(dev); + break; + + default: + addrconf_eth_config(dev); + break; + } +} + static int fixup_permanent_addr(struct net *net, struct inet6_dev *idev, struct inet6_ifaddr *ifp) @@ -3611,26 +3635,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event, run_pending = 1; } - switch (dev->type) { -#if IS_ENABLED(CONFIG_IPV6_SIT) - case ARPHRD_SIT: - addrconf_sit_config(dev); - break; -#endif -#if IS_ENABLED(CONFIG_NET_IPGRE) || IS_ENABLED(CONFIG_IPV6_GRE) - case ARPHRD_IP6GRE: - case ARPHRD_IPGRE: - addrconf_gre_config(dev); - break; -#endif - case ARPHRD_LOOPBACK: - init_loopback(dev); - break; - - default: - addrconf_dev_config(dev); - break; - } + addrconf_dev_config(dev); if (!IS_ERR_OR_NULL(idev)) { if (run_pending)
Commit e5dd729460ca changed the code path so that GRE tunnels generate an IPv6 address based on the tunnel source address. It also changed the code path so GRE tunnels don't call addrconf_addr_gen in addrconf_dev_config which is called by addrconf_sysctl_addr_gen_mode when the IN6_ADDR_GEN_MODE is changed. This patch aims to fix this issue by moving the code in addrconf_notify which calls the addr gen for GRE and SIT into a separate function and calling it in the places that expect the IPv6 address to be generated. The previous addrconf_dev_config is renamed to addrconf_eth_config since it only expected eth type interfaces and follows the addrconf_gre/sit_config format. Fixes: e5dd729460ca ("ip/ip6_gre: use the same logic as SIT interfaces when computing v6LL address") Signed-off-by: Thomas Winter <Thomas.Winter@alliedtelesis.co.nz> --- net/ipv6/addrconf.c | 47 +++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 21 deletions(-)