| Message ID | 20200730054130.16923-11-steffen.klassert@secunet.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [01/19] xfrm: introduce oseq-may-wrap flag | expand |
Le 30/07/2020 à 07:41, Steffen Klassert a écrit : > From: Xin Long <lucien.xin@gmail.com> > > Similar to ip6_vti, IP6IP6 and IP6IP tunnels processing can easily > be done with .cb_handler for xfrm interface. > > v1->v2: > - no change. > v2-v3: > - enable it only when CONFIG_INET6_XFRM_TUNNEL is defined, to fix > the build error, reported by kbuild test robot. > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> > --- This patch broke standard IP tunnels. With this setup: + ip link set ntfp2 up + ip addr add 10.125.0.2/24 dev ntfp2 + ip tunnel add tun1 mode ipip ttl 64 local 10.125.0.2 remote 10.125.0.1 dev ntfp2 + ip addr add 192.168.0.2/32 peer 192.168.0.1/32 dev tun1 + ip link set dev tun1 up incoming packets are dropped: $ cat /proc/net/xfrm_stat ... XfrmInNoStates 31 ... Xin, can you have a look? > net/xfrm/xfrm_interface.c | 38 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c > index c407ecbc5d46..b9ef496d3d7c 100644 > --- a/net/xfrm/xfrm_interface.c > +++ b/net/xfrm/xfrm_interface.c > @@ -798,6 +798,26 @@ static struct xfrm6_protocol xfrmi_ipcomp6_protocol __read_mostly = { > .priority = 10, > }; > > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) > +static int xfrmi6_rcv_tunnel(struct sk_buff *skb) > +{ > + const xfrm_address_t *saddr; > + __be32 spi; > + > + saddr = (const xfrm_address_t *)&ipv6_hdr(skb)->saddr; > + spi = xfrm6_tunnel_spi_lookup(dev_net(skb->dev), saddr); > + > + return xfrm6_rcv_spi(skb, IPPROTO_IPV6, spi, NULL); > +} > + > +static struct xfrm6_tunnel xfrmi_ipv6_handler __read_mostly = { > + .handler = xfrmi6_rcv_tunnel, > + .cb_handler = xfrmi_rcv_cb, > + .err_handler = xfrmi6_err, > + .priority = -1, > +}; > +#endif > + > static struct xfrm4_protocol xfrmi_esp4_protocol __read_mostly = { > .handler = xfrm4_rcv, > .input_handler = xfrm_input, > @@ -866,9 +886,23 @@ static int __init xfrmi6_init(void) > err = xfrm6_protocol_register(&xfrmi_ipcomp6_protocol, IPPROTO_COMP); > if (err < 0) > goto xfrm_proto_comp_failed; > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) > + err = xfrm6_tunnel_register(&xfrmi_ipv6_handler, AF_INET6); > + if (err < 0) > + goto xfrm_tunnel_ipv6_failed; > + err = xfrm6_tunnel_register(&xfrmi_ipv6_handler, AF_INET); > + if (err < 0) > + goto xfrm_tunnel_ip6ip_failed; > +#endif > > return 0; > > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) > +xfrm_tunnel_ip6ip_failed: > + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET6); > +xfrm_tunnel_ipv6_failed: > + xfrm6_protocol_deregister(&xfrmi_ipcomp6_protocol, IPPROTO_COMP); > +#endif > xfrm_proto_comp_failed: > xfrm6_protocol_deregister(&xfrmi_ah6_protocol, IPPROTO_AH); > xfrm_proto_ah_failed: > @@ -879,6 +913,10 @@ static int __init xfrmi6_init(void) > > static void xfrmi6_fini(void) > { > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) > + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET); > + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET6); > +#endif > xfrm6_protocol_deregister(&xfrmi_ipcomp6_protocol, IPPROTO_COMP); > xfrm6_protocol_deregister(&xfrmi_ah6_protocol, IPPROTO_AH); > xfrm6_protocol_deregister(&xfrmi_esp6_protocol, IPPROTO_ESP); >
On Fri, Oct 2, 2020 at 10:44 PM Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > > Le 30/07/2020 à 07:41, Steffen Klassert a écrit : > > From: Xin Long <lucien.xin@gmail.com> > > > > Similar to ip6_vti, IP6IP6 and IP6IP tunnels processing can easily > > be done with .cb_handler for xfrm interface. > > > > v1->v2: > > - no change. > > v2-v3: > > - enable it only when CONFIG_INET6_XFRM_TUNNEL is defined, to fix > > the build error, reported by kbuild test robot. > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com> > > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com> > > --- > > This patch broke standard IP tunnels. With this setup: > + ip link set ntfp2 up > + ip addr add 10.125.0.2/24 dev ntfp2 > + ip tunnel add tun1 mode ipip ttl 64 local 10.125.0.2 remote 10.125.0.1 dev ntfp2 > + ip addr add 192.168.0.2/32 peer 192.168.0.1/32 dev tun1 > + ip link set dev tun1 up > > incoming packets are dropped: > $ cat /proc/net/xfrm_stat > ... > XfrmInNoStates 31 > ... > > Xin, can you have a look? Sure. When xfrmi processes the ipip packets, it does the state lookup and xfrmi device lookup both in xfrm_input(). When either of them fails, instead of returning err and continuing the next .handler in tunnel4_rcv(), it would drop the packet and return 0. It's kinda the same as xfrm_tunnel_rcv() and xfrm6_tunnel_rcv(). So the safe fix is to lower the priority of xfrmi .handler but it should still be higher than xfrm_tunnel_rcv() and xfrm6_tunnel_rcv(). Having xfrmi loaded will only break IPCOMP, and it's expected. I'll post a fix: diff --git a/net/ipv4/xfrm4_tunnel.c b/net/ipv4/xfrm4_tunnel.c index dc19aff7c2e0..fb0648e7fb32 100644 --- a/net/ipv4/xfrm4_tunnel.c +++ b/net/ipv4/xfrm4_tunnel.c @@ -64,14 +64,14 @@ static int xfrm_tunnel_err(struct sk_buff *skb, u32 info) static struct xfrm_tunnel xfrm_tunnel_handler __read_mostly = { .handler = xfrm_tunnel_rcv, .err_handler = xfrm_tunnel_err, - .priority = 3, + .priority = 4, }; #if IS_ENABLED(CONFIG_IPV6) static struct xfrm_tunnel xfrm64_tunnel_handler __read_mostly = { .handler = xfrm_tunnel_rcv, .err_handler = xfrm_tunnel_err, - .priority = 2, + .priority = 3, }; #endif diff --git a/net/ipv6/xfrm6_tunnel.c b/net/ipv6/xfrm6_tunnel.c index 25b7ebda2fab..f696d46e6910 100644 --- a/net/ipv6/xfrm6_tunnel.c +++ b/net/ipv6/xfrm6_tunnel.c @@ -303,13 +303,13 @@ static const struct xfrm_type xfrm6_tunnel_type = { static struct xfrm6_tunnel xfrm6_tunnel_handler __read_mostly = { .handler = xfrm6_tunnel_rcv, .err_handler = xfrm6_tunnel_err, - .priority = 2, + .priority = 3, }; static struct xfrm6_tunnel xfrm46_tunnel_handler __read_mostly = { .handler = xfrm6_tunnel_rcv, .err_handler = xfrm6_tunnel_err, - .priority = 2, + .priority = 3, }; static int __net_init xfrm6_tunnel_net_init(struct net *net) diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c index a8f66112c52b..0bb7963b9f6b 100644 --- a/net/xfrm/xfrm_interface.c +++ b/net/xfrm/xfrm_interface.c @@ -830,14 +830,14 @@ static struct xfrm6_tunnel xfrmi_ipv6_handler __read_mostly = { .handler = xfrmi6_rcv_tunnel, .cb_handler = xfrmi_rcv_cb, .err_handler = xfrmi6_err, - .priority = -1, + .priority = 2, }; static struct xfrm6_tunnel xfrmi_ip6ip_handler __read_mostly = { .handler = xfrmi6_rcv_tunnel, .cb_handler = xfrmi_rcv_cb, .err_handler = xfrmi6_err, - .priority = -1, + .priority = 2, }; #endif @@ -875,14 +875,14 @@ static struct xfrm_tunnel xfrmi_ipip_handler __read_mostly = { .handler = xfrmi4_rcv_tunnel, .cb_handler = xfrmi_rcv_cb, .err_handler = xfrmi4_err, - .priority = -1, + .priority = 3, }; static struct xfrm_tunnel xfrmi_ipip6_handler __read_mostly = { .handler = xfrmi4_rcv_tunnel, .cb_handler = xfrmi_rcv_cb, .err_handler = xfrmi4_err, - .priority = -1, + .priority = 2, }; #endif > > net/xfrm/xfrm_interface.c | 38 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 38 insertions(+) > > > > diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c > > index c407ecbc5d46..b9ef496d3d7c 100644 > > --- a/net/xfrm/xfrm_interface.c > > +++ b/net/xfrm/xfrm_interface.c > > @@ -798,6 +798,26 @@ static struct xfrm6_protocol xfrmi_ipcomp6_protocol __read_mostly = { > > .priority = 10, > > }; > > > > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) > > +static int xfrmi6_rcv_tunnel(struct sk_buff *skb) > > +{ > > + const xfrm_address_t *saddr; > > + __be32 spi; > > + > > + saddr = (const xfrm_address_t *)&ipv6_hdr(skb)->saddr; > > + spi = xfrm6_tunnel_spi_lookup(dev_net(skb->dev), saddr); > > + > > + return xfrm6_rcv_spi(skb, IPPROTO_IPV6, spi, NULL); > > +} > > + > > +static struct xfrm6_tunnel xfrmi_ipv6_handler __read_mostly = { > > + .handler = xfrmi6_rcv_tunnel, > > + .cb_handler = xfrmi_rcv_cb, > > + .err_handler = xfrmi6_err, > > + .priority = -1, > > +}; > > +#endif > > + > > static struct xfrm4_protocol xfrmi_esp4_protocol __read_mostly = { > > .handler = xfrm4_rcv, > > .input_handler = xfrm_input, > > @@ -866,9 +886,23 @@ static int __init xfrmi6_init(void) > > err = xfrm6_protocol_register(&xfrmi_ipcomp6_protocol, IPPROTO_COMP); > > if (err < 0) > > goto xfrm_proto_comp_failed; > > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) > > + err = xfrm6_tunnel_register(&xfrmi_ipv6_handler, AF_INET6); > > + if (err < 0) > > + goto xfrm_tunnel_ipv6_failed; > > + err = xfrm6_tunnel_register(&xfrmi_ipv6_handler, AF_INET); > > + if (err < 0) > > + goto xfrm_tunnel_ip6ip_failed; > > +#endif > > > > return 0; > > > > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) > > +xfrm_tunnel_ip6ip_failed: > > + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET6); > > +xfrm_tunnel_ipv6_failed: > > + xfrm6_protocol_deregister(&xfrmi_ipcomp6_protocol, IPPROTO_COMP); > > +#endif > > xfrm_proto_comp_failed: > > xfrm6_protocol_deregister(&xfrmi_ah6_protocol, IPPROTO_AH); > > xfrm_proto_ah_failed: > > @@ -879,6 +913,10 @@ static int __init xfrmi6_init(void) > > > > static void xfrmi6_fini(void) > > { > > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) > > + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET); > > + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET6); > > +#endif > > xfrm6_protocol_deregister(&xfrmi_ipcomp6_protocol, IPPROTO_COMP); > > xfrm6_protocol_deregister(&xfrmi_ah6_protocol, IPPROTO_AH); > > xfrm6_protocol_deregister(&xfrmi_esp6_protocol, IPPROTO_ESP); > >
Le 03/10/2020 à 11:41, Xin Long a écrit : [snip] > When xfrmi processes the ipip packets, it does the state lookup and xfrmi > device lookup both in xfrm_input(). When either of them fails, instead of > returning err and continuing the next .handler in tunnel4_rcv(), it would > drop the packet and return 0. > > It's kinda the same as xfrm_tunnel_rcv() and xfrm6_tunnel_rcv(). > > So the safe fix is to lower the priority of xfrmi .handler but it should > still be higher than xfrm_tunnel_rcv() and xfrm6_tunnel_rcv(). Having > xfrmi loaded will only break IPCOMP, and it's expected. I'll post a fix: Thanks. This patch fixes my test cases. Regards, Nicolas
Le 05/10/2020 à 17:11, Nicolas Dichtel a écrit : > Le 03/10/2020 à 11:41, Xin Long a écrit : > [snip] >> When xfrmi processes the ipip packets, it does the state lookup and xfrmi >> device lookup both in xfrm_input(). When either of them fails, instead of >> returning err and continuing the next .handler in tunnel4_rcv(), it would >> drop the packet and return 0. >> >> It's kinda the same as xfrm_tunnel_rcv() and xfrm6_tunnel_rcv(). >> >> So the safe fix is to lower the priority of xfrmi .handler but it should >> still be higher than xfrm_tunnel_rcv() and xfrm6_tunnel_rcv(). Having >> xfrmi loaded will only break IPCOMP, and it's expected. I'll post a fix: > Thanks. This patch fixes my test cases. Do you think that you will have time to send the patch before the release (v5.9) goes out? If not, I can send it ;-)
On Wed, Oct 7, 2020 at 11:40 PM Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote: > > Le 05/10/2020 à 17:11, Nicolas Dichtel a écrit : > > Le 03/10/2020 à 11:41, Xin Long a écrit : > > [snip] > >> When xfrmi processes the ipip packets, it does the state lookup and xfrmi > >> device lookup both in xfrm_input(). When either of them fails, instead of > >> returning err and continuing the next .handler in tunnel4_rcv(), it would > >> drop the packet and return 0. > >> > >> It's kinda the same as xfrm_tunnel_rcv() and xfrm6_tunnel_rcv(). > >> > >> So the safe fix is to lower the priority of xfrmi .handler but it should > >> still be higher than xfrm_tunnel_rcv() and xfrm6_tunnel_rcv(). Having > >> xfrmi loaded will only break IPCOMP, and it's expected. I'll post a fix: > > Thanks. This patch fixes my test cases. > Do you think that you will have time to send the patch before the release (v5.9) > goes out? Sure, I will do it tomorrow.
Le 07/10/2020 à 18:26, Xin Long a écrit : > On Wed, Oct 7, 2020 at 11:40 PM Nicolas Dichtel > <nicolas.dichtel@6wind.com> wrote: [snip] >> Do you think that you will have time to send the patch before the release (v5.9) >> goes out? > Sure, I will do it tomorrow. > Thanks!
diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c index c407ecbc5d46..b9ef496d3d7c 100644 --- a/net/xfrm/xfrm_interface.c +++ b/net/xfrm/xfrm_interface.c @@ -798,6 +798,26 @@ static struct xfrm6_protocol xfrmi_ipcomp6_protocol __read_mostly = { .priority = 10, }; +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) +static int xfrmi6_rcv_tunnel(struct sk_buff *skb) +{ + const xfrm_address_t *saddr; + __be32 spi; + + saddr = (const xfrm_address_t *)&ipv6_hdr(skb)->saddr; + spi = xfrm6_tunnel_spi_lookup(dev_net(skb->dev), saddr); + + return xfrm6_rcv_spi(skb, IPPROTO_IPV6, spi, NULL); +} + +static struct xfrm6_tunnel xfrmi_ipv6_handler __read_mostly = { + .handler = xfrmi6_rcv_tunnel, + .cb_handler = xfrmi_rcv_cb, + .err_handler = xfrmi6_err, + .priority = -1, +}; +#endif + static struct xfrm4_protocol xfrmi_esp4_protocol __read_mostly = { .handler = xfrm4_rcv, .input_handler = xfrm_input, @@ -866,9 +886,23 @@ static int __init xfrmi6_init(void) err = xfrm6_protocol_register(&xfrmi_ipcomp6_protocol, IPPROTO_COMP); if (err < 0) goto xfrm_proto_comp_failed; +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) + err = xfrm6_tunnel_register(&xfrmi_ipv6_handler, AF_INET6); + if (err < 0) + goto xfrm_tunnel_ipv6_failed; + err = xfrm6_tunnel_register(&xfrmi_ipv6_handler, AF_INET); + if (err < 0) + goto xfrm_tunnel_ip6ip_failed; +#endif return 0; +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) +xfrm_tunnel_ip6ip_failed: + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET6); +xfrm_tunnel_ipv6_failed: + xfrm6_protocol_deregister(&xfrmi_ipcomp6_protocol, IPPROTO_COMP); +#endif xfrm_proto_comp_failed: xfrm6_protocol_deregister(&xfrmi_ah6_protocol, IPPROTO_AH); xfrm_proto_ah_failed: @@ -879,6 +913,10 @@ static int __init xfrmi6_init(void) static void xfrmi6_fini(void) { +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL) + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET); + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET6); +#endif xfrm6_protocol_deregister(&xfrmi_ipcomp6_protocol, IPPROTO_COMP); xfrm6_protocol_deregister(&xfrmi_ah6_protocol, IPPROTO_AH); xfrm6_protocol_deregister(&xfrmi_esp6_protocol, IPPROTO_ESP);