| Message ID | 20201107153139.3552-5-andrea.mayer@uniroma2.it (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2,1/5] vrf: add mac header for tunneled packets when sniffer is attached | expand |
On Sat, 7 Nov 2020 16:31:38 +0100 Andrea Mayer wrote: > SRv6 End.DT4 is defined in the SRv6 Network Programming [1]. > > The SRv6 End.DT4 is used to implement IPv4 L3VPN use-cases in > multi-tenants environments. It decapsulates the received packets and it > performs IPv4 routing lookup in the routing table of the tenant. > > The SRv6 End.DT4 Linux implementation leverages a VRF device in order to > force the routing lookup into the associated routing table. How does the behavior of DT4 compare to DT6? The implementation looks quite different. > To make the End.DT4 work properly, it must be guaranteed that the routing > table used for routing lookup operations is bound to one and only one > VRF during the tunnel creation. Such constraint has to be enforced by > enabling the VRF strict_mode sysctl parameter, i.e: > $ sysctl -wq net.vrf.strict_mode=1. > > At JANOG44, LINE corporation presented their multi-tenant DC architecture > using SRv6 [2]. In the slides, they reported that the Linux kernel is > missing the support of SRv6 End.DT4 behavior. > > The iproute2 counterpart required for configuring the SRv6 End.DT4 > behavior is already implemented along with the other supported SRv6 > behaviors [3]. > > [1] https://tools.ietf.org/html/draft-ietf-spring-srv6-network-programming > [2] https://speakerdeck.com/line_developers/line-data-center-networking-with-srv6 > [3] https://patchwork.ozlabs.org/patch/799837/ > > Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it> > --- > net/ipv6/seg6_local.c | 205 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 205 insertions(+) > > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c > index 4b0f155d641d..a41074acd43e 100644 > --- a/net/ipv6/seg6_local.c > +++ b/net/ipv6/seg6_local.c > @@ -57,6 +57,14 @@ struct bpf_lwt_prog { > char *name; > }; > > +struct seg6_end_dt4_info { > + struct net *net; > + /* VRF device associated to the routing table used by the SRv6 End.DT4 > + * behavior for routing IPv4 packets. > + */ > + int vrf_ifindex; > +}; > + > struct seg6_local_lwt { > int action; > struct ipv6_sr_hdr *srh; > @@ -66,6 +74,7 @@ struct seg6_local_lwt { > int iif; > int oif; > struct bpf_lwt_prog bpf; > + struct seg6_end_dt4_info dt4_info; > > int headroom; > struct seg6_action_desc *desc; > @@ -413,6 +422,194 @@ static int input_action_end_dx4(struct sk_buff *skb, > return -EINVAL; > } > > +#ifdef CONFIG_NET_L3_MASTER_DEV > + no need for this empty line. > +static struct net *fib6_config_get_net(const struct fib6_config *fib6_cfg) > +{ > + const struct nl_info *nli = &fib6_cfg->fc_nlinfo; > + > + return nli->nl_net; > +} > + > +static int seg6_end_dt4_build(struct seg6_local_lwt *slwt, const void *cfg, > + struct netlink_ext_ack *extack) > +{ > + struct seg6_end_dt4_info *info = &slwt->dt4_info; > + int vrf_ifindex; > + struct net *net; > + > + net = fib6_config_get_net(cfg); > + > + vrf_ifindex = l3mdev_ifindex_lookup_by_table_id(L3MDEV_TYPE_VRF, net, > + slwt->table); > + if (vrf_ifindex < 0) { > + if (vrf_ifindex == -EPERM) { > + NL_SET_ERR_MSG(extack, > + "Strict mode for VRF is disabled"); > + } else if (vrf_ifindex == -ENODEV) { > + NL_SET_ERR_MSG(extack, "No such device"); That's what -ENODEV already says. > + } else { > + NL_SET_ERR_MSG(extack, "Unknown error"); Useless error. > + pr_debug("seg6local: SRv6 End.DT4 creation error=%d\n", > + vrf_ifindex); > + } > + > + return vrf_ifindex; > + } > + > + info->net = net; > + info->vrf_ifindex = vrf_ifindex; > + > + return 0; > +} > + > +/* The SRv6 End.DT4 behavior extracts the inner (IPv4) packet and routes the > + * IPv4 packet by looking at the configured routing table. > + * > + * In the SRv6 End.DT4 use case, we can receive traffic (IPv6+Segment Routing > + * Header packets) from several interfaces and the IPv6 destination address (DA) > + * is used for retrieving the specific instance of the End.DT4 behavior that > + * should process the packets. > + * > + * However, the inner IPv4 packet is not really bound to any receiving > + * interface and thus the End.DT4 sets the VRF (associated with the > + * corresponding routing table) as the *receiving* interface. > + * In other words, the End.DT4 processes a packet as if it has been received > + * directly by the VRF (and not by one of its slave devices, if any). > + * In this way, the VRF interface is used for routing the IPv4 packet in > + * according to the routing table configured by the End.DT4 instance. > + * > + * This design allows you to get some interesting features like: > + * 1) the statistics on rx packets; > + * 2) the possibility to install a packet sniffer on the receiving interface > + * (the VRF one) for looking at the incoming packets; > + * 3) the possibility to leverage the netfilter prerouting hook for the inner > + * IPv4 packet. > + * > + * This function returns: > + * - the sk_buff* when the VRF rcv handler has processed the packet correctly; > + * - NULL when the skb is consumed by the VRF rcv handler; > + * - a pointer which encodes a negative error number in case of error. > + * Note that in this case, the function takes care of freeing the skb. > + */ > +static struct sk_buff *end_dt4_vrf_rcv(struct sk_buff *skb, > + struct net_device *dev) > +{ > + /* based on l3mdev_ip_rcv; we are only interested in the master */ > + if (unlikely(!netif_is_l3_master(dev) && !netif_has_l3_rx_handler(dev))) > + goto drop; > + > + if (unlikely(!dev->l3mdev_ops->l3mdev_l3_rcv)) > + goto drop; > + > + /* the decap packet (IPv4) does not come with any mac header info. > + * We must unset the mac header to allow the VRF device to rebuild it, > + * just in case there is a sniffer attached on the device. > + */ > + skb_unset_mac_header(skb); > + > + skb = dev->l3mdev_ops->l3mdev_l3_rcv(dev, skb, AF_INET); > + if (!skb) > + /* the skb buffer was consumed by the handler */ > + return NULL; > + > + /* when a packet is received by a VRF or by one of its slaves, the > + * master device reference is set into the skb. > + */ > + if (unlikely(skb->dev != dev || skb->skb_iif != dev->ifindex)) > + goto drop; > + > + return skb; > + > +drop: > + kfree_skb(skb); > + return ERR_PTR(-EINVAL); > +} > + > +static struct net_device *end_dt4_get_vrf_rcu(struct sk_buff *skb, > + struct seg6_end_dt4_info *info) > +{ > + int vrf_ifindex = info->vrf_ifindex; > + struct net *net = info->net; > + > + if (unlikely(vrf_ifindex < 0)) > + goto error; > + > + if (unlikely(!net_eq(dev_net(skb->dev), net))) > + goto error; > + > + return dev_get_by_index_rcu(net, vrf_ifindex); > + > +error: > + return NULL; > +} > + > +static int input_action_end_dt4(struct sk_buff *skb, > + struct seg6_local_lwt *slwt) > +{ > + struct net_device *vrf; > + struct iphdr *iph; > + int err; > + > + if (!decap_and_validate(skb, IPPROTO_IPIP)) > + goto drop; > + > + if (!pskb_may_pull(skb, sizeof(struct iphdr))) > + goto drop; > + > + vrf = end_dt4_get_vrf_rcu(skb, &slwt->dt4_info); > + if (unlikely(!vrf)) > + goto drop; > + > + skb->protocol = htons(ETH_P_IP); > + > + skb_dst_drop(skb); > + > + skb_set_transport_header(skb, sizeof(struct iphdr)); > + > + skb = end_dt4_vrf_rcv(skb, vrf); > + if (!skb) > + /* packet has been processed and consumed by the VRF */ > + return 0; > + > + if (IS_ERR(skb)) { > + err = PTR_ERR(skb); > + return err; return PTR_ERR(skb) > + } > + > + iph = ip_hdr(skb); > + > + err = ip_route_input(skb, iph->daddr, iph->saddr, 0, skb->dev); > + if (err) > + goto drop; > + > + return dst_input(skb); > + > +drop: > + kfree_skb(skb); > + return -EINVAL; > +} > + > +#else > + new line not needed > +static int seg6_end_dt4_build(struct seg6_local_lwt *slwt, const void *cfg, > + struct netlink_ext_ack *extack) > +{ > + NL_SET_ERR_MSG(extack, "Operation is not supported"); This extack message probably could be more helpful. As it stands it's basically > + > + return -EOPNOTSUPP; > +} > + > +static int input_action_end_dt4(struct sk_buff *skb, > + struct seg6_local_lwt *slwt) Maybe just ifdef out the part of the action table instead of creating those stubs? > +{ > + kfree_skb(skb); > + return -EOPNOTSUPP; > +} > + > +#endif > + > static int input_action_end_dt6(struct sk_buff *skb, > struct seg6_local_lwt *slwt) > { > @@ -601,6 +798,14 @@ static struct seg6_action_desc seg6_action_table[] = { BTW any idea why the action table is not marked as const? Would you mind sending a patch to fix that? > .attrs = (1 << SEG6_LOCAL_NH4), > .input = input_action_end_dx4, > }, > + { > + .action = SEG6_LOCAL_ACTION_END_DT4, > + .attrs = (1 << SEG6_LOCAL_TABLE), > + .input = input_action_end_dt4, > + .slwt_ops = { > + .build_state = seg6_end_dt4_build, > + }, > + }, > { > .action = SEG6_LOCAL_ACTION_END_DT6, > .attrs = (1 << SEG6_LOCAL_TABLE),
Hi Jakub, many thanks for your review. Please see my responses inline: On Tue, 10 Nov 2020 15:12:55 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Sat, 7 Nov 2020 16:31:38 +0100 Andrea Mayer wrote: > > SRv6 End.DT4 is defined in the SRv6 Network Programming [1]. > > > > The SRv6 End.DT4 is used to implement IPv4 L3VPN use-cases in > > multi-tenants environments. It decapsulates the received packets and it > > performs IPv4 routing lookup in the routing table of the tenant. > > > > The SRv6 End.DT4 Linux implementation leverages a VRF device in order to > > force the routing lookup into the associated routing table. > > How does the behavior of DT4 compare to DT6? > The implementation of SRv6 End.DT4 differs from the the implementation of SRv6 End.DT6 due to the different *route input* lookup functions. For IPv6 is it possible to force the routing lookup specifying a routing table through the ip6_pol_route() function (as it is done in the seg6_lookup_any_nexthop()). Conversely, for the IPv4 we cannot force the lookup into a specific table with the functions that are currently exposed by the kernel. > The implementation looks quite different. > Long story short: A long time ago, we discussed here on the mailing list how best to implement the SRv6 DT4. After some time, we identified with the help of David Ahern the VRF as the key infrastructure on which to build the SRv6 End.DT4. Indeed, the use of VRF allows us not to touch in any way the core components of the kernel (i.e.: the ipv4 routing system) and to exploit an already existing infrastructure. I would say that also the SRv6 End.DT6 should leverage the VRF as we did for SRv6 End.DT4. We can also try to change End.DT6 implementation, if needed. > > To make the End.DT4 work properly, it must be guaranteed that the routing > > table used for routing lookup operations is bound to one and only one > > VRF during the tunnel creation. Such constraint has to be enforced by > > enabling the VRF strict_mode sysctl parameter, i.e: > > $ sysctl -wq net.vrf.strict_mode=1. > > > > At JANOG44, LINE corporation presented their multi-tenant DC architecture > > using SRv6 [2]. In the slides, they reported that the Linux kernel is > > missing the support of SRv6 End.DT4 behavior. > > > > The iproute2 counterpart required for configuring the SRv6 End.DT4 > > behavior is already implemented along with the other supported SRv6 > > behaviors [3]. > > > > [1] https://tools.ietf.org/html/draft-ietf-spring-srv6-network-programming > > [2] https://speakerdeck.com/line_developers/line-data-center-networking-with-srv6 > > [3] https://patchwork.ozlabs.org/patch/799837/ > > > > Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it> > > --- > > net/ipv6/seg6_local.c | 205 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 205 insertions(+) > > > > diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c > > index 4b0f155d641d..a41074acd43e 100644 > > --- a/net/ipv6/seg6_local.c > > +++ b/net/ipv6/seg6_local.c > > @@ -57,6 +57,14 @@ struct bpf_lwt_prog { > > char *name; > > }; > > > > +struct seg6_end_dt4_info { > > + struct net *net; > > + /* VRF device associated to the routing table used by the SRv6 End.DT4 > > + * behavior for routing IPv4 packets. > > + */ > > + int vrf_ifindex; > > +}; > > + > > struct seg6_local_lwt { > > int action; > > struct ipv6_sr_hdr *srh; > > @@ -66,6 +74,7 @@ struct seg6_local_lwt { > > int iif; > > int oif; > > struct bpf_lwt_prog bpf; > > + struct seg6_end_dt4_info dt4_info; > > > > int headroom; > > struct seg6_action_desc *desc; > > @@ -413,6 +422,194 @@ static int input_action_end_dx4(struct sk_buff *skb, > > return -EINVAL; > > } > > > > +#ifdef CONFIG_NET_L3_MASTER_DEV > > + > > no need for this empty line. > Ok. > > +static struct net *fib6_config_get_net(const struct fib6_config *fib6_cfg) > > +{ > > + const struct nl_info *nli = &fib6_cfg->fc_nlinfo; > > + > > + return nli->nl_net; > > +} > > + > > +static int seg6_end_dt4_build(struct seg6_local_lwt *slwt, const void *cfg, > > + struct netlink_ext_ack *extack) > > +{ > > + struct seg6_end_dt4_info *info = &slwt->dt4_info; > > + int vrf_ifindex; > > + struct net *net; > > + > > + net = fib6_config_get_net(cfg); > > + > > + vrf_ifindex = l3mdev_ifindex_lookup_by_table_id(L3MDEV_TYPE_VRF, net, > > + slwt->table); > > + if (vrf_ifindex < 0) { > > + if (vrf_ifindex == -EPERM) { > > + NL_SET_ERR_MSG(extack, > > + "Strict mode for VRF is disabled"); > > + } else if (vrf_ifindex == -ENODEV) { > > + NL_SET_ERR_MSG(extack, "No such device"); > > That's what -ENODEV already says. > Yes, sorry for this very trivial message. I will improve it in v3. > > + } else { > > + NL_SET_ERR_MSG(extack, "Unknown error"); > > Useless error. > Ok, I will remove it and keep only the pr_debug message in v3. > > + pr_debug("seg6local: SRv6 End.DT4 creation error=%d\n", > > + vrf_ifindex); > > + } > > + > > + return vrf_ifindex; > > + } > > + > > + info->net = net; > > + info->vrf_ifindex = vrf_ifindex; > > + > > + return 0; > > +} > > + > > +/* The SRv6 End.DT4 behavior extracts the inner (IPv4) packet and routes the > > + * IPv4 packet by looking at the configured routing table. > > + * > > + * In the SRv6 End.DT4 use case, we can receive traffic (IPv6+Segment Routing > > + * Header packets) from several interfaces and the IPv6 destination address (DA) > > + * is used for retrieving the specific instance of the End.DT4 behavior that > > + * should process the packets. > > + * > > + * However, the inner IPv4 packet is not really bound to any receiving > > + * interface and thus the End.DT4 sets the VRF (associated with the > > + * corresponding routing table) as the *receiving* interface. > > + * In other words, the End.DT4 processes a packet as if it has been received > > + * directly by the VRF (and not by one of its slave devices, if any). > > + * In this way, the VRF interface is used for routing the IPv4 packet in > > + * according to the routing table configured by the End.DT4 instance. > > + * > > + * This design allows you to get some interesting features like: > > + * 1) the statistics on rx packets; > > + * 2) the possibility to install a packet sniffer on the receiving interface > > + * (the VRF one) for looking at the incoming packets; > > + * 3) the possibility to leverage the netfilter prerouting hook for the inner > > + * IPv4 packet. > > + * > > + * This function returns: > > + * - the sk_buff* when the VRF rcv handler has processed the packet correctly; > > + * - NULL when the skb is consumed by the VRF rcv handler; > > + * - a pointer which encodes a negative error number in case of error. > > + * Note that in this case, the function takes care of freeing the skb. > > + */ > > +static struct sk_buff *end_dt4_vrf_rcv(struct sk_buff *skb, > > + struct net_device *dev) > > +{ > > + /* based on l3mdev_ip_rcv; we are only interested in the master */ > > + if (unlikely(!netif_is_l3_master(dev) && !netif_has_l3_rx_handler(dev))) > > + goto drop; > > + > > + if (unlikely(!dev->l3mdev_ops->l3mdev_l3_rcv)) > > + goto drop; > > + > > + /* the decap packet (IPv4) does not come with any mac header info. > > + * We must unset the mac header to allow the VRF device to rebuild it, > > + * just in case there is a sniffer attached on the device. > > + */ > > + skb_unset_mac_header(skb); > > + > > + skb = dev->l3mdev_ops->l3mdev_l3_rcv(dev, skb, AF_INET); > > + if (!skb) > > + /* the skb buffer was consumed by the handler */ > > + return NULL; > > + > > + /* when a packet is received by a VRF or by one of its slaves, the > > + * master device reference is set into the skb. > > + */ > > + if (unlikely(skb->dev != dev || skb->skb_iif != dev->ifindex)) > > + goto drop; > > + > > + return skb; > > + > > +drop: > > + kfree_skb(skb); > > + return ERR_PTR(-EINVAL); > > +} > > + > > +static struct net_device *end_dt4_get_vrf_rcu(struct sk_buff *skb, > > + struct seg6_end_dt4_info *info) > > +{ > > + int vrf_ifindex = info->vrf_ifindex; > > + struct net *net = info->net; > > + > > + if (unlikely(vrf_ifindex < 0)) > > + goto error; > > + > > + if (unlikely(!net_eq(dev_net(skb->dev), net))) > > + goto error; > > + > > + return dev_get_by_index_rcu(net, vrf_ifindex); > > + > > +error: > > + return NULL; > > +} > > + > > +static int input_action_end_dt4(struct sk_buff *skb, > > + struct seg6_local_lwt *slwt) > > +{ > > + struct net_device *vrf; > > + struct iphdr *iph; > > + int err; > > + > > + if (!decap_and_validate(skb, IPPROTO_IPIP)) > > + goto drop; > > + > > + if (!pskb_may_pull(skb, sizeof(struct iphdr))) > > + goto drop; > > + > > + vrf = end_dt4_get_vrf_rcu(skb, &slwt->dt4_info); > > + if (unlikely(!vrf)) > > + goto drop; > > + > > + skb->protocol = htons(ETH_P_IP); > > + > > + skb_dst_drop(skb); > > + > > + skb_set_transport_header(skb, sizeof(struct iphdr)); > > + > > + skb = end_dt4_vrf_rcv(skb, vrf); > > + if (!skb) > > + /* packet has been processed and consumed by the VRF */ > > + return 0; > > + > > + if (IS_ERR(skb)) { > > + err = PTR_ERR(skb); > > + return err; > > return PTR_ERR(skb) > I will fix it in v3. > > + } > > + > > + iph = ip_hdr(skb); > > + > > + err = ip_route_input(skb, iph->daddr, iph->saddr, 0, skb->dev); > > + if (err) > > + goto drop; > > + > > + return dst_input(skb); > > + > > +drop: > > + kfree_skb(skb); > > + return -EINVAL; > > +} > > + > > +#else > > + > > new line not needed > Ok. > > +static int seg6_end_dt4_build(struct seg6_local_lwt *slwt, const void *cfg, > > + struct netlink_ext_ack *extack) > > +{ > > + NL_SET_ERR_MSG(extack, "Operation is not supported"); > > This extack message probably could be more helpful. As it stands it's > basically > Please, see just right below. > > + > > + return -EOPNOTSUPP; > > +} > > + > > +static int input_action_end_dt4(struct sk_buff *skb, > > + struct seg6_local_lwt *slwt) > > Maybe just ifdef out the part of the action table instead of creating > those stubs? > This is a very interesting point and I like your idea. We can eliminate the two stubs while keeping the "unsupported operation" semantics in this way: static struct seg6_action_desc seg6_action_table[] = { [...] { .action = SEG6_LOCAL_ACTION_END_DT4, .attrs = (1 << SEG6_LOCAL_TABLE), #ifdef CONFIG_NET_L3_MASTER_DEV .input = input_action_end_dt4, .slwt_ops = { .build_state = seg6_end_dt4_build, }, #endif }, [...] } when the CONFIG_NET_L3_MASTER_DEV is not defined, the behavior can not be instantiated because the "input" callback is initialized to NULL. This fact forces the parse_nla_action() to fail returning -EOPNOTSUPP to the user (that is exactly what we want to achieve). Note that surrounding the entire DT4 action table entry with #ifdef/#endif does not allow us to distinguish whether the DT4 was really implemented or it was not supported due to the way in which the CONFIG_NET_L3_MASTER_DEV was set. In both cases, when the user tries to instantiate a new DT4 behavior, the kernel replies back with the -EINVAL error. > > +{ > > + kfree_skb(skb); > > + return -EOPNOTSUPP; > > +} > > + > > +#endif > > + > > static int input_action_end_dt6(struct sk_buff *skb, > > struct seg6_local_lwt *slwt) > > { > > @@ -601,6 +798,14 @@ static struct seg6_action_desc seg6_action_table[] = { > > BTW any idea why the action table is not marked as const? > Frankly speaking, I have no idea. I have been working on the seg6 infrastructure for some time now, and I have never seen a single value changed in seg6_action_table[] after its initialization (neither the necessity to carry out an update operation). > Would you mind sending a patch to fix that? > Yes, I will send a fix for this issue adding the 'const' keyword. > > .attrs = (1 << SEG6_LOCAL_NH4), > > .input = input_action_end_dx4, > > }, > > + { > > + .action = SEG6_LOCAL_ACTION_END_DT4, > > + .attrs = (1 << SEG6_LOCAL_TABLE), > > + .input = input_action_end_dt4, > > + .slwt_ops = { > > + .build_state = seg6_end_dt4_build, > > + }, > > + }, > > { > > .action = SEG6_LOCAL_ACTION_END_DT6, > > .attrs = (1 << SEG6_LOCAL_TABLE), > Thank you, Andrea
On 11/12/20 6:28 PM, Andrea Mayer wrote: > The implementation of SRv6 End.DT4 differs from the the implementation of SRv6 > End.DT6 due to the different *route input* lookup functions. For IPv6 is it > possible to force the routing lookup specifying a routing table through the > ip6_pol_route() function (as it is done in the seg6_lookup_any_nexthop()). It is unfortunate that the IPv6 variant got in without the VRF piece.
Hi Andrea,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on ipvs/master]
[also build test ERROR on linus/master sparc-next/master v5.10-rc3 next-20201112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Andrea-Mayer/seg6-add-support-for-the-SRv6-End-DT4-behavior/20201109-093019
base: https://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git master
config: x86_64-randconfig-a005-20201111 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 874b0a0b9db93f5d3350ffe6b5efda2d908415d0)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/761138e2f757ac64efe97b03311c976db242dc92
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Andrea-Mayer/seg6-add-support-for-the-SRv6-End-DT4-behavior/20201109-093019
git checkout 761138e2f757ac64efe97b03311c976db242dc92
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> net/ipv6/seg6_local.c:793:4: error: field designator 'slwt_ops' does not refer to any field in type 'struct seg6_action_desc'
.slwt_ops = {
^
>> net/ipv6/seg6_local.c:826:10: error: invalid application of 'sizeof' to an incomplete type 'struct seg6_action_desc []'
count = ARRAY_SIZE(seg6_action_table);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/kernel.h:48:32: note: expanded from macro 'ARRAY_SIZE'
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
^~~~~
2 errors generated.
vim +793 net/ipv6/seg6_local.c
757
758 static struct seg6_action_desc seg6_action_table[] = {
759 {
760 .action = SEG6_LOCAL_ACTION_END,
761 .attrs = 0,
762 .input = input_action_end,
763 },
764 {
765 .action = SEG6_LOCAL_ACTION_END_X,
766 .attrs = (1 << SEG6_LOCAL_NH6),
767 .input = input_action_end_x,
768 },
769 {
770 .action = SEG6_LOCAL_ACTION_END_T,
771 .attrs = (1 << SEG6_LOCAL_TABLE),
772 .input = input_action_end_t,
773 },
774 {
775 .action = SEG6_LOCAL_ACTION_END_DX2,
776 .attrs = (1 << SEG6_LOCAL_OIF),
777 .input = input_action_end_dx2,
778 },
779 {
780 .action = SEG6_LOCAL_ACTION_END_DX6,
781 .attrs = (1 << SEG6_LOCAL_NH6),
782 .input = input_action_end_dx6,
783 },
784 {
785 .action = SEG6_LOCAL_ACTION_END_DX4,
786 .attrs = (1 << SEG6_LOCAL_NH4),
787 .input = input_action_end_dx4,
788 },
789 {
790 .action = SEG6_LOCAL_ACTION_END_DT4,
791 .attrs = (1 << SEG6_LOCAL_TABLE),
792 .input = input_action_end_dt4,
> 793 .slwt_ops = {
794 .build_state = seg6_end_dt4_build,
795 },
796 },
797 {
798 .action = SEG6_LOCAL_ACTION_END_DT6,
799 .attrs = (1 << SEG6_LOCAL_TABLE),
800 .input = input_action_end_dt6,
801 },
802 {
803 .action = SEG6_LOCAL_ACTION_END_B6,
804 .attrs = (1 << SEG6_LOCAL_SRH),
805 .input = input_action_end_b6,
806 },
807 {
808 .action = SEG6_LOCAL_ACTION_END_B6_ENCAP,
809 .attrs = (1 << SEG6_LOCAL_SRH),
810 .input = input_action_end_b6_encap,
811 .static_headroom = sizeof(struct ipv6hdr),
812 },
813 {
814 .action = SEG6_LOCAL_ACTION_END_BPF,
815 .attrs = (1 << SEG6_LOCAL_BPF),
816 .input = input_action_end_bpf,
817 },
818
819 };
820
821 static struct seg6_action_desc *__get_action_desc(int action)
822 {
823 struct seg6_action_desc *desc;
824 int i, count;
825
> 826 count = ARRAY_SIZE(seg6_action_table);
827 for (i = 0; i < count; i++) {
828 desc = &seg6_action_table[i];
829 if (desc->action == action)
830 return desc;
831 }
832
833 return NULL;
834 }
835
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu, 12 Nov 2020 18:49:17 -0700 David Ahern wrote: > On 11/12/20 6:28 PM, Andrea Mayer wrote: > > The implementation of SRv6 End.DT4 differs from the the implementation of SRv6 > > End.DT6 due to the different *route input* lookup functions. For IPv6 is it > > possible to force the routing lookup specifying a routing table through the > > ip6_pol_route() function (as it is done in the seg6_lookup_any_nexthop()). > > It is unfortunate that the IPv6 variant got in without the VRF piece. Should we make it a requirement for this series to also extend the v6 version to support the preferred VRF-based operation? Given VRF is better and we require v4 features to be implemented for v6?
Good people of build bot, would you mind shedding some light on this one? It was also reported on v1, and Andrea said it's impossible to repro. Strange that build bot would make the same mistake twice, tho. Thanks! On Fri, 13 Nov 2020 17:23:09 +0800 kernel test robot wrote: > Hi Andrea, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on ipvs/master] > [also build test ERROR on linus/master sparc-next/master v5.10-rc3 next-20201112] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Andrea-Mayer/seg6-add-support-for-the-SRv6-End-DT4-behavior/20201109-093019 > base: https://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git master > config: x86_64-randconfig-a005-20201111 (attached as .config) > compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 874b0a0b9db93f5d3350ffe6b5efda2d908415d0) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # install x86_64 cross compiling tool for clang build > # apt-get install binutils-x86-64-linux-gnu > # https://github.com/0day-ci/linux/commit/761138e2f757ac64efe97b03311c976db242dc92 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Andrea-Mayer/seg6-add-support-for-the-SRv6-End-DT4-behavior/20201109-093019 > git checkout 761138e2f757ac64efe97b03311c976db242dc92 > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > >> net/ipv6/seg6_local.c:793:4: error: field designator 'slwt_ops' does not refer to any field in type 'struct seg6_action_desc' > .slwt_ops = { > ^ > >> net/ipv6/seg6_local.c:826:10: error: invalid application of 'sizeof' to an incomplete type 'struct seg6_action_desc []' > count = ARRAY_SIZE(seg6_action_table); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/kernel.h:48:32: note: expanded from macro 'ARRAY_SIZE' > #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) > ^~~~~ > 2 errors generated. > > vim +793 net/ipv6/seg6_local.c > > 757 > 758 static struct seg6_action_desc seg6_action_table[] = { > 759 { > 760 .action = SEG6_LOCAL_ACTION_END, > 761 .attrs = 0, > 762 .input = input_action_end, > 763 }, > 764 { > 765 .action = SEG6_LOCAL_ACTION_END_X, > 766 .attrs = (1 << SEG6_LOCAL_NH6), > 767 .input = input_action_end_x, > 768 }, > 769 { > 770 .action = SEG6_LOCAL_ACTION_END_T, > 771 .attrs = (1 << SEG6_LOCAL_TABLE), > 772 .input = input_action_end_t, > 773 }, > 774 { > 775 .action = SEG6_LOCAL_ACTION_END_DX2, > 776 .attrs = (1 << SEG6_LOCAL_OIF), > 777 .input = input_action_end_dx2, > 778 }, > 779 { > 780 .action = SEG6_LOCAL_ACTION_END_DX6, > 781 .attrs = (1 << SEG6_LOCAL_NH6), > 782 .input = input_action_end_dx6, > 783 }, > 784 { > 785 .action = SEG6_LOCAL_ACTION_END_DX4, > 786 .attrs = (1 << SEG6_LOCAL_NH4), > 787 .input = input_action_end_dx4, > 788 }, > 789 { > 790 .action = SEG6_LOCAL_ACTION_END_DT4, > 791 .attrs = (1 << SEG6_LOCAL_TABLE), > 792 .input = input_action_end_dt4, > > 793 .slwt_ops = { > 794 .build_state = seg6_end_dt4_build, > 795 }, > 796 }, > 797 { > 798 .action = SEG6_LOCAL_ACTION_END_DT6, > 799 .attrs = (1 << SEG6_LOCAL_TABLE), > 800 .input = input_action_end_dt6, > 801 }, > 802 { > 803 .action = SEG6_LOCAL_ACTION_END_B6, > 804 .attrs = (1 << SEG6_LOCAL_SRH), > 805 .input = input_action_end_b6, > 806 }, > 807 { > 808 .action = SEG6_LOCAL_ACTION_END_B6_ENCAP, > 809 .attrs = (1 << SEG6_LOCAL_SRH), > 810 .input = input_action_end_b6_encap, > 811 .static_headroom = sizeof(struct ipv6hdr), > 812 }, > 813 { > 814 .action = SEG6_LOCAL_ACTION_END_BPF, > 815 .attrs = (1 << SEG6_LOCAL_BPF), > 816 .input = input_action_end_bpf, > 817 }, > 818 > 819 }; > 820 > 821 static struct seg6_action_desc *__get_action_desc(int action) > 822 { > 823 struct seg6_action_desc *desc; > 824 int i, count; > 825 > > 826 count = ARRAY_SIZE(seg6_action_table); > 827 for (i = 0; i < count; i++) { > 828 desc = &seg6_action_table[i]; > 829 if (desc->action == action) > 830 return desc; > 831 } > 832 > 833 return NULL; > 834 } > 835 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Il 2020-11-13 17:55, Jakub Kicinski ha scritto: > On Thu, 12 Nov 2020 18:49:17 -0700 David Ahern wrote: >> On 11/12/20 6:28 PM, Andrea Mayer wrote: >>> The implementation of SRv6 End.DT4 differs from the the implementation of SRv6 >>> End.DT6 due to the different *route input* lookup functions. For IPv6 is it >>> possible to force the routing lookup specifying a routing table through the >>> ip6_pol_route() function (as it is done in the seg6_lookup_any_nexthop()). >> >> It is unfortunate that the IPv6 variant got in without the VRF piece. > > Should we make it a requirement for this series to also extend the v6 > version to support the preferred VRF-based operation? Given VRF is > better and we require v4 features to be implemented for v6? I think it is better to separate the two aspects... adding a missing feature in IPv4 datapath should not depend on improving the quality of the implementation of the IPv6 datapath :-) I think that Andrea is willing to work on improving the IPv6 implementation, but this should be considered after this patchset... my 2c Stefano
On 11/13/20 10:02 AM, Stefano Salsano wrote: > Il 2020-11-13 17:55, Jakub Kicinski ha scritto: >> On Thu, 12 Nov 2020 18:49:17 -0700 David Ahern wrote: >>> On 11/12/20 6:28 PM, Andrea Mayer wrote: >>>> The implementation of SRv6 End.DT4 differs from the the >>>> implementation of SRv6 >>>> End.DT6 due to the different *route input* lookup functions. For >>>> IPv6 is it >>>> possible to force the routing lookup specifying a routing table >>>> through the >>>> ip6_pol_route() function (as it is done in the >>>> seg6_lookup_any_nexthop()). >>> >>> It is unfortunate that the IPv6 variant got in without the VRF piece. >> >> Should we make it a requirement for this series to also extend the v6 >> version to support the preferred VRF-based operation? Given VRF is >> better and we require v4 features to be implemented for v6? > > I think it is better to separate the two aspects... adding a missing > feature in IPv4 datapath should not depend on improving the quality of > the implementation of the IPv6 datapath :-) > > I think that Andrea is willing to work on improving the IPv6 > implementation, but this should be considered after this patchset... > agreed. The v6 variant has existed for a while. The v4 version is independent.
On 11/13/20 9:57 AM, Jakub Kicinski wrote: > Good people of build bot, > > would you mind shedding some light on this one? It was also reported on > v1, and Andrea said it's impossible to repro. Strange that build bot > would make the same mistake twice, tho. > I kicked off a build this morning using Andrea's patches and the config from the build bot; builds fine as long as the first 3 patches are applied.
On Fri, Nov 13, 2020 at 10:05:56AM -0700, David Ahern wrote: > On 11/13/20 9:57 AM, Jakub Kicinski wrote: > > Good people of build bot, > > > > would you mind shedding some light on this one? It was also reported on > > v1, and Andrea said it's impossible to repro. Strange that build bot > > would make the same mistake twice, tho. > > > > I kicked off a build this morning using Andrea's patches and the config > from the build bot; builds fine as long as the first 3 patches are applied. > I can confirm this as well with clang; if I applied the first three patches then this one, there is no error but if you just apply this one, there will be. If you open the GitHub URL, it shows just this patch applied, not the first three, which explains it. For what it's worth, b4 chokes over this series: $ b4 am -o - 20201107153139.3552-1-andrea.mayer@uniroma2.it | git am Looking up https://lore.kernel.org/r/20201107153139.3552-1-andrea.mayer%40uniroma2.it Grabbing thread from lore.kernel.org/linux-kselftest Analyzing 18 messages in the thread --- Writing /tmp/tmp8425by7fb4-am-stdout [net-next,v2,3/5] seg6: add callbacks for customizing the creation/destruction of a behavior --- Total patches: 1 --- Link: https://lore.kernel.org/r/20201107153139.3552-1-andrea.mayer@uniroma2.it Base: not found --- Applying: seg6: add callbacks for customizing the creation/destruction of a behavior error: patch failed: net/ipv6/seg6_local.c:1015 error: net/ipv6/seg6_local.c: patch does not apply Patch failed at 0001 seg6: add callbacks for customizing the creation/destruction of a behavior hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Even if I grab the mbox from lore.kernel.org, it tries to do the same thing and apply the 3rd patch first, which might explain why the 0day bot got confused. Cheers, Nathan
On Fri, 13 Nov 2020 10:04:44 -0700 David Ahern wrote: > On 11/13/20 10:02 AM, Stefano Salsano wrote: > > Il 2020-11-13 17:55, Jakub Kicinski ha scritto: > >> On Thu, 12 Nov 2020 18:49:17 -0700 David Ahern wrote: > >>> On 11/12/20 6:28 PM, Andrea Mayer wrote: > >>>> The implementation of SRv6 End.DT4 differs from the the > >>>> implementation of SRv6 > >>>> End.DT6 due to the different *route input* lookup functions. For > >>>> IPv6 is it > >>>> possible to force the routing lookup specifying a routing table > >>>> through the > >>>> ip6_pol_route() function (as it is done in the > >>>> seg6_lookup_any_nexthop()). > >>> > >>> It is unfortunate that the IPv6 variant got in without the VRF piece. > >> > >> Should we make it a requirement for this series to also extend the v6 > >> version to support the preferred VRF-based operation? Given VRF is > >> better and we require v4 features to be implemented for v6? > > > > I think it is better to separate the two aspects... adding a missing > > feature in IPv4 datapath should not depend on improving the quality of > > the implementation of the IPv6 datapath :-) > > > > I think that Andrea is willing to work on improving the IPv6 > > implementation, but this should be considered after this patchset... > > agreed. The v6 variant has existed for a while. The v4 version is > independent. Okay, I'm not sure what's the right call so I asked DaveM. TBH I wasn't expecting this reaction, we're talking about a 200 LoC patch which would probably be 90% reused for v6...
Il 2020-11-13 20:40, Jakub Kicinski ha scritto: > On Fri, 13 Nov 2020 10:04:44 -0700 David Ahern wrote: >> On 11/13/20 10:02 AM, Stefano Salsano wrote: >>> Il 2020-11-13 17:55, Jakub Kicinski ha scritto: >>>> On Thu, 12 Nov 2020 18:49:17 -0700 David Ahern wrote: >>>>> On 11/12/20 6:28 PM, Andrea Mayer wrote: >>>>>> The implementation of SRv6 End.DT4 differs from the the >>>>>> implementation of SRv6 >>>>>> End.DT6 due to the different *route input* lookup functions. For >>>>>> IPv6 is it >>>>>> possible to force the routing lookup specifying a routing table >>>>>> through the >>>>>> ip6_pol_route() function (as it is done in the >>>>>> seg6_lookup_any_nexthop()). >>>>> >>>>> It is unfortunate that the IPv6 variant got in without the VRF piece. >>>> >>>> Should we make it a requirement for this series to also extend the v6 >>>> version to support the preferred VRF-based operation? Given VRF is >>>> better and we require v4 features to be implemented for v6? >>> >>> I think it is better to separate the two aspects... adding a missing >>> feature in IPv4 datapath should not depend on improving the quality of >>> the implementation of the IPv6 datapath :-) >>> >>> I think that Andrea is willing to work on improving the IPv6 >>> implementation, but this should be considered after this patchset... >> >> agreed. The v6 variant has existed for a while. The v4 version is >> independent. > > Okay, I'm not sure what's the right call so I asked DaveM. > > TBH I wasn't expecting this reaction, we're talking about a 200 LoC > patch which would probably be 90% reused for v6... > Jakub, we've considered the possibility to extend the v6 version to support the preferred VRF-based operation as you suggested at first glance, it would break the uAPI compatibility with existing scripts that use SRv6 DT6, currently we configure the decap operation in this way ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 table 100 dev eth0 if the v6 version is extended to support the VRF-based operation, in order to configure the decap operation we have to do (like we do in the v4 version) ip link add vrf0 type vrf table 100 sysctl -w net.vrf.strict_mode=1 ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 table 100 dev eth0 (of course the sysctl is needed globally once... while the "ip link add..." command is needed once for every table X that is used in a script) considering how much we care of not breaking existing functionality... it is not clear IMO if we should go into this direction or we should think twice... and maybe look for another design to introduce VRFs into v6 so I would prefer finalizing the DT4 patchset and then start discussing the VRF support in v6 version
On Fri, 13 Nov 2020 11:40:36 -0800 Jakub Kicinski wrote: > > agreed. The v6 variant has existed for a while. The v4 version is > > independent. > > Okay, I'm not sure what's the right call so I asked DaveM. DaveM raised a concern that unless we implement v6 now we can't be sure the interface we create for v4 is going to fit there. So Andrea unless it's a major hurdle, could you take a stab at the v6 version with VRFs as part of this series?
Hi Jakub, On Fri, 13 Nov 2020 13:40:10 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Fri, 13 Nov 2020 11:40:36 -0800 Jakub Kicinski wrote: > > > agreed. The v6 variant has existed for a while. The v4 version is > > > independent. > > > > Okay, I'm not sure what's the right call so I asked DaveM. > > DaveM raised a concern that unless we implement v6 now we can't be sure > the interface we create for v4 is going to fit there. > > So Andrea unless it's a major hurdle, could you take a stab at the v6 > version with VRFs as part of this series? I can tackle the v6 version but how do we face the compatibility issue raised by Stefano in his message? if it is ok to implement a uAPI that breaks the existing scripts, it is relatively easy to replicate the VRF-based approach also in v6. Waiting for your advice! Thanks, Andrea
On Sat, 14 Nov 2020 00:00:24 +0100 Andrea Mayer wrote: > On Fri, 13 Nov 2020 13:40:10 -0800 > Jakub Kicinski <kuba@kernel.org> wrote: > > > On Fri, 13 Nov 2020 11:40:36 -0800 Jakub Kicinski wrote: > > > > agreed. The v6 variant has existed for a while. The v4 version is > > > > independent. > > > > > > Okay, I'm not sure what's the right call so I asked DaveM. > > > > DaveM raised a concern that unless we implement v6 now we can't be sure > > the interface we create for v4 is going to fit there. > > > > So Andrea unless it's a major hurdle, could you take a stab at the v6 > > version with VRFs as part of this series? > > I can tackle the v6 version but how do we face the compatibility issue raised > by Stefano in his message? > > if it is ok to implement a uAPI that breaks the existing scripts, it is relatively > easy to replicate the VRF-based approach also in v6. We need to keep existing End.DT6 as is, and add a separate implementation. The way to distinguish between the two could be either by passing via netlink a flag attribute (which would request use of VRF in both cases); using a different attribute than SEG6_LOCAL_TABLE for the table id (or perhaps passing VRF's ifindex instead), e.g. SEG6_LOCAL_TABLE_VRF; or adding a new command (SEG6_LOCAL_ACTION_END_DT6_VRF) which would behave like End.DT4.
Hi Jakub, Please see my responses inline: On Fri, 13 Nov 2020 15:54:37 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Sat, 14 Nov 2020 00:00:24 +0100 Andrea Mayer wrote: > > On Fri, 13 Nov 2020 13:40:10 -0800 > > Jakub Kicinski <kuba@kernel.org> wrote: > > > > I can tackle the v6 version but how do we face the compatibility issue raised > > by Stefano in his message? > > > > if it is ok to implement a uAPI that breaks the existing scripts, it is relatively > > easy to replicate the VRF-based approach also in v6. > > We need to keep existing End.DT6 as is, and add a separate > implementation. ok > > The way to distinguish between the two could be either by > 1) passing via > netlink a flag attribute (which would request use of VRF in both > cases); yes, feasible... see UAPI solution 1 > 2) using a different attribute than SEG6_LOCAL_TABLE for the > table id (or perhaps passing VRF's ifindex instead), e.g. > SEG6_LOCAL_TABLE_VRF; yes, feasible... see UAPI solution 2 > 3) or adding a new command > (SEG6_LOCAL_ACTION_END_DT6_VRF) which would behave like End.DT4. no, we prefer not to add a new command, because it is better to keep a semantic one-to-one relationship between these commands and the SRv6 behaviors defined in the draft. UAPI solution 1 we add a new parameter "vrfmode". DT4 can only be used with the vrfmode parameter (hence it is a required parameter for DT4). DT6 can be used with "vrfmode" (new vrf based mode) or without "vrfmode" (legacy mode)(hence "vrfmode" is an optional parameter for DT6) UAPI solution 1 examples: ip -6 route add 2001:db8::1/128 encap seg6local action End.DT4 vrfmode table 100 dev eth0 ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 vrfmode table 100 dev eth0 ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 table 100 dev eth0 UAPI solution 2 we turn "table" into an optional parameter and we add the "vrftable" optional parameter. DT4 can only be used with the "vrftable" (hence it is a required parameter for DT4). DT6 can be used with "vrftable" (new vrf mode) or with "table" (legacy mode) (hence it is an optional parameter for DT6). UAPI solution 2 examples: ip -6 route add 2001:db8::1/128 encap seg6local action End.DT4 vrftable 100 dev eth0 ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 vrftable 100 dev eth0 ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 table 100 dev eth0 IMO solution 2 is nicer from UAPI POV because we always have only one parameter, maybe solution 1 is slightly easier to implement, all in all we prefer solution 2 but we can go for 1 if you prefer. Waiting for your advice! Thanks, Andrea
On Sat, 14 Nov 2020 02:50:58 +0100 Andrea Mayer wrote: > Hi Jakub, > Please see my responses inline: > > On Fri, 13 Nov 2020 15:54:37 -0800 > Jakub Kicinski <kuba@kernel.org> wrote: > > > On Sat, 14 Nov 2020 00:00:24 +0100 Andrea Mayer wrote: > > > On Fri, 13 Nov 2020 13:40:10 -0800 > > > Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > I can tackle the v6 version but how do we face the compatibility issue raised > > > by Stefano in his message? > > > > > > if it is ok to implement a uAPI that breaks the existing scripts, it is relatively > > > easy to replicate the VRF-based approach also in v6. > > > > We need to keep existing End.DT6 as is, and add a separate > > implementation. > > ok > > > > > The way to distinguish between the two could be either by > > > 1) passing via > > netlink a flag attribute (which would request use of VRF in both > > cases); > > yes, feasible... see UAPI solution 1 > > > 2) using a different attribute than SEG6_LOCAL_TABLE for the > > table id (or perhaps passing VRF's ifindex instead), e.g. > > SEG6_LOCAL_TABLE_VRF; > > yes, feasible... see UAPI solution 2 > > > 3) or adding a new command > > (SEG6_LOCAL_ACTION_END_DT6_VRF) which would behave like End.DT4. > > no, we prefer not to add a new command, because it is better to keep a > semantic one-to-one relationship between these commands and the SRv6 > behaviors defined in the draft. > > > UAPI solution 1 > > we add a new parameter "vrfmode". DT4 can only be used with the > vrfmode parameter (hence it is a required parameter for DT4). > DT6 can be used with "vrfmode" (new vrf based mode) or without "vrfmode" > (legacy mode)(hence "vrfmode" is an optional parameter for DT6) > > UAPI solution 1 examples: > > ip -6 route add 2001:db8::1/128 encap seg6local action End.DT4 vrfmode table 100 dev eth0 > ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 vrfmode table 100 dev eth0 > ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 table 100 dev eth0 > > UAPI solution 2 > > we turn "table" into an optional parameter and we add the "vrftable" optional > parameter. DT4 can only be used with the "vrftable" (hence it is a required > parameter for DT4). > DT6 can be used with "vrftable" (new vrf mode) or with "table" (legacy mode) > (hence it is an optional parameter for DT6). > > UAPI solution 2 examples: > > ip -6 route add 2001:db8::1/128 encap seg6local action End.DT4 vrftable 100 dev eth0 > ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 vrftable 100 dev eth0 > ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 table 100 dev eth0 > > IMO solution 2 is nicer from UAPI POV because we always have only one > parameter, maybe solution 1 is slightly easier to implement, all in all > we prefer solution 2 but we can go for 1 if you prefer. Agreed, 2 looks better to me as well. But let's not conflate uABI with iproute2's command line. I'm more concerned about the kernel ABI. BTW you prefer to operate on tables (and therefore require net.vrf.strict_mode=1) because that's closer to the spirit of the RFC, correct? As I said from the implementation perspective passing any VRF ifindex down from user space to the kernel should be fine?
Hi Jakub, On Fri, 13 Nov 2020 18:01:26 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > > UAPI solution 2 > > > > we turn "table" into an optional parameter and we add the "vrftable" optional > > parameter. DT4 can only be used with the "vrftable" (hence it is a required > > parameter for DT4). > > DT6 can be used with "vrftable" (new vrf mode) or with "table" (legacy mode) > > (hence it is an optional parameter for DT6). > > > > UAPI solution 2 examples: > > > > ip -6 route add 2001:db8::1/128 encap seg6local action End.DT4 vrftable 100 dev eth0 > > ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 vrftable 100 dev eth0 > > ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 table 100 dev eth0 > > > > IMO solution 2 is nicer from UAPI POV because we always have only one > > parameter, maybe solution 1 is slightly easier to implement, all in all > > we prefer solution 2 but we can go for 1 if you prefer. > > Agreed, 2 looks better to me as well. But let's not conflate uABI with > iproute2's command line. I'm more concerned about the kernel ABI. Sorry I was a little imprecise here. I reported only the user command perspective. From the kernel point of view in solution 2 the vrftable will be a new [SEG6_LOCAL_VRFTABLE] optional parameter. > BTW you prefer to operate on tables (and therefore require > net.vrf.strict_mode=1) because that's closer to the spirit of the RFC, > correct? As I said from the implementation perspective passing any VRF > ifindex down from user space to the kernel should be fine? Yes, I definitely prefer to operate on tables (and so on the table ID) due to the spirit of the RFC. We have discussed in depth this design choice with David Ahern when implementing the DT4 patch and we are confident that operating with VRF strict mode is a sound approach also for DT6. Thanks Andrea,
On 11/13/20 7:29 PM, Andrea Mayer wrote: > Hi Jakub, > > On Fri, 13 Nov 2020 18:01:26 -0800 > Jakub Kicinski <kuba@kernel.org> wrote: > >>> UAPI solution 2 >>> >>> we turn "table" into an optional parameter and we add the "vrftable" optional >>> parameter. DT4 can only be used with the "vrftable" (hence it is a required >>> parameter for DT4). >>> DT6 can be used with "vrftable" (new vrf mode) or with "table" (legacy mode) >>> (hence it is an optional parameter for DT6). >>> >>> UAPI solution 2 examples: >>> >>> ip -6 route add 2001:db8::1/128 encap seg6local action End.DT4 vrftable 100 dev eth0 >>> ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 vrftable 100 dev eth0 >>> ip -6 route add 2001:db8::1/128 encap seg6local action End.DT6 table 100 dev eth0 >>> >>> IMO solution 2 is nicer from UAPI POV because we always have only one >>> parameter, maybe solution 1 is slightly easier to implement, all in all >>> we prefer solution 2 but we can go for 1 if you prefer. >> >> Agreed, 2 looks better to me as well. But let's not conflate uABI with >> iproute2's command line. I'm more concerned about the kernel ABI. > > Sorry I was a little imprecise here. I reported only the user command perspective. > From the kernel point of view in solution 2 the vrftable will be a new > [SEG6_LOCAL_VRFTABLE] optional parameter. > >> BTW you prefer to operate on tables (and therefore require >> net.vrf.strict_mode=1) because that's closer to the spirit of the RFC, >> correct? As I said from the implementation perspective passing any VRF >> ifindex down from user space to the kernel should be fine? > > Yes, I definitely prefer to operate on tables (and so on the table ID) due to > the spirit of the RFC. We have discussed in depth this design choice with > David Ahern when implementing the DT4 patch and we are confident that operating > with VRF strict mode is a sound approach also for DT6. > I like the vrftable option. Straightforward extension from current table argument.
On 11/13/20 12:00 PM, Nathan Chancellor wrote: > On Fri, Nov 13, 2020 at 10:05:56AM -0700, David Ahern wrote: >> On 11/13/20 9:57 AM, Jakub Kicinski wrote: >>> Good people of build bot, >>> >>> would you mind shedding some light on this one? It was also reported on >>> v1, and Andrea said it's impossible to repro. Strange that build bot >>> would make the same mistake twice, tho. >>> >> >> I kicked off a build this morning using Andrea's patches and the config >> from the build bot; builds fine as long as the first 3 patches are applied. >> > > I can confirm this as well with clang; if I applied the first three > patches then this one, there is no error but if you just apply this one, > there will be. If you open the GitHub URL, it shows just this patch > applied, not the first three, which explains it. > > For what it's worth, b4 chokes over this series: Thanks, Nathan. I'll forward to Konstantin. > > $ b4 am -o - 20201107153139.3552-1-andrea.mayer@uniroma2.it | git am > Looking up https://lore.kernel.org/r/20201107153139.3552-1-andrea.mayer%40uniroma2.it > Grabbing thread from lore.kernel.org/linux-kselftest > Analyzing 18 messages in the thread > --- > Writing /tmp/tmp8425by7fb4-am-stdout > [net-next,v2,3/5] seg6: add callbacks for customizing the creation/destruction of a behavior > --- > Total patches: 1 > --- > Link: https://lore.kernel.org/r/20201107153139.3552-1-andrea.mayer@uniroma2.it > Base: not found > --- > Applying: seg6: add callbacks for customizing the creation/destruction of a behavior > error: patch failed: net/ipv6/seg6_local.c:1015 > error: net/ipv6/seg6_local.c: patch does not apply > Patch failed at 0001 seg6: add callbacks for customizing the creation/destruction of a behavior > hint: Use 'git am --show-current-patch=diff' to see the failed patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > > Even if I grab the mbox from lore.kernel.org, it tries to do the same > thing and apply the 3rd patch first, which might explain why the 0day > bot got confused. > > Cheers, > Nathan >
Hi Jakub, Sorry for the inconvenience, we have optimized the build bot to avoid this situation. Best Regards, Rong Chen On 11/14/20 12:57 AM, Jakub Kicinski wrote: > Good people of build bot, > > would you mind shedding some light on this one? It was also reported on > v1, and Andrea said it's impossible to repro. Strange that build bot > would make the same mistake twice, tho. > > Thanks! > > On Fri, 13 Nov 2020 17:23:09 +0800 kernel test robot wrote: >> Hi Andrea, >> >> Thank you for the patch! Yet something to improve: >> >> [auto build test ERROR on ipvs/master] >> [also build test ERROR on linus/master sparc-next/master v5.10-rc3 next-20201112] >> [If your patch is applied to the wrong git tree, kindly drop us a note. >> And when submitting patch, we suggest to use '--base' as documented in >> https://git-scm.com/docs/git-format-patch] >> >> url: https://github.com/0day-ci/linux/commits/Andrea-Mayer/seg6-add-support-for-the-SRv6-End-DT4-behavior/20201109-093019 >> base: https://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git master >> config: x86_64-randconfig-a005-20201111 (attached as .config) >> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 874b0a0b9db93f5d3350ffe6b5efda2d908415d0) >> reproduce (this is a W=1 build): >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >> chmod +x ~/bin/make.cross >> # install x86_64 cross compiling tool for clang build >> # apt-get install binutils-x86-64-linux-gnu >> # https://github.com/0day-ci/linux/commit/761138e2f757ac64efe97b03311c976db242dc92 >> git remote add linux-review https://github.com/0day-ci/linux >> git fetch --no-tags linux-review Andrea-Mayer/seg6-add-support-for-the-SRv6-End-DT4-behavior/20201109-093019 >> git checkout 761138e2f757ac64efe97b03311c976db242dc92 >> # save the attached .config to linux build tree >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 >> >> If you fix the issue, kindly add following tag as appropriate >> Reported-by: kernel test robot <lkp@intel.com> >> >> All errors (new ones prefixed by >>): >> >>>> net/ipv6/seg6_local.c:793:4: error: field designator 'slwt_ops' does not refer to any field in type 'struct seg6_action_desc' >> .slwt_ops = { >> ^ >>>> net/ipv6/seg6_local.c:826:10: error: invalid application of 'sizeof' to an incomplete type 'struct seg6_action_desc []' >> count = ARRAY_SIZE(seg6_action_table); >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> include/linux/kernel.h:48:32: note: expanded from macro 'ARRAY_SIZE' >> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) >> ^~~~~ >> 2 errors generated. >> >> vim +793 net/ipv6/seg6_local.c >> >> 757 >> 758 static struct seg6_action_desc seg6_action_table[] = { >> 759 { >> 760 .action = SEG6_LOCAL_ACTION_END, >> 761 .attrs = 0, >> 762 .input = input_action_end, >> 763 }, >> 764 { >> 765 .action = SEG6_LOCAL_ACTION_END_X, >> 766 .attrs = (1 << SEG6_LOCAL_NH6), >> 767 .input = input_action_end_x, >> 768 }, >> 769 { >> 770 .action = SEG6_LOCAL_ACTION_END_T, >> 771 .attrs = (1 << SEG6_LOCAL_TABLE), >> 772 .input = input_action_end_t, >> 773 }, >> 774 { >> 775 .action = SEG6_LOCAL_ACTION_END_DX2, >> 776 .attrs = (1 << SEG6_LOCAL_OIF), >> 777 .input = input_action_end_dx2, >> 778 }, >> 779 { >> 780 .action = SEG6_LOCAL_ACTION_END_DX6, >> 781 .attrs = (1 << SEG6_LOCAL_NH6), >> 782 .input = input_action_end_dx6, >> 783 }, >> 784 { >> 785 .action = SEG6_LOCAL_ACTION_END_DX4, >> 786 .attrs = (1 << SEG6_LOCAL_NH4), >> 787 .input = input_action_end_dx4, >> 788 }, >> 789 { >> 790 .action = SEG6_LOCAL_ACTION_END_DT4, >> 791 .attrs = (1 << SEG6_LOCAL_TABLE), >> 792 .input = input_action_end_dt4, >> > 793 .slwt_ops = { >> 794 .build_state = seg6_end_dt4_build, >> 795 }, >> 796 }, >> 797 { >> 798 .action = SEG6_LOCAL_ACTION_END_DT6, >> 799 .attrs = (1 << SEG6_LOCAL_TABLE), >> 800 .input = input_action_end_dt6, >> 801 }, >> 802 { >> 803 .action = SEG6_LOCAL_ACTION_END_B6, >> 804 .attrs = (1 << SEG6_LOCAL_SRH), >> 805 .input = input_action_end_b6, >> 806 }, >> 807 { >> 808 .action = SEG6_LOCAL_ACTION_END_B6_ENCAP, >> 809 .attrs = (1 << SEG6_LOCAL_SRH), >> 810 .input = input_action_end_b6_encap, >> 811 .static_headroom = sizeof(struct ipv6hdr), >> 812 }, >> 813 { >> 814 .action = SEG6_LOCAL_ACTION_END_BPF, >> 815 .attrs = (1 << SEG6_LOCAL_BPF), >> 816 .input = input_action_end_bpf, >> 817 }, >> 818 >> 819 }; >> 820 >> 821 static struct seg6_action_desc *__get_action_desc(int action) >> 822 { >> 823 struct seg6_action_desc *desc; >> 824 int i, count; >> 825 >> > 826 count = ARRAY_SIZE(seg6_action_table); >> 827 for (i = 0; i < count; i++) { >> 828 desc = &seg6_action_table[i]; >> 829 if (desc->action == action) >> 830 return desc; >> 831 } >> 832 >> 833 return NULL; >> 834 } >> 835 >> >> --- >> 0-DAY CI Kernel Test Service, Intel Corporation >> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org > _______________________________________________ > kbuild-all mailing list -- kbuild-all@lists.01.org > To unsubscribe send an email to kbuild-all-leave@lists.01.org
On Mon, 23 Nov 2020 09:13:49 +0800 Rong Chen wrote: > Sorry for the inconvenience, we have optimized the build bot to avoid > this situation. Great to hear, thank you!
diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c index 4b0f155d641d..a41074acd43e 100644 --- a/net/ipv6/seg6_local.c +++ b/net/ipv6/seg6_local.c @@ -57,6 +57,14 @@ struct bpf_lwt_prog { char *name; }; +struct seg6_end_dt4_info { + struct net *net; + /* VRF device associated to the routing table used by the SRv6 End.DT4 + * behavior for routing IPv4 packets. + */ + int vrf_ifindex; +}; + struct seg6_local_lwt { int action; struct ipv6_sr_hdr *srh; @@ -66,6 +74,7 @@ struct seg6_local_lwt { int iif; int oif; struct bpf_lwt_prog bpf; + struct seg6_end_dt4_info dt4_info; int headroom; struct seg6_action_desc *desc; @@ -413,6 +422,194 @@ static int input_action_end_dx4(struct sk_buff *skb, return -EINVAL; } +#ifdef CONFIG_NET_L3_MASTER_DEV + +static struct net *fib6_config_get_net(const struct fib6_config *fib6_cfg) +{ + const struct nl_info *nli = &fib6_cfg->fc_nlinfo; + + return nli->nl_net; +} + +static int seg6_end_dt4_build(struct seg6_local_lwt *slwt, const void *cfg, + struct netlink_ext_ack *extack) +{ + struct seg6_end_dt4_info *info = &slwt->dt4_info; + int vrf_ifindex; + struct net *net; + + net = fib6_config_get_net(cfg); + + vrf_ifindex = l3mdev_ifindex_lookup_by_table_id(L3MDEV_TYPE_VRF, net, + slwt->table); + if (vrf_ifindex < 0) { + if (vrf_ifindex == -EPERM) { + NL_SET_ERR_MSG(extack, + "Strict mode for VRF is disabled"); + } else if (vrf_ifindex == -ENODEV) { + NL_SET_ERR_MSG(extack, "No such device"); + } else { + NL_SET_ERR_MSG(extack, "Unknown error"); + + pr_debug("seg6local: SRv6 End.DT4 creation error=%d\n", + vrf_ifindex); + } + + return vrf_ifindex; + } + + info->net = net; + info->vrf_ifindex = vrf_ifindex; + + return 0; +} + +/* The SRv6 End.DT4 behavior extracts the inner (IPv4) packet and routes the + * IPv4 packet by looking at the configured routing table. + * + * In the SRv6 End.DT4 use case, we can receive traffic (IPv6+Segment Routing + * Header packets) from several interfaces and the IPv6 destination address (DA) + * is used for retrieving the specific instance of the End.DT4 behavior that + * should process the packets. + * + * However, the inner IPv4 packet is not really bound to any receiving + * interface and thus the End.DT4 sets the VRF (associated with the + * corresponding routing table) as the *receiving* interface. + * In other words, the End.DT4 processes a packet as if it has been received + * directly by the VRF (and not by one of its slave devices, if any). + * In this way, the VRF interface is used for routing the IPv4 packet in + * according to the routing table configured by the End.DT4 instance. + * + * This design allows you to get some interesting features like: + * 1) the statistics on rx packets; + * 2) the possibility to install a packet sniffer on the receiving interface + * (the VRF one) for looking at the incoming packets; + * 3) the possibility to leverage the netfilter prerouting hook for the inner + * IPv4 packet. + * + * This function returns: + * - the sk_buff* when the VRF rcv handler has processed the packet correctly; + * - NULL when the skb is consumed by the VRF rcv handler; + * - a pointer which encodes a negative error number in case of error. + * Note that in this case, the function takes care of freeing the skb. + */ +static struct sk_buff *end_dt4_vrf_rcv(struct sk_buff *skb, + struct net_device *dev) +{ + /* based on l3mdev_ip_rcv; we are only interested in the master */ + if (unlikely(!netif_is_l3_master(dev) && !netif_has_l3_rx_handler(dev))) + goto drop; + + if (unlikely(!dev->l3mdev_ops->l3mdev_l3_rcv)) + goto drop; + + /* the decap packet (IPv4) does not come with any mac header info. + * We must unset the mac header to allow the VRF device to rebuild it, + * just in case there is a sniffer attached on the device. + */ + skb_unset_mac_header(skb); + + skb = dev->l3mdev_ops->l3mdev_l3_rcv(dev, skb, AF_INET); + if (!skb) + /* the skb buffer was consumed by the handler */ + return NULL; + + /* when a packet is received by a VRF or by one of its slaves, the + * master device reference is set into the skb. + */ + if (unlikely(skb->dev != dev || skb->skb_iif != dev->ifindex)) + goto drop; + + return skb; + +drop: + kfree_skb(skb); + return ERR_PTR(-EINVAL); +} + +static struct net_device *end_dt4_get_vrf_rcu(struct sk_buff *skb, + struct seg6_end_dt4_info *info) +{ + int vrf_ifindex = info->vrf_ifindex; + struct net *net = info->net; + + if (unlikely(vrf_ifindex < 0)) + goto error; + + if (unlikely(!net_eq(dev_net(skb->dev), net))) + goto error; + + return dev_get_by_index_rcu(net, vrf_ifindex); + +error: + return NULL; +} + +static int input_action_end_dt4(struct sk_buff *skb, + struct seg6_local_lwt *slwt) +{ + struct net_device *vrf; + struct iphdr *iph; + int err; + + if (!decap_and_validate(skb, IPPROTO_IPIP)) + goto drop; + + if (!pskb_may_pull(skb, sizeof(struct iphdr))) + goto drop; + + vrf = end_dt4_get_vrf_rcu(skb, &slwt->dt4_info); + if (unlikely(!vrf)) + goto drop; + + skb->protocol = htons(ETH_P_IP); + + skb_dst_drop(skb); + + skb_set_transport_header(skb, sizeof(struct iphdr)); + + skb = end_dt4_vrf_rcv(skb, vrf); + if (!skb) + /* packet has been processed and consumed by the VRF */ + return 0; + + if (IS_ERR(skb)) { + err = PTR_ERR(skb); + return err; + } + + iph = ip_hdr(skb); + + err = ip_route_input(skb, iph->daddr, iph->saddr, 0, skb->dev); + if (err) + goto drop; + + return dst_input(skb); + +drop: + kfree_skb(skb); + return -EINVAL; +} + +#else + +static int seg6_end_dt4_build(struct seg6_local_lwt *slwt, const void *cfg, + struct netlink_ext_ack *extack) +{ + NL_SET_ERR_MSG(extack, "Operation is not supported"); + + return -EOPNOTSUPP; +} + +static int input_action_end_dt4(struct sk_buff *skb, + struct seg6_local_lwt *slwt) +{ + kfree_skb(skb); + return -EOPNOTSUPP; +} + +#endif + static int input_action_end_dt6(struct sk_buff *skb, struct seg6_local_lwt *slwt) { @@ -601,6 +798,14 @@ static struct seg6_action_desc seg6_action_table[] = { .attrs = (1 << SEG6_LOCAL_NH4), .input = input_action_end_dx4, }, + { + .action = SEG6_LOCAL_ACTION_END_DT4, + .attrs = (1 << SEG6_LOCAL_TABLE), + .input = input_action_end_dt4, + .slwt_ops = { + .build_state = seg6_end_dt4_build, + }, + }, { .action = SEG6_LOCAL_ACTION_END_DT6, .attrs = (1 << SEG6_LOCAL_TABLE),
SRv6 End.DT4 is defined in the SRv6 Network Programming [1]. The SRv6 End.DT4 is used to implement IPv4 L3VPN use-cases in multi-tenants environments. It decapsulates the received packets and it performs IPv4 routing lookup in the routing table of the tenant. The SRv6 End.DT4 Linux implementation leverages a VRF device in order to force the routing lookup into the associated routing table. To make the End.DT4 work properly, it must be guaranteed that the routing table used for routing lookup operations is bound to one and only one VRF during the tunnel creation. Such constraint has to be enforced by enabling the VRF strict_mode sysctl parameter, i.e: $ sysctl -wq net.vrf.strict_mode=1. At JANOG44, LINE corporation presented their multi-tenant DC architecture using SRv6 [2]. In the slides, they reported that the Linux kernel is missing the support of SRv6 End.DT4 behavior. The iproute2 counterpart required for configuring the SRv6 End.DT4 behavior is already implemented along with the other supported SRv6 behaviors [3]. [1] https://tools.ietf.org/html/draft-ietf-spring-srv6-network-programming [2] https://speakerdeck.com/line_developers/line-data-center-networking-with-srv6 [3] https://patchwork.ozlabs.org/patch/799837/ Signed-off-by: Andrea Mayer <andrea.mayer@uniroma2.it> --- net/ipv6/seg6_local.c | 205 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 205 insertions(+)