| Message ID | 72e04afdffddf899e62ebd67ae2cb62dab09f95d.1496998307.git.echaudro@redhat.com |
|---|---|
| State | Deferred |
| Headers | show |
On Fri, Jun 09, 2017 at 11:09:08AM +0200, Eelco Chaudron wrote: > This is a follow up patch for an earlier patch send by Cascardo, > however I think this patch might not be needed... > > This patch will make sure VXLAN tunnels with and without the group > based policy (gbp) option enabled can not coexist on the same > destination udp port. > > However the interface ports for VXLAN have to be unique on the same > destination port, i.e. they need a different VNI. Looking at the > datapath code (only Linux seems to support this), this is not a > problem for the ingress/egress path. For egress based on the > configuration the correct header is build. For ingress, if gbp is not > configured and a gbp VXLAN is received the packet is dropped. If gbp > is enabled and a non gbp packet is received its accepted (meaning > default group policy as per the draft rfc). > > Can some one that worked more in depth on the VXLAN side confirm this > patch can be tossed in the bin? If I missed some specific > configuration / use case why it is needed, please review the patch. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> I've read this commit message a few times and I'm still not confident that I understand. Let me restate it and you can correct me if I'm wrong. I *think* that you are saying that the Linux datapath handles GBP and non-GBP tunnels that are otherwise the same in a sensible way, so that there is no need to add code to reject them. Is that right? Thanks, Ben.
On 07/07/2017 08:32 PM, Ben Pfaff wrote: > On Fri, Jun 09, 2017 at 11:09:08AM +0200, Eelco Chaudron wrote: >> This is a follow up patch for an earlier patch send by Cascardo, >> however I think this patch might not be needed... >> >> This patch will make sure VXLAN tunnels with and without the group >> based policy (gbp) option enabled can not coexist on the same >> destination udp port. >> >> However the interface ports for VXLAN have to be unique on the same >> destination port, i.e. they need a different VNI. Looking at the >> datapath code (only Linux seems to support this), this is not a >> problem for the ingress/egress path. For egress based on the >> configuration the correct header is build. For ingress, if gbp is not >> configured and a gbp VXLAN is received the packet is dropped. If gbp >> is enabled and a non gbp packet is received its accepted (meaning >> default group policy as per the draft rfc). >> >> Can some one that worked more in depth on the VXLAN side confirm this >> patch can be tossed in the bin? If I missed some specific >> configuration / use case why it is needed, please review the patch. >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > I've read this commit message a few times and I'm still not confident > that I understand. Let me restate it and you can correct me if I'm > wrong. I *think* that you are saying that the Linux datapath handles > GBP and non-GBP tunnels that are otherwise the same in a sensible way, > so that there is no need to add code to reject them. Is that right? > > Thanks, > > Ben. Hi Ben, Yes your summary is correct! I was just wondering if I missed something that does require this fix to be added. Cheers, Eelco
On Mon, Jul 10, 2017 at 10:19:49AM +0200, Eelco Chaudron wrote: > On 07/07/2017 08:32 PM, Ben Pfaff wrote: > > On Fri, Jun 09, 2017 at 11:09:08AM +0200, Eelco Chaudron wrote: > > > This is a follow up patch for an earlier patch send by Cascardo, > > > however I think this patch might not be needed... > > > > > > This patch will make sure VXLAN tunnels with and without the group > > > based policy (gbp) option enabled can not coexist on the same > > > destination udp port. > > > > > > However the interface ports for VXLAN have to be unique on the same > > > destination port, i.e. they need a different VNI. Looking at the > > > datapath code (only Linux seems to support this), this is not a > > > problem for the ingress/egress path. For egress based on the > > > configuration the correct header is build. For ingress, if gbp is not > > > configured and a gbp VXLAN is received the packet is dropped. If gbp > > > is enabled and a non gbp packet is received its accepted (meaning > > > default group policy as per the draft rfc). But, then, it does not go through the non-GBP configured vport, does it? So any flows configured for the non-GBP port are ignored. Doesn't it at least cause user confusion? I'd say it's a non-supported configuration, and OVS should just not allow it. Cascardo. > > > > > > Can some one that worked more in depth on the VXLAN side confirm this > > > patch can be tossed in the bin? If I missed some specific > > > configuration / use case why it is needed, please review the patch. > > > > > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > > I've read this commit message a few times and I'm still not confident > > that I understand. Let me restate it and you can correct me if I'm > > wrong. I *think* that you are saying that the Linux datapath handles > > GBP and non-GBP tunnels that are otherwise the same in a sensible way, > > so that there is no need to add code to reject them. Is that right? > > > > Thanks, > > > > Ben. > Hi Ben, > > Yes your summary is correct! I was just wondering if I missed something > that does require this fix to be added. > > Cheers, > > Eelco
On 10/07/17 11:47, Thadeu Lima de Souza Cascardo wrote: > On Mon, Jul 10, 2017 at 10:19:49AM +0200, Eelco Chaudron wrote: >> On 07/07/2017 08:32 PM, Ben Pfaff wrote: >>> On Fri, Jun 09, 2017 at 11:09:08AM +0200, Eelco Chaudron wrote: >>>> This is a follow up patch for an earlier patch send by Cascardo, >>>> however I think this patch might not be needed... >>>> >>>> This patch will make sure VXLAN tunnels with and without the group >>>> based policy (gbp) option enabled can not coexist on the same >>>> destination udp port. >>>> >>>> However the interface ports for VXLAN have to be unique on the same >>>> destination port, i.e. they need a different VNI. Looking at the >>>> datapath code (only Linux seems to support this), this is not a >>>> problem for the ingress/egress path. For egress based on the >>>> configuration the correct header is build. For ingress, if gbp is not >>>> configured and a gbp VXLAN is received the packet is dropped. If gbp >>>> is enabled and a non gbp packet is received its accepted (meaning >>>> default group policy as per the draft rfc). > But, then, it does not go through the non-GBP configured vport, does it? > So any flows configured for the non-GBP port are ignored. Doesn't it at > least cause user confusion? I'd say it's a non-supported configuration, > and OVS should just not allow it. > > Cascardo. I do not think its a non-supported configuration, as you could have a non-GBP VNI (key) and a GBP VNIconfigured on the same UDP port. The only problem I see, (looking at it again) is with the key=flow option. An installed rule could use a VNI (key) setup for a (non-)GBP interface. For example with the following configuration: $ ovs-vsctl add-port br0 p1 -- set Interface p1 type=vxlan \ options:key=1 options:remote_ip=flow ofport_request=1 $ ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan \ options:key=flow options:remote_ip=flow options:exts=gbp \ ofport_request=2 If an OF rules egressing p2 uses 1 as the key it will be dropped by the remote site as its expecting a non GBP. The patch will prevent a configuration like the one above, however I could not find any reference in the RFC that this is an invalid configuration, i.e. having GBP and non-GBP VXLAN sessions multiplexed on the same UDP port based on the VNI value. The only problem I have with applying this patch is that existing configurations (that are working) will fail. > >>>> Can some one that worked more in depth on the VXLAN side confirm this >>>> patch can be tossed in the bin? If I missed some specific >>>> configuration / use case why it is needed, please review the patch. >>>> >>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>> I've read this commit message a few times and I'm still not confident >>> that I understand. Let me restate it and you can correct me if I'm >>> wrong. I *think* that you are saying that the Linux datapath handles >>> GBP and non-GBP tunnels that are otherwise the same in a sensible way, >>> so that there is no need to add code to reject them. Is that right? >>> >>> Thanks, >>> >>> Ben. >> Hi Ben, >> >> Yes your summary is correct! I was just wondering if I missed something >> that does require this fix to be added. >> >> Cheers, >> >> Eelco
On Mon, Jul 10, 2017 at 06:47:21AM -0300, Thadeu Lima de Souza Cascardo wrote: > On Mon, Jul 10, 2017 at 10:19:49AM +0200, Eelco Chaudron wrote: > > On 07/07/2017 08:32 PM, Ben Pfaff wrote: > > > On Fri, Jun 09, 2017 at 11:09:08AM +0200, Eelco Chaudron wrote: > > > > This is a follow up patch for an earlier patch send by Cascardo, > > > > however I think this patch might not be needed... > > > > > > > > This patch will make sure VXLAN tunnels with and without the group > > > > based policy (gbp) option enabled can not coexist on the same > > > > destination udp port. > > > > > > > > However the interface ports for VXLAN have to be unique on the same > > > > destination port, i.e. they need a different VNI. Looking at the > > > > datapath code (only Linux seems to support this), this is not a > > > > problem for the ingress/egress path. For egress based on the > > > > configuration the correct header is build. For ingress, if gbp is not > > > > configured and a gbp VXLAN is received the packet is dropped. If gbp > > > > is enabled and a non gbp packet is received its accepted (meaning > > > > default group policy as per the draft rfc). > > But, then, it does not go through the non-GBP configured vport, does it? > So any flows configured for the non-GBP port are ignored. Doesn't it at > least cause user confusion? I'd say it's a non-supported configuration, > and OVS should just not allow it. > > Cascardo. You also have to consider whether ovs will configure the vxlan device to have either GBP flag set or not. At least, this is a "bug" you should take care of. And as ovs names its vxlan devices vxlan_sys_<port>, iirc, it would fail to create one of the ports as the devices already existed. If you really believe it should just work, then you might still want to "merge" these concomitant ports and use the superset of the flags for them. Also, take note that this was work that was done in the GPE context, so expecting that whatever affected GBP would also affect GPE. If you demonstrante this works fine for GBP, maybe it doesn't for GPE, and you might want to consider implementing this anyway, but using a test for "conflicting" flags. From what I remember, whenever configuring two ports with conflicting flags (one GBP and one non-GBP), OVS would not alert of any failure, but one of the ports would not work, aka, receive any packets. Regards. Cascardo. > > > > > > > > > Can some one that worked more in depth on the VXLAN side confirm this > > > > patch can be tossed in the bin? If I missed some specific > > > > configuration / use case why it is needed, please review the patch. > > > > > > > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > > > I've read this commit message a few times and I'm still not confident > > > that I understand. Let me restate it and you can correct me if I'm > > > wrong. I *think* that you are saying that the Linux datapath handles > > > GBP and non-GBP tunnels that are otherwise the same in a sensible way, > > > so that there is no need to add code to reject them. Is that right? > > > > > > Thanks, > > > > > > Ben. > > Hi Ben, > > > > Yes your summary is correct! I was just wondering if I missed something > > that does require this fix to be added. > > > > Cheers, > > > > Eelco
On 10/07/17 14:03, Thadeu Lima de Souza Cascardo wrote: > On Mon, Jul 10, 2017 at 06:47:21AM -0300, Thadeu Lima de Souza Cascardo wrote: >> ... >>>>> However the interface ports for VXLAN have to be unique on the same >>>>> destination port, i.e. they need a different VNI. Looking at the >>>>> datapath code (only Linux seems to support this), this is not a >>>>> problem for the ingress/egress path. For egress based on the >>>>> configuration the correct header is build. For ingress, if gbp is not >>>>> configured and a gbp VXLAN is received the packet is dropped. If gbp >>>>> is enabled and a non gbp packet is received its accepted (meaning >>>>> default group policy as per the draft rfc). >> But, then, it does not go through the non-GBP configured vport, does it? >> So any flows configured for the non-GBP port are ignored. Doesn't it at >> least cause user confusion? I'd say it's a non-supported configuration, >> and OVS should just not allow it. >> >> Cascardo. > You also have to consider whether ovs will configure the vxlan device to > have either GBP flag set or not. At least, this is a "bug" you should > take care of. And as ovs names its vxlan devices vxlan_sys_<port>, iirc, > it would fail to create one of the ports as the devices already existed. > If you really believe it should just work, then you might still want to > "merge" these concomitant ports and use the superset of the flags for > them. > > Also, take note that this was work that was done in the GPE context, so > expecting that whatever affected GBP would also affect GPE. If you > demonstrante this works fine for GBP, maybe it doesn't for GPE, and you > might want to consider implementing this anyway, but using a test for > "conflicting" flags. > > From what I remember, whenever configuring two ports with conflicting > flags (one GBP and one non-GBP), OVS would not alert of any failure, but > one of the ports would not work, aka, receive any packets. > > Regards. > Cascardo. Thanks for the additional details, will do some more testing and report back...
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index d390f37..a88794f 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -61,11 +61,13 @@ static uint64_t rt_change_seqno; static int get_patch_config(const struct netdev *netdev, struct smap *args); static int get_tunnel_config(const struct netdev *, struct smap *args); static bool tunnel_check_status_change__(struct netdev_vport *); +static const struct vport_class *netdev_vport_get_class_by_name(const char *); struct vport_class { const char *dpif_port; struct netdev_class netdev_class; }; +static const struct vport_class vport_classes[]; bool netdev_vport_is_vport_class(const struct netdev_class *class) @@ -403,6 +405,43 @@ parse_tunnel_ip(const char *value, bool accept_mcast, bool *flow, return 0; } +static bool +is_concomitant_vxlan_tunnel_present(struct netdev_vport *dev, + const struct netdev_tunnel_config *tnl_cfg) { + bool is_present = false; + struct shash device_shash; + struct shash_node *node; + const char *type = netdev_get_type(&dev->up); + const struct vport_class *vport_class; + + if (strcmp(type, "vxlan")) { + return false; + } + + shash_init(&device_shash); + vport_class = netdev_vport_get_class_by_name("vxlan"); + ovs_assert(vport_class) + + netdev_get_devices(&vport_class->netdev_class, &device_shash); + + SHASH_FOR_EACH (node, &device_shash) { + struct netdev *netdev_ = node->data; + struct netdev_vport *netdev = netdev_vport_cast(netdev_); + + if (netdev != dev && + tnl_cfg->dst_port == netdev->tnl_cfg.dst_port && + (tnl_cfg->exts & (1 << OVS_VXLAN_EXT_GBP)) != + (netdev->tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GBP))) { + is_present = true; + } + netdev_close(netdev_); + } + + shash_destroy(&device_shash); + + return is_present; +} + static int set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp) { @@ -576,6 +615,15 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp) &tnl_cfg.out_key_present, &tnl_cfg.out_key_flow); + if (is_concomitant_vxlan_tunnel_present(dev, &tnl_cfg)) { + ds_put_format(&errors, "%s: VXLAN-GBP, and non-VXLAN-GBP " + "tunnels can't be configured on the same " + "dst_port\n", + name); + err = EEXIST; + goto out; + } + ovs_mutex_lock(&dev->mutex); if (memcmp(&dev->tnl_cfg, &tnl_cfg, sizeof tnl_cfg)) { dev->tnl_cfg = tnl_cfg; @@ -885,24 +933,37 @@ get_stats(const struct netdev *netdev, struct netdev_stats *stats) tunnel_get_status, \ BUILD_HEADER, PUSH_HEADER, POP_HEADER) }} +/* The name of the dpif_port should be short enough to accomodate adding + * a port number to the end if one is necessary. */ +static const struct vport_class vport_classes[] = { + TUNNEL_CLASS("geneve", "genev_sys", netdev_geneve_build_header, + netdev_tnl_push_udp_header, + netdev_geneve_pop_header), + TUNNEL_CLASS("gre", "gre_sys", netdev_gre_build_header, + netdev_gre_push_header, + netdev_gre_pop_header), + TUNNEL_CLASS("vxlan", "vxlan_sys", netdev_vxlan_build_header, + netdev_tnl_push_udp_header, + netdev_vxlan_pop_header), + TUNNEL_CLASS("lisp", "lisp_sys", NULL, NULL, NULL), + TUNNEL_CLASS("stt", "stt_sys", NULL, NULL, NULL), +}; + +static const struct vport_class * +netdev_vport_get_class_by_name(const char *name) { + int i; + + for (i = 0; i < ARRAY_SIZE(vport_classes); i++) { + if (!strcmp(vport_classes[i].netdev_class.type, name)) { + return &vport_classes[i]; + } + } + return NULL; +} + void netdev_vport_tunnel_register(void) { - /* The name of the dpif_port should be short enough to accomodate adding - * a port number to the end if one is necessary. */ - static const struct vport_class vport_classes[] = { - TUNNEL_CLASS("geneve", "genev_sys", netdev_geneve_build_header, - netdev_tnl_push_udp_header, - netdev_geneve_pop_header), - TUNNEL_CLASS("gre", "gre_sys", netdev_gre_build_header, - netdev_gre_push_header, - netdev_gre_pop_header), - TUNNEL_CLASS("vxlan", "vxlan_sys", netdev_vxlan_build_header, - netdev_tnl_push_udp_header, - netdev_vxlan_pop_header), - TUNNEL_CLASS("lisp", "lisp_sys", NULL, NULL, NULL), - TUNNEL_CLASS("stt", "stt_sys", NULL, NULL, NULL), - }; static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; if (ovsthread_once_start(&once)) { diff --git a/tests/tunnel.at b/tests/tunnel.at index 222fb8d..e209124 100644 --- a/tests/tunnel.at +++ b/tests/tunnel.at @@ -676,6 +676,35 @@ AT_CHECK([tail -1 stdout], [0], [Datapath actions: set(tunnel(tun_id=0x0,dst=1.1.1.1,ttl=64,tp_dst=4789,flags(df|key))),4789 ]) +OVS_VSWITCHD_STOP +AT_CLEANUP + +AT_SETUP([tunnel - concomitant incompatible tunnels on the same port]) +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \ + options:remote_ip=flow ofport_request=1]) + +AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan \ + options:remote_ip=flow options:exts=gbp options:key=1 ofport_request=2], [0], + [], [ignore]) + +AT_CHECK([grep 'p2: could not set configuration (File exists)' ovs-vswitchd.log | sed "s/^.*\(p2:.*\)$/\1/"], [0], + [p2: could not set configuration (File exists) +]) + +OVS_VSWITCHD_STOP(["/p2: VXLAN-GBP, and non-VXLAN-GBP tunnels can't be configured on the same dst_port/d +/p2: could not set configuration (File exists)/d"]) +AT_CLEANUP + +AT_SETUP([tunnel - concomitant incompatible tunnels on different ports]) +OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=vxlan \ + options:remote_ip=flow ofport_request=1]) + +AT_CHECK([ovs-vsctl add-port br0 p2 -- set Interface p2 type=vxlan options:dst_port=9000 \ + options:remote_ip=flow options:exts=gbp ofport_request=2]) + +AT_CHECK([grep p2 ovs-vswitchd.log | sed "s/^.*\(bridge br0:.*\)$/\1/"], [0], + [bridge br0: added interface p2 on port 2 +]) OVS_VSWITCHD_STOP AT_CLEANUP
This is a follow up patch for an earlier patch send by Cascardo, however I think this patch might not be needed... This patch will make sure VXLAN tunnels with and without the group based policy (gbp) option enabled can not coexist on the same destination udp port. However the interface ports for VXLAN have to be unique on the same destination port, i.e. they need a different VNI. Looking at the datapath code (only Linux seems to support this), this is not a problem for the ingress/egress path. For egress based on the configuration the correct header is build. For ingress, if gbp is not configured and a gbp VXLAN is received the packet is dropped. If gbp is enabled and a non gbp packet is received its accepted (meaning default group policy as per the draft rfc). Can some one that worked more in depth on the VXLAN side confirm this patch can be tossed in the bin? If I missed some specific configuration / use case why it is needed, please review the patch. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/netdev-vport.c | 91 +++++++++++++++++++++++++++++++++++++++++++++--------- tests/tunnel.at | 29 +++++++++++++++++ 2 files changed, 105 insertions(+), 15 deletions(-)