| Message ID | 20190813163404.19126-1-fbl@sysclose.org |
|---|---|
| State | Accepted |
| Commit | ea41c034dbc76fccfef6ce71e8a4768d6eff0b3a |
| Headers | show |
| Series | [ovs-dev] tnl-neigh: Use outgoing ofproto version. | expand |
On Tue, Aug 13, 2019 at 01:34:04PM -0300, Flavio Leitner via dev wrote: > When a packet needs to be encapsulated in userspace, the endpoint > address needs to be resolved to fill in the headers. If it is not, > then currently OvS sends either a Neighbor Solicitation (IPv6) > or an ARP Query (IPv4) to resolve it. > > The problem is that the NS/ARP packet will go through the flow > rules in the new bridge, but inheriting the ofproto table version > from the original packet to be encapsulated. When those versions > don't match, the result is unexpected because no flow rules might > be visible, which would cause the default table rule to be used > to drop the packet. Or only part of the flow rules would be visible > and so on. > > Since the NS/ARP packet is created by OvS and will be injected in > the outgoing bridge, use the corresponding ofproto version instead. Hi, Please backport this up to 2.9 at least. Thanks, fbl
Looks good Flavio. Acked-By: Vasu Dasari <vdasari@gmail.com> Thanks -Vasu *Vasu Dasari* On Tue, Aug 13, 2019 at 12:45 PM Flavio Leitner via dev < ovs-dev@openvswitch.org> wrote: > On Tue, Aug 13, 2019 at 01:34:04PM -0300, Flavio Leitner via dev wrote: > > When a packet needs to be encapsulated in userspace, the endpoint > > address needs to be resolved to fill in the headers. If it is not, > > then currently OvS sends either a Neighbor Solicitation (IPv6) > > or an ARP Query (IPv4) to resolve it. > > > > The problem is that the NS/ARP packet will go through the flow > > rules in the new bridge, but inheriting the ofproto table version > > from the original packet to be encapsulated. When those versions > > don't match, the result is unexpected because no flow rules might > > be visible, which would cause the default table rule to be used > > to drop the packet. Or only part of the flow rules would be visible > > and so on. > > > > Since the NS/ARP packet is created by OvS and will be injected in > > the outgoing bridge, use the corresponding ofproto version instead. > > Hi, > > Please backport this up to 2.9 at least. > Thanks, > fbl > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Tue, Aug 13, 2019 at 6:35 PM Flavio Leitner via dev <ovs-dev@openvswitch.org> wrote: > > When a packet needs to be encapsulated in userspace, the endpoint > address needs to be resolved to fill in the headers. If it is not, > then currently OvS sends either a Neighbor Solicitation (IPv6) > or an ARP Query (IPv4) to resolve it. > > The problem is that the NS/ARP packet will go through the flow > rules in the new bridge, but inheriting the ofproto table version > from the original packet to be encapsulated. When those versions > don't match, the result is unexpected because no flow rules might > be visible, which would cause the default table rule to be used > to drop the packet. Or only part of the flow rules would be visible > and so on. > > Since the NS/ARP packet is created by OvS and will be injected in > the outgoing bridge, use the corresponding ofproto version instead. > > Signed-off-by: Flavio Leitner <fbl@sysclose.org> > --- > ofproto/ofproto-dpif-xlate.c | 4 +-- > tests/tunnel.at | 62 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 64 insertions(+), 2 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 28a7fdd84..5a8a46370 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3414,6 +3414,7 @@ compose_table_xlate(struct xlate_ctx *ctx, const struct xport *out_dev, > struct dp_packet *packet) > { > struct xbridge *xbridge = out_dev->xbridge; > + ovs_version_t version = ofproto_dpif_get_tables_version(xbridge->ofproto); > struct ofpact_output output; > struct flow flow; > > @@ -3423,8 +3424,7 @@ compose_table_xlate(struct xlate_ctx *ctx, const struct xport *out_dev, > output.port = OFPP_TABLE; > output.max_len = 0; > > - return ofproto_dpif_execute_actions__(xbridge->ofproto, > - ctx->xin->tables_version, &flow, > + return ofproto_dpif_execute_actions__(xbridge->ofproto, version, &flow, > NULL, &output.ofpact, sizeof output, > ctx->depth, ctx->resubmits, packet); > } > diff --git a/tests/tunnel.at b/tests/tunnel.at > index fc6f87936..faffb4149 100644 > --- a/tests/tunnel.at > +++ b/tests/tunnel.at > @@ -394,6 +394,68 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([tunnel - table version]) > +dnl check if changes in the egress bridge flow table affects > +dnl discovering the link layer address of tunnel endpoints. > +OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 other-config:hwaddr=aa:55:aa:55:00:00]) > +AT_CHECK([ovs-vsctl add-br int-br -- set bridge int-br datapath_type=dummy], [0]) > +AT_CHECK([ovs-vsctl add-port int-br v1 -- set Interface v1 type=vxlan \ > + options:remote_ip=172.31.1.2 options:key=123 \ > + ofport_request=2 \ > + -- add-port int-br v2 -- set Interface v2 type=internal \ > + ofport_request=3 \ > + ], [0]) > + > +AT_CHECK([ovs-appctl dpif/show], [0], [dnl > +dummy@ovs-dummy: hit:0 missed:0 > + br0: > + br0 65534/100: (dummy-internal) > + p0 1/1: (dummy) > + int-br: > + int-br 65534/2: (dummy-internal) > + v1 2/4789: (vxlan: key=123, remote_ip=172.31.1.2) > + v2 3/3: (dummy-internal) > +]) > + > +dnl First setup dummy interface IP address, then add the route > +dnl so that tnl-port table can get valid IP address for the device. > +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 172.31.1.1/24], [0], [OK > +]) > +AT_CHECK([ovs-appctl ovs/route/add 172.31.1.0/24 br0], [0], [OK > +]) > + > +dnl change the flow table to bump the internal table version > +AT_CHECK([ovs-ofctl add-flow int-br action=normal]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > + > +dnl Check Neighbour discovery. > +AT_CHECK([ovs-vsctl -- set Interface p0 options:pcap=p0.pcap]) > + > +AT_CHECK([ovs-appctl netdev-dummy/receive int-br 'in_port(2),eth(src=aa:55:aa:55:00:00,dst=f8:bc:12:ff:ff:ff),eth_type(0x0800),ipv4(src=1.1.3.92,dst=1.1.3.88,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)']) > +AT_CHECK([ovs-pcap p0.pcap > p0.pcap.txt 2>&1]) > + > +dnl When the wrong version is used, the flow is not visible and the > +dnl packet is dropped. > +AT_CHECK([cat p0.pcap.txt | grep ffffffffffffaa55aa55000008060001080006040001aa55aa550000ac1f0101000000000000ac1f0102 | uniq], [0], [dnl > +ffffffffffffaa55aa55000008060001080006040001aa55aa550000ac1f0101000000000000ac1f0102 > +]) > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([tunnel - LISP]) > OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=lisp \ > options:remote_ip=1.1.1.1 ofport_request=1]) > -- > 2.20.1 Reviewed-by: David Marchand <david.marchand@redhat.com> -- David Marchand
Hi Ben and Ilya, This patch has two reviews, so any chance for you to take a look soon as well? Thanks in advance! fbl On Tue, Aug 13, 2019 at 01:34:04PM -0300, Flavio Leitner via dev wrote: > When a packet needs to be encapsulated in userspace, the endpoint > address needs to be resolved to fill in the headers. If it is not, > then currently OvS sends either a Neighbor Solicitation (IPv6) > or an ARP Query (IPv4) to resolve it. > > The problem is that the NS/ARP packet will go through the flow > rules in the new bridge, but inheriting the ofproto table version > from the original packet to be encapsulated. When those versions > don't match, the result is unexpected because no flow rules might > be visible, which would cause the default table rule to be used > to drop the packet. Or only part of the flow rules would be visible > and so on. > > Since the NS/ARP packet is created by OvS and will be injected in > the outgoing bridge, use the corresponding ofproto version instead. > > Signed-off-by: Flavio Leitner <fbl@sysclose.org> > --- > ofproto/ofproto-dpif-xlate.c | 4 +-- > tests/tunnel.at | 62 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 64 insertions(+), 2 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 28a7fdd84..5a8a46370 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3414,6 +3414,7 @@ compose_table_xlate(struct xlate_ctx *ctx, const struct xport *out_dev, > struct dp_packet *packet) > { > struct xbridge *xbridge = out_dev->xbridge; > + ovs_version_t version = ofproto_dpif_get_tables_version(xbridge->ofproto); > struct ofpact_output output; > struct flow flow; > > @@ -3423,8 +3424,7 @@ compose_table_xlate(struct xlate_ctx *ctx, const struct xport *out_dev, > output.port = OFPP_TABLE; > output.max_len = 0; > > - return ofproto_dpif_execute_actions__(xbridge->ofproto, > - ctx->xin->tables_version, &flow, > + return ofproto_dpif_execute_actions__(xbridge->ofproto, version, &flow, > NULL, &output.ofpact, sizeof output, > ctx->depth, ctx->resubmits, packet); > } > diff --git a/tests/tunnel.at b/tests/tunnel.at > index fc6f87936..faffb4149 100644 > --- a/tests/tunnel.at > +++ b/tests/tunnel.at > @@ -394,6 +394,68 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([tunnel - table version]) > +dnl check if changes in the egress bridge flow table affects > +dnl discovering the link layer address of tunnel endpoints. > +OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 other-config:hwaddr=aa:55:aa:55:00:00]) > +AT_CHECK([ovs-vsctl add-br int-br -- set bridge int-br datapath_type=dummy], [0]) > +AT_CHECK([ovs-vsctl add-port int-br v1 -- set Interface v1 type=vxlan \ > + options:remote_ip=172.31.1.2 options:key=123 \ > + ofport_request=2 \ > + -- add-port int-br v2 -- set Interface v2 type=internal \ > + ofport_request=3 \ > + ], [0]) > + > +AT_CHECK([ovs-appctl dpif/show], [0], [dnl > +dummy@ovs-dummy: hit:0 missed:0 > + br0: > + br0 65534/100: (dummy-internal) > + p0 1/1: (dummy) > + int-br: > + int-br 65534/2: (dummy-internal) > + v1 2/4789: (vxlan: key=123, remote_ip=172.31.1.2) > + v2 3/3: (dummy-internal) > +]) > + > +dnl First setup dummy interface IP address, then add the route > +dnl so that tnl-port table can get valid IP address for the device. > +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 172.31.1.1/24], [0], [OK > +]) > +AT_CHECK([ovs-appctl ovs/route/add 172.31.1.0/24 br0], [0], [OK > +]) > + > +dnl change the flow table to bump the internal table version > +AT_CHECK([ovs-ofctl add-flow int-br action=normal]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) > + > +dnl Check Neighbour discovery. > +AT_CHECK([ovs-vsctl -- set Interface p0 options:pcap=p0.pcap]) > + > +AT_CHECK([ovs-appctl netdev-dummy/receive int-br 'in_port(2),eth(src=aa:55:aa:55:00:00,dst=f8:bc:12:ff:ff:ff),eth_type(0x0800),ipv4(src=1.1.3.92,dst=1.1.3.88,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)']) > +AT_CHECK([ovs-pcap p0.pcap > p0.pcap.txt 2>&1]) > + > +dnl When the wrong version is used, the flow is not visible and the > +dnl packet is dropped. > +AT_CHECK([cat p0.pcap.txt | grep ffffffffffffaa55aa55000008060001080006040001aa55aa550000ac1f0101000000000000ac1f0102 | uniq], [0], [dnl > +ffffffffffffaa55aa55000008060001080006040001aa55aa550000ac1f0101000000000000ac1f0102 > +]) > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([tunnel - LISP]) > OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=lisp \ > options:remote_ip=1.1.1.1 ofport_request=1]) > -- > 2.20.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Tue, Aug 13, 2019 at 01:34:04PM -0300, Flavio Leitner via dev wrote: > When a packet needs to be encapsulated in userspace, the endpoint > address needs to be resolved to fill in the headers. If it is not, > then currently OvS sends either a Neighbor Solicitation (IPv6) > or an ARP Query (IPv4) to resolve it. Thanks, I applied this to master and backported it as far as it would go.
On Wed, Aug 28, 2019 at 11:29:54AM -0700, Ben Pfaff wrote: > On Tue, Aug 13, 2019 at 01:34:04PM -0300, Flavio Leitner via dev wrote: > > When a packet needs to be encapsulated in userspace, the endpoint > > address needs to be resolved to fill in the headers. If it is not, > > then currently OvS sends either a Neighbor Solicitation (IPv6) > > or an ARP Query (IPv4) to resolve it. > > Thanks, I applied this to master and backported it as far as it would go. Thanks a lot Ben! fbl
On Wed, Aug 28, 2019 at 11:29:54AM -0700, Ben Pfaff wrote: > On Tue, Aug 13, 2019 at 01:34:04PM -0300, Flavio Leitner via dev wrote: > > When a packet needs to be encapsulated in userspace, the endpoint > > address needs to be resolved to fill in the headers. If it is not, > > then currently OvS sends either a Neighbor Solicitation (IPv6) > > or an ARP Query (IPv4) to resolve it. > > Thanks, I applied this to master and backported it as far as it would go. Hi Ben, Sorry, but 2.9, 2.8 and 2.7 have a different output indentation which breaks the testsuite. I sent out a patch to fix those branches: https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/362166.html Thanks, fbl
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 28a7fdd84..5a8a46370 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3414,6 +3414,7 @@ compose_table_xlate(struct xlate_ctx *ctx, const struct xport *out_dev, struct dp_packet *packet) { struct xbridge *xbridge = out_dev->xbridge; + ovs_version_t version = ofproto_dpif_get_tables_version(xbridge->ofproto); struct ofpact_output output; struct flow flow; @@ -3423,8 +3424,7 @@ compose_table_xlate(struct xlate_ctx *ctx, const struct xport *out_dev, output.port = OFPP_TABLE; output.max_len = 0; - return ofproto_dpif_execute_actions__(xbridge->ofproto, - ctx->xin->tables_version, &flow, + return ofproto_dpif_execute_actions__(xbridge->ofproto, version, &flow, NULL, &output.ofpact, sizeof output, ctx->depth, ctx->resubmits, packet); } diff --git a/tests/tunnel.at b/tests/tunnel.at index fc6f87936..faffb4149 100644 --- a/tests/tunnel.at +++ b/tests/tunnel.at @@ -394,6 +394,68 @@ AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([tunnel - table version]) +dnl check if changes in the egress bridge flow table affects +dnl discovering the link layer address of tunnel endpoints. +OVS_VSWITCHD_START([add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 other-config:hwaddr=aa:55:aa:55:00:00]) +AT_CHECK([ovs-vsctl add-br int-br -- set bridge int-br datapath_type=dummy], [0]) +AT_CHECK([ovs-vsctl add-port int-br v1 -- set Interface v1 type=vxlan \ + options:remote_ip=172.31.1.2 options:key=123 \ + ofport_request=2 \ + -- add-port int-br v2 -- set Interface v2 type=internal \ + ofport_request=3 \ + ], [0]) + +AT_CHECK([ovs-appctl dpif/show], [0], [dnl +dummy@ovs-dummy: hit:0 missed:0 + br0: + br0 65534/100: (dummy-internal) + p0 1/1: (dummy) + int-br: + int-br 65534/2: (dummy-internal) + v1 2/4789: (vxlan: key=123, remote_ip=172.31.1.2) + v2 3/3: (dummy-internal) +]) + +dnl First setup dummy interface IP address, then add the route +dnl so that tnl-port table can get valid IP address for the device. +AT_CHECK([ovs-appctl netdev-dummy/ip4addr br0 172.31.1.1/24], [0], [OK +]) +AT_CHECK([ovs-appctl ovs/route/add 172.31.1.0/24 br0], [0], [OK +]) + +dnl change the flow table to bump the internal table version +AT_CHECK([ovs-ofctl add-flow int-br action=normal]) +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flow br0 action=normal]) + +dnl Check Neighbour discovery. +AT_CHECK([ovs-vsctl -- set Interface p0 options:pcap=p0.pcap]) + +AT_CHECK([ovs-appctl netdev-dummy/receive int-br 'in_port(2),eth(src=aa:55:aa:55:00:00,dst=f8:bc:12:ff:ff:ff),eth_type(0x0800),ipv4(src=1.1.3.92,dst=1.1.3.88,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)']) +AT_CHECK([ovs-pcap p0.pcap > p0.pcap.txt 2>&1]) + +dnl When the wrong version is used, the flow is not visible and the +dnl packet is dropped. +AT_CHECK([cat p0.pcap.txt | grep ffffffffffffaa55aa55000008060001080006040001aa55aa550000ac1f0101000000000000ac1f0102 | uniq], [0], [dnl +ffffffffffffaa55aa55000008060001080006040001aa55aa550000ac1f0101000000000000ac1f0102 +]) +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([tunnel - LISP]) OVS_VSWITCHD_START([add-port br0 p1 -- set Interface p1 type=lisp \ options:remote_ip=1.1.1.1 ofport_request=1])
When a packet needs to be encapsulated in userspace, the endpoint address needs to be resolved to fill in the headers. If it is not, then currently OvS sends either a Neighbor Solicitation (IPv6) or an ARP Query (IPv4) to resolve it. The problem is that the NS/ARP packet will go through the flow rules in the new bridge, but inheriting the ofproto table version from the original packet to be encapsulated. When those versions don't match, the result is unexpected because no flow rules might be visible, which would cause the default table rule to be used to drop the packet. Or only part of the flow rules would be visible and so on. Since the NS/ARP packet is created by OvS and will be injected in the outgoing bridge, use the corresponding ofproto version instead. Signed-off-by: Flavio Leitner <fbl@sysclose.org> --- ofproto/ofproto-dpif-xlate.c | 4 +-- tests/tunnel.at | 62 ++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-)