Message ID | 20170816170202.456851-4-equinox@diac24.net |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Hi David, [auto build test ERROR on net-next/master] [also build test ERROR on next-20170817] [cannot apply to v4.13-rc5] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/David-Lamparter/bridge-learn-dst-metadata-in-FDB/20170820-001849 config: xtensa-allmodconfig (attached as .config) compiler: xtensa-linux-gcc (GCC) 4.9.0 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=xtensa All error/warnings (new ones prefixed by >>): In file included from net/mpls/af_mpls.c:25:0: >> net/mpls/internal.h:231:19: error: function declaration isn't a prototype [-Werror=strict-prototypes] static inline int vpls_rcv(skb, in_dev, pt, rt, hdr, orig_dev) ^ net/mpls/internal.h: In function 'vpls_rcv': >> net/mpls/internal.h:233:2: warning: passing argument 1 of 'kfree_skb' makes pointer from integer without a cast kfree_skb(skb); ^ In file included from net/mpls/af_mpls.c:2:0: include/linux/skbuff.h:956:6: note: expected 'struct sk_buff *' but argument is of type 'int' void kfree_skb(struct sk_buff *skb); ^ net/mpls/af_mpls.c: In function 'mpls_rt_xmit': net/mpls/af_mpls.c:459:45: warning: 'out_dev' may be used uninitialized in this function [-Wmaybe-uninitialized] out_mdev = out_dev ? mpls_dev_get(out_dev) : NULL; ^ cc1: some warnings being treated as errors -- In file included from net/mpls/mpls_iptunnel.c:28:0: >> net/mpls/internal.h:231:19: error: function declaration isn't a prototype [-Werror=strict-prototypes] static inline int vpls_rcv(skb, in_dev, pt, rt, hdr, orig_dev) ^ net/mpls/internal.h: In function 'vpls_rcv': >> net/mpls/internal.h:233:2: warning: passing argument 1 of 'kfree_skb' makes pointer from integer without a cast kfree_skb(skb); ^ In file included from net/mpls/mpls_iptunnel.c:14:0: include/linux/skbuff.h:956:6: note: expected 'struct sk_buff *' but argument is of type 'int' void kfree_skb(struct sk_buff *skb); ^ cc1: some warnings being treated as errors vim +231 net/mpls/internal.h 229 230 #else /* !CONFIG_MPLS_VPLS */ > 231 static inline int vpls_rcv(skb, in_dev, pt, rt, hdr, orig_dev) 232 { > 233 kfree_skb(skb); 234 return NET_RX_DROP; 235 } 236 static inline int is_vpls_dev(struct net_device *dev) 237 { 238 return 0; 239 } 240 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 08/16/2017 07:01 PM, David Lamparter wrote: > This wires up the neccessary calls for VPLS into the MPLS forwarding > pieces. Since CONFIG_MPLS_VPLS doesn't exist yet in Kconfig, it'll > never be enabled, so we're on the stubs for now. > > Signed-off-by: David Lamparter <equinox@diac24.net> > --- > include/uapi/linux/rtnetlink.h | 1 + > net/mpls/af_mpls.c | 54 ++++++++++++++++++++++++++++++++++++++++++ > net/mpls/internal.h | 29 +++++++++++++++++++++++ > 3 files changed, 84 insertions(+) > > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h > index dab7dad9e01a..b7840ed94526 100644 > --- a/include/uapi/linux/rtnetlink.h > +++ b/include/uapi/linux/rtnetlink.h > @@ -326,6 +326,7 @@ enum rtattr_type_t { > RTA_PAD, > RTA_UID, > RTA_TTL_PROPAGATE, > + RTA_VPLS_IF, > __RTA_MAX > }; > > diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c > index 0c5953e5d5bd..4d3ce007b7db 100644 > --- a/net/mpls/af_mpls.c > +++ b/net/mpls/af_mpls.c > @@ -299,6 +299,11 @@ static bool mpls_egress(struct net *net, struct mpls_route *rt, > success = true; > break; > } > + case MPT_VPLS: > + /* nothing to do here, no TTL in Ethernet > + * (and we shouldn't mess with the TTL in inner IP packets, > + * pseudowires are supposed to be transparent) */ > + break; > case MPT_UNSPEC: > /* Should have decided which protocol it is by now */ > break; > @@ -349,6 +354,8 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev, > goto drop; > } > > + if (rt->rt_payload_type == MPT_VPLS) > + return vpls_rcv(skb, dev, pt, rt, hdr, orig_dev); you should get the ret value of vpls_rcv() and increment stats if error occurs. int ret; ret = vpls_rc(...) if (ret) goto err; return NET_RX_SUCCESS; > > /* Pop the label */ > skb_pull(skb, sizeof(*hdr)); > @@ -469,6 +476,7 @@ static const struct nla_policy rtm_mpls_policy[RTA_MAX+1] = { > struct mpls_route_config { > u32 rc_protocol; > u32 rc_ifindex; > + u32 rc_vpls_ifindex; > u8 rc_via_table; > u8 rc_via_alen; > u8 rc_via[MAX_VIA_ALEN]; > @@ -541,6 +549,8 @@ static void mpls_route_update(struct net *net, unsigned index, > rt = rtnl_dereference(platform_label[index]); > rcu_assign_pointer(platform_label[index], new); > > + vpls_label_update(index, rt, new); > + > mpls_notify_route(net, index, rt, new, info); > > /* If we removed a route free it now */ > @@ -942,6 +952,7 @@ static int mpls_route_add(struct mpls_route_config *cfg, > struct mpls_route __rcu **platform_label; > struct net *net = cfg->rc_nlinfo.nl_net; > struct mpls_route *rt, *old; > + struct net_device *vpls_dev = NULL; > int err = -EINVAL; > u8 max_via_alen; > unsigned index; > @@ -996,6 +1007,24 @@ static int mpls_route_add(struct mpls_route_config *cfg, > goto errout; > } > > + if (cfg->rc_vpls_ifindex) { > + vpls_dev = dev_get_by_index(net, cfg->rc_vpls_ifindex); > + if (!vpls_dev) { > + err = -ENODEV; > + NL_SET_ERR_MSG(extack, "Invalid VPLS ifindex"); > + goto errout; > + } > + /* we're under RTNL; and we'll drop routes when we're > + * notified the device is going away. */ > + dev_put(vpls_dev); > + > + if (!is_vpls_dev(vpls_dev)) { > + err = -ENODEV; > + NL_SET_ERR_MSG(extack, "Not a VPLS device"); > + goto errout; > + } > + } > + > err = -ENOMEM; > rt = mpls_rt_alloc(nhs, max_via_alen, max_labels); > if (IS_ERR(rt)) { > @@ -1006,6 +1035,7 @@ static int mpls_route_add(struct mpls_route_config *cfg, > rt->rt_protocol = cfg->rc_protocol; > rt->rt_payload_type = cfg->rc_payload_type; > rt->rt_ttl_propagate = cfg->rc_ttl_propagate; > + rt->rt_vpls_dev = vpls_dev; > > if (cfg->rc_mp) > err = mpls_nh_build_multi(cfg, rt, max_labels, extack); > @@ -1430,6 +1460,14 @@ static void mpls_ifdown(struct net_device *dev, int event) > if (!rt) > continue; > > + if (rt->rt_vpls_dev == dev) { > + switch (event) { > + case NETDEV_UNREGISTER: > + mpls_route_update(net, index, NULL, NULL); > + continue; > + } > + } > + > alive = 0; > deleted = 0; > change_nexthops(rt) { > @@ -1777,6 +1815,10 @@ static int rtm_to_route_config(struct sk_buff *skb, > case RTA_OIF: > cfg->rc_ifindex = nla_get_u32(nla); > break; > + case RTA_VPLS_IF: > + cfg->rc_vpls_ifindex = nla_get_u32(nla); > + cfg->rc_payload_type = MPT_VPLS; > + break; > case RTA_NEWDST: > if (nla_get_labels(nla, MAX_NEW_LABELS, > &cfg->rc_output_labels, > @@ -1911,6 +1953,11 @@ static int mpls_dump_route(struct sk_buff *skb, u32 portid, u32 seq, int event, > ttl_propagate)) > goto nla_put_failure; > } > + > + if (rt->rt_vpls_dev) > + if (nla_put_u32(skb, RTA_VPLS_IF, rt->rt_vpls_dev->ifindex)) > + goto nla_put_failure; > + > if (rt->rt_nhn == 1) { > const struct mpls_nh *nh = rt->rt_nh; > > @@ -2220,6 +2267,10 @@ static int mpls_getroute(struct sk_buff *in_skb, struct nlmsghdr *in_nlh, > if (nla_put_labels(skb, RTA_DST, 1, &in_label)) > goto nla_put_failure; > > + if (rt->rt_vpls_dev) > + if (nla_put_u32(skb, RTA_VPLS_IF, rt->rt_vpls_dev->ifindex)) > + goto nla_put_failure; > + > if (nh->nh_labels && > nla_put_labels(skb, RTA_NEWDST, nh->nh_labels, > nh->nh_label)) > @@ -2491,6 +2542,8 @@ static int __init mpls_init(void) > > rtnl_af_register(&mpls_af_ops); > > + vpls_init(); > + > rtnl_register(PF_MPLS, RTM_NEWROUTE, mpls_rtm_newroute, NULL, 0); > rtnl_register(PF_MPLS, RTM_DELROUTE, mpls_rtm_delroute, NULL, 0); > rtnl_register(PF_MPLS, RTM_GETROUTE, mpls_getroute, mpls_dump_routes, > @@ -2510,6 +2563,7 @@ module_init(mpls_init); > static void __exit mpls_exit(void) > { > rtnl_unregister_all(PF_MPLS); > + vpls_exit(); > rtnl_af_unregister(&mpls_af_ops); > dev_remove_pack(&mpls_packet_type); > unregister_netdevice_notifier(&mpls_dev_notifier); > diff --git a/net/mpls/internal.h b/net/mpls/internal.h > index b70c6663d4f3..876ae9993207 100644 > --- a/net/mpls/internal.h > +++ b/net/mpls/internal.h > @@ -76,6 +76,7 @@ struct sk_buff; > > enum mpls_payload_type { > MPT_UNSPEC, /* IPv4 or IPv6 */ > + MPT_VPLS = 2, /* pseudowire */ > MPT_IPV4 = 4, > MPT_IPV6 = 6, > > @@ -153,6 +154,8 @@ struct mpls_route { /* next hop label forwarding entry */ > u8 rt_nh_size; > u8 rt_via_offset; > u8 rt_reserved1; > + struct net_device *rt_vpls_dev; > + > struct mpls_nh rt_nh[0]; > }; > > @@ -214,4 +217,30 @@ struct mpls_route *mpls_route_input_rcu(struct net *net, unsigned index); > int mpls_rt_xmit(struct sk_buff *skb, struct mpls_route *rt, > struct mpls_entry_decoded dec); > > +#ifdef CONFIG_MPLS_VPLS > +int vpls_rcv(struct sk_buff *skb, struct net_device *in_dev, > + struct packet_type *pt, struct mpls_route *rt, > + struct mpls_shim_hdr *hdr, struct net_device *orig_dev); > +void vpls_label_update(unsigned label, struct mpls_route *rt_old, > + struct mpls_route *rt_new); > +__init int vpls_init(void); > +__exit void vpls_exit(void); > +int is_vpls_dev(struct net_device *dev); > + > +#else /* !CONFIG_MPLS_VPLS */ > +static inline int vpls_rcv(skb, in_dev, pt, rt, hdr, orig_dev) > +{ > + kfree_skb(skb); > + return NET_RX_DROP; just return NET_RX_DROP; > +} > +static inline int is_vpls_dev(struct net_device *dev) > +{ > + return 0; > +} > + > +#define vpls_label_update(label, rt_old, rt_new) do { } while (0) > +#define vpls_init() do { } while (0) > +#define vpls_exit() do { } while (0) > +#endif s/ #endif/#endif /* CONFIG_MPLS_VPLS *// > + > #endif /* MPLS_INTERNAL_H */ >
On Mon, Aug 21, 2017 at 04:01:15PM +0200, Amine Kherbouche wrote: > On 08/16/2017 07:01 PM, David Lamparter wrote: > > This wires up the neccessary calls for VPLS into the MPLS forwarding > > pieces. Since CONFIG_MPLS_VPLS doesn't exist yet in Kconfig, it'll > > never be enabled, so we're on the stubs for now. [...] > > + if (rt->rt_payload_type == MPT_VPLS) > > + return vpls_rcv(skb, dev, pt, rt, hdr, orig_dev); > > you should get the ret value of vpls_rcv() and increment stats if error > occurs. An error in vpls_rcv() is not a receive error on the outer device's MPLS layer; the packet was received correctly (and counted for that at the beginning of mpls_forward()) and dispatched onto an appropriately configured VPLS device. vpls_rcv() counts its own stats on the inner device. [...] > > +static inline int vpls_rcv(skb, in_dev, pt, rt, hdr, orig_dev) > > +{ > > + kfree_skb(skb); > > + return NET_RX_DROP; > > just return NET_RX_DROP; This is not correct; the skb ownership is passed down to vpls_rcv(), which itself will pass it on via netif_rx(). This is also why the call in mpls_forward() does *not* "goto drop", instead directly returning. vpls_rcv() consumes the skb in all cases. Doing this the other way around would incur extra overhead through a skb_clone() in vpls_rcv() before giving it to netif_rx(). Note that the vpls_rcv() dummy function in this patch can't be called because the kernel will refuse to install a VPLS route into the label table (the is_vpls_device() check will be 0). The dummy is just to keep the code tidy. [...] > > +#endif > s/ #endif/#endif /* CONFIG_MPLS_VPLS *// Fixed in next ver (along with the bugged prototype reported by the build bot.) Cheers, -David
On 08/21/2017 05:55 PM, David Lamparter wrote: >>> + if (rt->rt_payload_type == MPT_VPLS) >>> > > + return vpls_rcv(skb, dev, pt, rt, hdr, orig_dev); >> > >> > you should get the ret value of vpls_rcv() and increment stats if error >> > occurs. > An error in vpls_rcv() is not a receive error on the outer device's MPLS > layer; the packet was received correctly (and counted for that at the > beginning of mpls_forward()) and dispatched onto an appropriately > configured VPLS device. vpls_rcv() counts its own stats on the inner > device. Right.
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index dab7dad9e01a..b7840ed94526 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -326,6 +326,7 @@ enum rtattr_type_t { RTA_PAD, RTA_UID, RTA_TTL_PROPAGATE, + RTA_VPLS_IF, __RTA_MAX }; diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c index 0c5953e5d5bd..4d3ce007b7db 100644 --- a/net/mpls/af_mpls.c +++ b/net/mpls/af_mpls.c @@ -299,6 +299,11 @@ static bool mpls_egress(struct net *net, struct mpls_route *rt, success = true; break; } + case MPT_VPLS: + /* nothing to do here, no TTL in Ethernet + * (and we shouldn't mess with the TTL in inner IP packets, + * pseudowires are supposed to be transparent) */ + break; case MPT_UNSPEC: /* Should have decided which protocol it is by now */ break; @@ -349,6 +354,8 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev, goto drop; } + if (rt->rt_payload_type == MPT_VPLS) + return vpls_rcv(skb, dev, pt, rt, hdr, orig_dev); /* Pop the label */ skb_pull(skb, sizeof(*hdr)); @@ -469,6 +476,7 @@ static const struct nla_policy rtm_mpls_policy[RTA_MAX+1] = { struct mpls_route_config { u32 rc_protocol; u32 rc_ifindex; + u32 rc_vpls_ifindex; u8 rc_via_table; u8 rc_via_alen; u8 rc_via[MAX_VIA_ALEN]; @@ -541,6 +549,8 @@ static void mpls_route_update(struct net *net, unsigned index, rt = rtnl_dereference(platform_label[index]); rcu_assign_pointer(platform_label[index], new); + vpls_label_update(index, rt, new); + mpls_notify_route(net, index, rt, new, info); /* If we removed a route free it now */ @@ -942,6 +952,7 @@ static int mpls_route_add(struct mpls_route_config *cfg, struct mpls_route __rcu **platform_label; struct net *net = cfg->rc_nlinfo.nl_net; struct mpls_route *rt, *old; + struct net_device *vpls_dev = NULL; int err = -EINVAL; u8 max_via_alen; unsigned index; @@ -996,6 +1007,24 @@ static int mpls_route_add(struct mpls_route_config *cfg, goto errout; } + if (cfg->rc_vpls_ifindex) { + vpls_dev = dev_get_by_index(net, cfg->rc_vpls_ifindex); + if (!vpls_dev) { + err = -ENODEV; + NL_SET_ERR_MSG(extack, "Invalid VPLS ifindex"); + goto errout; + } + /* we're under RTNL; and we'll drop routes when we're + * notified the device is going away. */ + dev_put(vpls_dev); + + if (!is_vpls_dev(vpls_dev)) { + err = -ENODEV; + NL_SET_ERR_MSG(extack, "Not a VPLS device"); + goto errout; + } + } + err = -ENOMEM; rt = mpls_rt_alloc(nhs, max_via_alen, max_labels); if (IS_ERR(rt)) { @@ -1006,6 +1035,7 @@ static int mpls_route_add(struct mpls_route_config *cfg, rt->rt_protocol = cfg->rc_protocol; rt->rt_payload_type = cfg->rc_payload_type; rt->rt_ttl_propagate = cfg->rc_ttl_propagate; + rt->rt_vpls_dev = vpls_dev; if (cfg->rc_mp) err = mpls_nh_build_multi(cfg, rt, max_labels, extack); @@ -1430,6 +1460,14 @@ static void mpls_ifdown(struct net_device *dev, int event) if (!rt) continue; + if (rt->rt_vpls_dev == dev) { + switch (event) { + case NETDEV_UNREGISTER: + mpls_route_update(net, index, NULL, NULL); + continue; + } + } + alive = 0; deleted = 0; change_nexthops(rt) { @@ -1777,6 +1815,10 @@ static int rtm_to_route_config(struct sk_buff *skb, case RTA_OIF: cfg->rc_ifindex = nla_get_u32(nla); break; + case RTA_VPLS_IF: + cfg->rc_vpls_ifindex = nla_get_u32(nla); + cfg->rc_payload_type = MPT_VPLS; + break; case RTA_NEWDST: if (nla_get_labels(nla, MAX_NEW_LABELS, &cfg->rc_output_labels, @@ -1911,6 +1953,11 @@ static int mpls_dump_route(struct sk_buff *skb, u32 portid, u32 seq, int event, ttl_propagate)) goto nla_put_failure; } + + if (rt->rt_vpls_dev) + if (nla_put_u32(skb, RTA_VPLS_IF, rt->rt_vpls_dev->ifindex)) + goto nla_put_failure; + if (rt->rt_nhn == 1) { const struct mpls_nh *nh = rt->rt_nh; @@ -2220,6 +2267,10 @@ static int mpls_getroute(struct sk_buff *in_skb, struct nlmsghdr *in_nlh, if (nla_put_labels(skb, RTA_DST, 1, &in_label)) goto nla_put_failure; + if (rt->rt_vpls_dev) + if (nla_put_u32(skb, RTA_VPLS_IF, rt->rt_vpls_dev->ifindex)) + goto nla_put_failure; + if (nh->nh_labels && nla_put_labels(skb, RTA_NEWDST, nh->nh_labels, nh->nh_label)) @@ -2491,6 +2542,8 @@ static int __init mpls_init(void) rtnl_af_register(&mpls_af_ops); + vpls_init(); + rtnl_register(PF_MPLS, RTM_NEWROUTE, mpls_rtm_newroute, NULL, 0); rtnl_register(PF_MPLS, RTM_DELROUTE, mpls_rtm_delroute, NULL, 0); rtnl_register(PF_MPLS, RTM_GETROUTE, mpls_getroute, mpls_dump_routes, @@ -2510,6 +2563,7 @@ module_init(mpls_init); static void __exit mpls_exit(void) { rtnl_unregister_all(PF_MPLS); + vpls_exit(); rtnl_af_unregister(&mpls_af_ops); dev_remove_pack(&mpls_packet_type); unregister_netdevice_notifier(&mpls_dev_notifier); diff --git a/net/mpls/internal.h b/net/mpls/internal.h index b70c6663d4f3..876ae9993207 100644 --- a/net/mpls/internal.h +++ b/net/mpls/internal.h @@ -76,6 +76,7 @@ struct sk_buff; enum mpls_payload_type { MPT_UNSPEC, /* IPv4 or IPv6 */ + MPT_VPLS = 2, /* pseudowire */ MPT_IPV4 = 4, MPT_IPV6 = 6, @@ -153,6 +154,8 @@ struct mpls_route { /* next hop label forwarding entry */ u8 rt_nh_size; u8 rt_via_offset; u8 rt_reserved1; + struct net_device *rt_vpls_dev; + struct mpls_nh rt_nh[0]; }; @@ -214,4 +217,30 @@ struct mpls_route *mpls_route_input_rcu(struct net *net, unsigned index); int mpls_rt_xmit(struct sk_buff *skb, struct mpls_route *rt, struct mpls_entry_decoded dec); +#ifdef CONFIG_MPLS_VPLS +int vpls_rcv(struct sk_buff *skb, struct net_device *in_dev, + struct packet_type *pt, struct mpls_route *rt, + struct mpls_shim_hdr *hdr, struct net_device *orig_dev); +void vpls_label_update(unsigned label, struct mpls_route *rt_old, + struct mpls_route *rt_new); +__init int vpls_init(void); +__exit void vpls_exit(void); +int is_vpls_dev(struct net_device *dev); + +#else /* !CONFIG_MPLS_VPLS */ +static inline int vpls_rcv(skb, in_dev, pt, rt, hdr, orig_dev) +{ + kfree_skb(skb); + return NET_RX_DROP; +} +static inline int is_vpls_dev(struct net_device *dev) +{ + return 0; +} + +#define vpls_label_update(label, rt_old, rt_new) do { } while (0) +#define vpls_init() do { } while (0) +#define vpls_exit() do { } while (0) +#endif + #endif /* MPLS_INTERNAL_H */
This wires up the neccessary calls for VPLS into the MPLS forwarding pieces. Since CONFIG_MPLS_VPLS doesn't exist yet in Kconfig, it'll never be enabled, so we're on the stubs for now. Signed-off-by: David Lamparter <equinox@diac24.net> --- include/uapi/linux/rtnetlink.h | 1 + net/mpls/af_mpls.c | 54 ++++++++++++++++++++++++++++++++++++++++++ net/mpls/internal.h | 29 +++++++++++++++++++++++ 3 files changed, 84 insertions(+)