Re: [PATCH net-next v2] ipv6: sr: fix IPv6 initialization failure without lwtunnels
From: Roopa Prabhu Date: Wed, 16 Nov 2016 07:49:19 -0800 > On 11/15/16, 7:18 AM, David Miller wrote: >> Although I'd like to entertain the idea of making LWTUNNEL >> unconditionally built and considered a fundamental piece of >> networking infrastructure just like net/core/dst.c > ok, ack. I can submit a patch for that. But, I had the lwtunnel infra hooks in > CONFIG_LWTUNNEL to reduce the cost of hooks in the default fast path when it > was not enabled. > Will need to re-evaluate the cost of the hooks in the default fast-path. ... > I am assuming you are ok with various encaps staying in their > respective configs (mpls iptunnels, ila, and now ipv6 segment > routing). Yes.
Re: [PATCH net-next v2] ipv6: sr: fix IPv6 initialization failure without lwtunnels
On 11/15/16, 7:18 AM, David Miller wrote: > From: David Lebrun > Date: Tue, 15 Nov 2016 11:17:20 +0100 > >> On 11/14/2016 03:22 PM, Roopa Prabhu wrote: >>> I prefer option b). most LWTUNNEL encaps are done this way. >>> >>> seg6 and seg6_iptunnel is new segment routing code and can be under >>> CONFIG_IPV6_SEG6 which depends on CONFIG_LWTUNNEL and CONFIG_IPV6. >>> CONFIG_IPV6_SEG6_HMAC could then depend on CONFIG_IPV6_SEG6 >> Will do that, thanks > This is good for the time being. > > Although I'd like to entertain the idea of making LWTUNNEL > unconditionally built and considered a fundamental piece of > networking infrastructure just like net/core/dst.c ok, ack. I can submit a patch for that. But, I had the lwtunnel infra hooks in CONFIG_LWTUNNEL to reduce the cost of hooks in the default fast path when it was not enabled. Will need to re-evaluate the cost of the hooks in the default fast-path. I am assuming you are ok with various encaps staying in their respective configs (mpls iptunnels, ila, and now ipv6 segment routing). thanks
Re: [PATCH net-next v2] ipv6: sr: fix IPv6 initialization failure without lwtunnels
From: David Lebrun Date: Tue, 15 Nov 2016 11:17:20 +0100 > On 11/14/2016 03:22 PM, Roopa Prabhu wrote: >> I prefer option b). most LWTUNNEL encaps are done this way. >> >> seg6 and seg6_iptunnel is new segment routing code and can be under >> CONFIG_IPV6_SEG6 which depends on CONFIG_LWTUNNEL and CONFIG_IPV6. >> CONFIG_IPV6_SEG6_HMAC could then depend on CONFIG_IPV6_SEG6 > > Will do that, thanks This is good for the time being. Although I'd like to entertain the idea of making LWTUNNEL unconditionally built and considered a fundamental piece of networking infrastructure just like net/core/dst.c
Re: [PATCH net-next v2] ipv6: sr: fix IPv6 initialization failure without lwtunnels
On 11/14/2016 03:22 PM, Roopa Prabhu wrote: > I prefer option b). most LWTUNNEL encaps are done this way. > > seg6 and seg6_iptunnel is new segment routing code and can be under > CONFIG_IPV6_SEG6 which depends on CONFIG_LWTUNNEL and CONFIG_IPV6. > CONFIG_IPV6_SEG6_HMAC could then depend on CONFIG_IPV6_SEG6 Will do that, thanks David signature.asc Description: OpenPGP digital signature
Re: [PATCH net-next v2] ipv6: sr: fix IPv6 initialization failure without lwtunnels
On 11/13/16, 11:59 AM, David Lebrun wrote: > On 11/13/2016 06:23 AM, David Miller wrote: >> This seems like such a huge mess, quite frankly. >> >> IPV6-SR has so many strange dependencies, a weird Kconfig option that is >> simply controlling what a responsible sysadmin should be allow to do if >> he chooses anyways. >> >> Every distribution is going to say "¯\_(ツ)_/¯" and just turn the thing >> on in their builds. > Indeed, the issue is that seg6_iptunnel.o was included in obj-y instead > of ipv6-y, triggering the bug when CONFIG_IPV6=m. Fixed with the > following modification to the patch (tested with allyesconfig and > allmodconfig): > > diff --git a/net/ipv6/Makefile b/net/ipv6/Makefile > index 8979d53..a233136 100644 > --- a/net/ipv6/Makefile > +++ b/net/ipv6/Makefile > @@ -53,6 +53,6 @@ obj-$(subst m,y,$(CONFIG_IPV6)) += inet6_hashtables.o > > ifneq ($(CONFIG_IPV6),) > obj-$(CONFIG_NET_UDP_TUNNEL) += ip6_udp_tunnel.o > -obj-$(CONFIG_LWTUNNEL) += seg6_iptunnel.o > +ipv6-$(CONFIG_LWTUNNEL) += seg6_iptunnel.o > obj-y += mcast_snoop.o > endif > > I agree with you that the way to combine the dependencies is strange, > even if they are very few. The part of the IPv6-SR patch that is enabled > by default depends on two things: IPV6 and LWTUNNEL. The problem is that > LWTUNNEL does not depend on IPV6 and is not necessarily enabled. To fix > the bug reported by Lorenzo, I propose to select one the three following > solutions: > > 1. Make LWTUNNEL always enabled (removing the option). >Pros: remove an option >Cons: add always-enabled code > > 2. Create an option IPV6_SEG6_LWTUNNEL, which would select LWTUNNEL and > enable the compilation of seg6_iptunnel.o. >Pros: logically dissociate the part of IPv6-SR that depends on > LWTUNNEL from the core patch and simplifies compilation >Cons: add an option I prefer option b). most LWTUNNEL encaps are done this way. seg6 and seg6_iptunnel is new segment routing code and can be under CONFIG_IPV6_SEG6 which depends on CONFIG_LWTUNNEL and CONFIG_IPV6. CONFIG_IPV6_SEG6_HMAC could then depend on CONFIG_IPV6_SEG6 > > 3. Apply the proposed patch with the fix >Pros: do not modify options >Cons: weird conditional compilation > > What do you think ? > > David >
Re: [PATCH net-next v2] ipv6: sr: fix IPv6 initialization failure without lwtunnels
On 11/13/2016 06:23 AM, David Miller wrote: > This seems like such a huge mess, quite frankly. > > IPV6-SR has so many strange dependencies, a weird Kconfig option that is > simply controlling what a responsible sysadmin should be allow to do if > he chooses anyways. > > Every distribution is going to say "¯\_(ツ)_/¯" and just turn the thing > on in their builds. Indeed, the issue is that seg6_iptunnel.o was included in obj-y instead of ipv6-y, triggering the bug when CONFIG_IPV6=m. Fixed with the following modification to the patch (tested with allyesconfig and allmodconfig): diff --git a/net/ipv6/Makefile b/net/ipv6/Makefile index 8979d53..a233136 100644 --- a/net/ipv6/Makefile +++ b/net/ipv6/Makefile @@ -53,6 +53,6 @@ obj-$(subst m,y,$(CONFIG_IPV6)) += inet6_hashtables.o ifneq ($(CONFIG_IPV6),) obj-$(CONFIG_NET_UDP_TUNNEL) += ip6_udp_tunnel.o -obj-$(CONFIG_LWTUNNEL) += seg6_iptunnel.o +ipv6-$(CONFIG_LWTUNNEL) += seg6_iptunnel.o obj-y += mcast_snoop.o endif I agree with you that the way to combine the dependencies is strange, even if they are very few. The part of the IPv6-SR patch that is enabled by default depends on two things: IPV6 and LWTUNNEL. The problem is that LWTUNNEL does not depend on IPV6 and is not necessarily enabled. To fix the bug reported by Lorenzo, I propose to select one the three following solutions: 1. Make LWTUNNEL always enabled (removing the option). Pros: remove an option Cons: add always-enabled code 2. Create an option IPV6_SEG6_LWTUNNEL, which would select LWTUNNEL and enable the compilation of seg6_iptunnel.o. Pros: logically dissociate the part of IPv6-SR that depends on LWTUNNEL from the core patch and simplifies compilation Cons: add an option 3. Apply the proposed patch with the fix Pros: do not modify options Cons: weird conditional compilation What do you think ? David signature.asc Description: OpenPGP digital signature
Re: [PATCH net-next v2] ipv6: sr: fix IPv6 initialization failure without lwtunnels
From: David Miller
Date: Sun, 13 Nov 2016 00:20:55 -0500 (EST)
> From: David Lebrun
> Date: Thu, 10 Nov 2016 13:26:53 +0100
>
>> v2: fix conditional compilation for seg6_iptunnel.o in Makefile
>>
>> This patch compiles SR lwtunnels support only if CONFIG_LWTUNNEL=y.
>>
>> If IPv6 is enabled and CONFIG_LWTUNNEL=n, then seg6_iptunnel_init()
>> fails with EOPNOTSUPP which in turn makes seg6_init() fail, blocking
>> the IPv6 initialization, with the following messages:
>>
>> NET: Registered protocol family 10
>> IPv6: Attempt to unregister permanent protocol 6
>> IPv6: Attempt to unregister permanent protocol 136
>> IPv6: Attempt to unregister permanent protocol 17
>> NET: Unregistered protocol family 10
>>
>> Fix commit 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and
>> injection with lwtunnels")
>>
>> Tested with various combinations of CONFIG_IPV6 and CONFIG_LWTUNNEL.
>>
>> Reported-by: Lorenzo Colitti
>> Signed-off-by: David Lebrun
>
> Applied, thanks David.
Actually reverted, after just doing an "make oldconfig" on an
"make allmodconfig" tree after applying this patch the build
fails to link:
[davem@localhost net-next]$ make -s -j8
DESCEND objtool
net/built-in.o: In function `seg6_build_state':
seg6_iptunnel.c:(.text+0x1b7fbe): undefined reference to `seg6_validate_srh'
net/built-in.o: In function `seg6_do_srh':
seg6_iptunnel.c:(.text+0x1b8ad2): undefined reference to `ipv6_dev_get_saddr'
net/built-in.o: In function `seg6_input':
(.text+0x1b8eeb): undefined reference to `ip6_route_input'
net/built-in.o: In function `seg6_output':
(.text+0x1b9151): undefined reference to `ip6_route_output_flags'
Makefile:959: recipe for target 'vmlinux' failed
make: *** [vmlinux] Error 1
This seems like such a huge mess, quite frankly.
IPV6-SR has so many strange dependencies, a weird Kconfig option that is
simply controlling what a responsible sysadmin should be allow to do if
he chooses anyways.
Every distribution is going to say "¯\_(ツ)_/¯" and just turn the thing
on in their builds.
Re: [PATCH net-next v2] ipv6: sr: fix IPv6 initialization failure without lwtunnels
From: David Lebrun
Date: Thu, 10 Nov 2016 13:26:53 +0100
> v2: fix conditional compilation for seg6_iptunnel.o in Makefile
>
> This patch compiles SR lwtunnels support only if CONFIG_LWTUNNEL=y.
>
> If IPv6 is enabled and CONFIG_LWTUNNEL=n, then seg6_iptunnel_init()
> fails with EOPNOTSUPP which in turn makes seg6_init() fail, blocking
> the IPv6 initialization, with the following messages:
>
> NET: Registered protocol family 10
> IPv6: Attempt to unregister permanent protocol 6
> IPv6: Attempt to unregister permanent protocol 136
> IPv6: Attempt to unregister permanent protocol 17
> NET: Unregistered protocol family 10
>
> Fix commit 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and
> injection with lwtunnels")
>
> Tested with various combinations of CONFIG_IPV6 and CONFIG_LWTUNNEL.
>
> Reported-by: Lorenzo Colitti
> Signed-off-by: David Lebrun
Applied, thanks David.
[PATCH net-next v2] ipv6: sr: fix IPv6 initialization failure without lwtunnels
v2: fix conditional compilation for seg6_iptunnel.o in Makefile
This patch compiles SR lwtunnels support only if CONFIG_LWTUNNEL=y.
If IPv6 is enabled and CONFIG_LWTUNNEL=n, then seg6_iptunnel_init()
fails with EOPNOTSUPP which in turn makes seg6_init() fail, blocking
the IPv6 initialization, with the following messages:
NET: Registered protocol family 10
IPv6: Attempt to unregister permanent protocol 6
IPv6: Attempt to unregister permanent protocol 136
IPv6: Attempt to unregister permanent protocol 17
NET: Unregistered protocol family 10
Fix commit 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and
injection with lwtunnels")
Tested with various combinations of CONFIG_IPV6 and CONFIG_LWTUNNEL.
Reported-by: Lorenzo Colitti
Signed-off-by: David Lebrun
---
net/ipv6/Kconfig | 1 +
net/ipv6/Makefile | 3 ++-
net/ipv6/seg6.c | 8
3 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/Kconfig b/net/ipv6/Kconfig
index 0f00811..030cdb6 100644
--- a/net/ipv6/Kconfig
+++ b/net/ipv6/Kconfig
@@ -292,6 +292,7 @@ config IPV6_PIMSM_V2
config IPV6_SEG6_INLINE
bool "IPv6: direct Segment Routing Header insertion "
depends on IPV6
+ depends on LWTUNNEL
---help---
Support for direct insertion of the Segment Routing Header,
also known as inline mode. Be aware that direct insertion of
diff --git a/net/ipv6/Makefile b/net/ipv6/Makefile
index 129cad2..8979d53 100644
--- a/net/ipv6/Makefile
+++ b/net/ipv6/Makefile
@@ -9,7 +9,7 @@ ipv6-objs :=af_inet6.o anycast.o ip6_output.o ip6_input.o
addrconf.o \
route.o ip6_fib.o ipv6_sockglue.o ndisc.o udp.o udplite.o \
raw.o icmp.o mcast.o reassembly.o tcp_ipv6.o ping.o \
exthdrs.o datagram.o ip6_flowlabel.o inet6_connection_sock.o \
- udp_offload.o seg6.o seg6_iptunnel.o
+ udp_offload.o seg6.o
ipv6-offload :=ip6_offload.o tcpv6_offload.o exthdrs_offload.o
@@ -53,5 +53,6 @@ obj-$(subst m,y,$(CONFIG_IPV6)) += inet6_hashtables.o
ifneq ($(CONFIG_IPV6),)
obj-$(CONFIG_NET_UDP_TUNNEL) += ip6_udp_tunnel.o
+obj-$(CONFIG_LWTUNNEL) += seg6_iptunnel.o
obj-y += mcast_snoop.o
endif
diff --git a/net/ipv6/seg6.c b/net/ipv6/seg6.c
index 50f6e06..0f74f90 100644
--- a/net/ipv6/seg6.c
+++ b/net/ipv6/seg6.c
@@ -451,9 +451,11 @@ int __init seg6_init(void)
if (err)
goto out_unregister_genl;
+#ifdef CONFIG_LWTUNNEL
err = seg6_iptunnel_init();
if (err)
goto out_unregister_pernet;
+#endif
#ifdef CONFIG_IPV6_SEG6_HMAC
err = seg6_hmac_init();
@@ -467,10 +469,14 @@ int __init seg6_init(void)
return err;
#ifdef CONFIG_IPV6_SEG6_HMAC
out_unregister_iptun:
+#ifdef CONFIG_LWTUNNEL
seg6_iptunnel_exit();
#endif
+#endif
+#ifdef CONFIG_LWTUNNEL
out_unregister_pernet:
unregister_pernet_subsys(&ip6_segments_ops);
+#endif
out_unregister_genl:
genl_unregister_family(&seg6_genl_family);
goto out;
@@ -481,7 +487,9 @@ void seg6_exit(void)
#ifdef CONFIG_IPV6_SEG6_HMAC
seg6_hmac_exit();
#endif
+#ifdef CONFIG_LWTUNNEL
seg6_iptunnel_exit();
+#endif
unregister_pernet_subsys(&ip6_segments_ops);
genl_unregister_family(&seg6_genl_family);
}
--
2.7.3
