| Message ID | 20240812041620.691187-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v1] Reply only for multicast IPv6 Neigh Solicitation packets. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On 8/12/24 06:16, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > IPv6 Neigh Solicitation (NS) responder logical flows match on ip6.dst > field. These flows when translated to datapath flows also match on > ip6.dst, which means a separate datapath flow per destination IP > address. This may cause significant performance issues in some > setups (particularly ovs-dpdk telco deployments). > > This patch addresses this issue by matching on eth.mcast6 so that > datapath flows for normal IPv6 traffic doesn't have to match on > ip6.dst. IPv6 NS packets are generally multicast. A new logical > match "nd_ns_mcast" for this purpose. > > After this patch, We no longer respond to IPv6 NS unicast packets. > This behavior now matches the IPv4 ARP responder flows. Note > that after the commit [1] which was recently added we now only > respond to IPv4 ARP broadcast packets. But we still need to reply to unicast NS requests for router ports, right? Unlike ARP, IPv6 will use unicast NS requests for Reachability Confirmation: 7.3.1. Reachability Confirmation ... In some cases (e.g., UDP-based protocols and routers forwarding packets to hosts), such reachability information may not be readily available from upper-layer protocols. When no hints are available and a node is sending packets to a neighbor, the node actively probes the neighbor using unicast Neighbor Solicitation messages to verify that the forward path is still working. https://datatracker.ietf.org/doc/html/rfc4861#section-7.3.1 Or do we reply to those somewhere else? > > A recent patch [2] from Ilya partially addressed the same datapath > flow explosion issue by matching on eth.mcast6 for MLD packets. > With this patch, we now completely address the datapath flow > explosion issue for IPv6 traffic provided all the logical ports > of a logical switch are not configured with port security. > > [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") > [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD and IGMP.") > > Reported-at: https://issues.redhat.com/browse/FDP-728 > Reported-by: Mike Pattrick <mkp@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> Best regards, Ilya Maximets.
On 8/12/24 13:55, Ilya Maximets wrote: > On 8/12/24 06:16, numans@ovn.org wrote: >> From: Numan Siddique <numans@ovn.org> >> >> IPv6 Neigh Solicitation (NS) responder logical flows match on ip6.dst >> field. These flows when translated to datapath flows also match on >> ip6.dst, which means a separate datapath flow per destination IP >> address. This may cause significant performance issues in some >> setups (particularly ovs-dpdk telco deployments). >> >> This patch addresses this issue by matching on eth.mcast6 so that >> datapath flows for normal IPv6 traffic doesn't have to match on >> ip6.dst. IPv6 NS packets are generally multicast. A new logical >> match "nd_ns_mcast" for this purpose. >> >> After this patch, We no longer respond to IPv6 NS unicast packets. >> This behavior now matches the IPv4 ARP responder flows. Note >> that after the commit [1] which was recently added we now only >> respond to IPv4 ARP broadcast packets. > > But we still need to reply to unicast NS requests for router ports, right? > Unlike ARP, Actually, ARP has a similar mechanism - Unicast Poll, for the same purpose of validating cache entries without fully removing them. See: https://datatracker.ietf.org/doc/html/rfc1122#section-2 So, we should be responding for unicast ARPs for router ports as well. > IPv6 will use unicast NS requests for Reachability Confirmation: > > 7.3.1. Reachability Confirmation > > ... > > In some cases (e.g., UDP-based protocols and routers forwarding > packets to hosts), such reachability information may not be readily > available from upper-layer protocols. When no hints are available > and a node is sending packets to a neighbor, the node actively probes > the neighbor using unicast Neighbor Solicitation messages to verify > that the forward path is still working. > > https://datatracker.ietf.org/doc/html/rfc4861#section-7.3.1 > > Or do we reply to those somewhere else? > >> >> A recent patch [2] from Ilya partially addressed the same datapath >> flow explosion issue by matching on eth.mcast6 for MLD packets. >> With this patch, we now completely address the datapath flow >> explosion issue for IPv6 traffic provided all the logical ports >> of a logical switch are not configured with port security. >> >> [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") >> [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD and IGMP.") >> >> Reported-at: https://issues.redhat.com/browse/FDP-728 >> Reported-by: Mike Pattrick <mkp@redhat.com> >> Signed-off-by: Numan Siddique <numans@ovn.org> > > Best regards, Ilya Maximets.
On 8/12/24 14:05, Ilya Maximets wrote: > On 8/12/24 13:55, Ilya Maximets wrote: >> On 8/12/24 06:16, numans@ovn.org wrote: >>> From: Numan Siddique <numans@ovn.org> >>> >>> IPv6 Neigh Solicitation (NS) responder logical flows match on ip6.dst >>> field. These flows when translated to datapath flows also match on >>> ip6.dst, which means a separate datapath flow per destination IP >>> address. This may cause significant performance issues in some >>> setups (particularly ovs-dpdk telco deployments). >>> >>> This patch addresses this issue by matching on eth.mcast6 so that >>> datapath flows for normal IPv6 traffic doesn't have to match on >>> ip6.dst. IPv6 NS packets are generally multicast. A new logical >>> match "nd_ns_mcast" for this purpose. >>> >>> After this patch, We no longer respond to IPv6 NS unicast packets. >>> This behavior now matches the IPv4 ARP responder flows. Note >>> that after the commit [1] which was recently added we now only >>> respond to IPv4 ARP broadcast packets. >> >> But we still need to reply to unicast NS requests for router ports, right? >> Unlike ARP, > > Actually, ARP has a similar mechanism - Unicast Poll, for the same purpose > of validating cache entries without fully removing them. See: > https://datatracker.ietf.org/doc/html/rfc1122#section-2 > > So, we should be responding for unicast ARPs for router ports as well. > Both for ARP request and NS we only proxy in the switch pipeline. It's perfectly fine to not reply in the OVN pipeline and let packets reach the VIFs. The VIFs will reply to the unicast requests. This is transparent for the requester. I didn't review Numan's patch in detail but the idea behind it is correct. Regards, Dumitru >> IPv6 will use unicast NS requests for Reachability Confirmation: >> >> 7.3.1. Reachability Confirmation >> >> ... >> >> In some cases (e.g., UDP-based protocols and routers forwarding >> packets to hosts), such reachability information may not be readily >> available from upper-layer protocols. When no hints are available >> and a node is sending packets to a neighbor, the node actively probes >> the neighbor using unicast Neighbor Solicitation messages to verify >> that the forward path is still working. >> >> https://datatracker.ietf.org/doc/html/rfc4861#section-7.3.1 >> >> Or do we reply to those somewhere else? >> >>> >>> A recent patch [2] from Ilya partially addressed the same datapath >>> flow explosion issue by matching on eth.mcast6 for MLD packets. >>> With this patch, we now completely address the datapath flow >>> explosion issue for IPv6 traffic provided all the logical ports >>> of a logical switch are not configured with port security. >>> >>> [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") >>> [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD and IGMP.") >>> >>> Reported-at: https://issues.redhat.com/browse/FDP-728 >>> Reported-by: Mike Pattrick <mkp@redhat.com> >>> Signed-off-by: Numan Siddique <numans@ovn.org> >> >> Best regards, Ilya Maximets. >
On 8/12/24 14:07, Dumitru Ceara wrote: > On 8/12/24 14:05, Ilya Maximets wrote: >> On 8/12/24 13:55, Ilya Maximets wrote: >>> On 8/12/24 06:16, numans@ovn.org wrote: >>>> From: Numan Siddique <numans@ovn.org> >>>> >>>> IPv6 Neigh Solicitation (NS) responder logical flows match on ip6.dst >>>> field. These flows when translated to datapath flows also match on >>>> ip6.dst, which means a separate datapath flow per destination IP >>>> address. This may cause significant performance issues in some >>>> setups (particularly ovs-dpdk telco deployments). >>>> >>>> This patch addresses this issue by matching on eth.mcast6 so that >>>> datapath flows for normal IPv6 traffic doesn't have to match on >>>> ip6.dst. IPv6 NS packets are generally multicast. A new logical >>>> match "nd_ns_mcast" for this purpose. >>>> >>>> After this patch, We no longer respond to IPv6 NS unicast packets. >>>> This behavior now matches the IPv4 ARP responder flows. Note >>>> that after the commit [1] which was recently added we now only >>>> respond to IPv4 ARP broadcast packets. >>> >>> But we still need to reply to unicast NS requests for router ports, right? >>> Unlike ARP, >> >> Actually, ARP has a similar mechanism - Unicast Poll, for the same purpose >> of validating cache entries without fully removing them. See: >> https://datatracker.ietf.org/doc/html/rfc1122#section-2 >> >> So, we should be responding for unicast ARPs for router ports as well. >> > > Both for ARP request and NS we only proxy in the switch pipeline. It's > perfectly fine to not reply in the OVN pipeline and let packets reach > the VIFs. The VIFs will reply to the unicast requests. This is > transparent for the requester. I didn't ask about VIFs, my question is about router port IPs, that are only known to OVN., i.e. there is no other entity that would reply. > > I didn't review Numan's patch in detail but the idea behind it is correct. > > Regards, > Dumitru > >>> IPv6 will use unicast NS requests for Reachability Confirmation: >>> >>> 7.3.1. Reachability Confirmation >>> >>> ... >>> >>> In some cases (e.g., UDP-based protocols and routers forwarding >>> packets to hosts), such reachability information may not be readily >>> available from upper-layer protocols. When no hints are available >>> and a node is sending packets to a neighbor, the node actively probes >>> the neighbor using unicast Neighbor Solicitation messages to verify >>> that the forward path is still working. >>> >>> https://datatracker.ietf.org/doc/html/rfc4861#section-7.3.1 >>> >>> Or do we reply to those somewhere else? >>> >>>> >>>> A recent patch [2] from Ilya partially addressed the same datapath >>>> flow explosion issue by matching on eth.mcast6 for MLD packets. >>>> With this patch, we now completely address the datapath flow >>>> explosion issue for IPv6 traffic provided all the logical ports >>>> of a logical switch are not configured with port security. >>>> >>>> [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") >>>> [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD and IGMP.") >>>> >>>> Reported-at: https://issues.redhat.com/browse/FDP-728 >>>> Reported-by: Mike Pattrick <mkp@redhat.com> >>>> Signed-off-by: Numan Siddique <numans@ovn.org> >>> >>> Best regards, Ilya Maximets. >> >
On 8/12/24 14:10, Ilya Maximets wrote: > On 8/12/24 14:07, Dumitru Ceara wrote: >> On 8/12/24 14:05, Ilya Maximets wrote: >>> On 8/12/24 13:55, Ilya Maximets wrote: >>>> On 8/12/24 06:16, numans@ovn.org wrote: >>>>> From: Numan Siddique <numans@ovn.org> >>>>> >>>>> IPv6 Neigh Solicitation (NS) responder logical flows match on ip6.dst >>>>> field. These flows when translated to datapath flows also match on >>>>> ip6.dst, which means a separate datapath flow per destination IP >>>>> address. This may cause significant performance issues in some >>>>> setups (particularly ovs-dpdk telco deployments). >>>>> >>>>> This patch addresses this issue by matching on eth.mcast6 so that >>>>> datapath flows for normal IPv6 traffic doesn't have to match on >>>>> ip6.dst. IPv6 NS packets are generally multicast. A new logical >>>>> match "nd_ns_mcast" for this purpose. >>>>> >>>>> After this patch, We no longer respond to IPv6 NS unicast packets. >>>>> This behavior now matches the IPv4 ARP responder flows. Note >>>>> that after the commit [1] which was recently added we now only >>>>> respond to IPv4 ARP broadcast packets. >>>> >>>> But we still need to reply to unicast NS requests for router ports, right? >>>> Unlike ARP, >>> >>> Actually, ARP has a similar mechanism - Unicast Poll, for the same purpose >>> of validating cache entries without fully removing them. See: >>> https://datatracker.ietf.org/doc/html/rfc1122#section-2 >>> >>> So, we should be responding for unicast ARPs for router ports as well. >>> >> >> Both for ARP request and NS we only proxy in the switch pipeline. It's >> perfectly fine to not reply in the OVN pipeline and let packets reach >> the VIFs. The VIFs will reply to the unicast requests. This is >> transparent for the requester. > > I didn't ask about VIFs, my question is about router port IPs, that are > only known to OVN., i.e. there is no other entity that would reply. > This patch doesn't touch any of the router port related flows. Router ports still reply to all neighbor solicitations (regardless if they're unicast or multicast). >> >> I didn't review Numan's patch in detail but the idea behind it is correct. >> >> Regards, >> Dumitru >> >>>> IPv6 will use unicast NS requests for Reachability Confirmation: >>>> >>>> 7.3.1. Reachability Confirmation >>>> >>>> ... >>>> >>>> In some cases (e.g., UDP-based protocols and routers forwarding >>>> packets to hosts), such reachability information may not be readily >>>> available from upper-layer protocols. When no hints are available >>>> and a node is sending packets to a neighbor, the node actively probes >>>> the neighbor using unicast Neighbor Solicitation messages to verify >>>> that the forward path is still working. >>>> >>>> https://datatracker.ietf.org/doc/html/rfc4861#section-7.3.1 >>>> >>>> Or do we reply to those somewhere else? >>>> >>>>> >>>>> A recent patch [2] from Ilya partially addressed the same datapath >>>>> flow explosion issue by matching on eth.mcast6 for MLD packets. >>>>> With this patch, we now completely address the datapath flow >>>>> explosion issue for IPv6 traffic provided all the logical ports >>>>> of a logical switch are not configured with port security. >>>>> >>>>> [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") >>>>> [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD and IGMP.") >>>>> >>>>> Reported-at: https://issues.redhat.com/browse/FDP-728 >>>>> Reported-by: Mike Pattrick <mkp@redhat.com> >>>>> Signed-off-by: Numan Siddique <numans@ovn.org> >>>> >>>> Best regards, Ilya Maximets. >>> >> >
On 8/12/24 14:14, Dumitru Ceara wrote: > On 8/12/24 14:10, Ilya Maximets wrote: >> On 8/12/24 14:07, Dumitru Ceara wrote: >>> On 8/12/24 14:05, Ilya Maximets wrote: >>>> On 8/12/24 13:55, Ilya Maximets wrote: >>>>> On 8/12/24 06:16, numans@ovn.org wrote: >>>>>> From: Numan Siddique <numans@ovn.org> >>>>>> >>>>>> IPv6 Neigh Solicitation (NS) responder logical flows match on ip6.dst >>>>>> field. These flows when translated to datapath flows also match on >>>>>> ip6.dst, which means a separate datapath flow per destination IP >>>>>> address. This may cause significant performance issues in some >>>>>> setups (particularly ovs-dpdk telco deployments). >>>>>> >>>>>> This patch addresses this issue by matching on eth.mcast6 so that >>>>>> datapath flows for normal IPv6 traffic doesn't have to match on >>>>>> ip6.dst. IPv6 NS packets are generally multicast. A new logical >>>>>> match "nd_ns_mcast" for this purpose. >>>>>> >>>>>> After this patch, We no longer respond to IPv6 NS unicast packets. >>>>>> This behavior now matches the IPv4 ARP responder flows. Note >>>>>> that after the commit [1] which was recently added we now only >>>>>> respond to IPv4 ARP broadcast packets. >>>>> >>>>> But we still need to reply to unicast NS requests for router ports, right? >>>>> Unlike ARP, >>>> >>>> Actually, ARP has a similar mechanism - Unicast Poll, for the same purpose >>>> of validating cache entries without fully removing them. See: >>>> https://datatracker.ietf.org/doc/html/rfc1122#section-2 >>>> >>>> So, we should be responding for unicast ARPs for router ports as well. >>>> >>> >>> Both for ARP request and NS we only proxy in the switch pipeline. It's >>> perfectly fine to not reply in the OVN pipeline and let packets reach >>> the VIFs. The VIFs will reply to the unicast requests. This is >>> transparent for the requester. >> >> I didn't ask about VIFs, my question is about router port IPs, that are >> only known to OVN., i.e. there is no other entity that would reply. >> > > This patch doesn't touch any of the router port related flows. Router > ports still reply to all neighbor solicitations (regardless if they're > unicast or multicast). Could you point me to the code? I'm a little confused, because of this code snippet: build_lswitch_arp_nd_responder_known_ips: /* * Add ARP/ND reply flows if either the * - port is up and it doesn't have 'unknown' address defined or it * doesn't have the option disable_arp_nd_rsp=true. * - port type is router or <--- THIS * - port type is localport */ if (check_lsp_is_up && !lsp_is_up(op->nbsp) && !lsp_is_router(op->nbsp) && strcmp(op->nbsp->type, "localport")) { return; } This suggests that we're replying to ARP/ND requests for router ports in the switch pipline right here where we're now not replying to unicast requests. > >>> >>> I didn't review Numan's patch in detail but the idea behind it is correct. >>> >>> Regards, >>> Dumitru >>> >>>>> IPv6 will use unicast NS requests for Reachability Confirmation: >>>>> >>>>> 7.3.1. Reachability Confirmation >>>>> >>>>> ... >>>>> >>>>> In some cases (e.g., UDP-based protocols and routers forwarding >>>>> packets to hosts), such reachability information may not be readily >>>>> available from upper-layer protocols. When no hints are available >>>>> and a node is sending packets to a neighbor, the node actively probes >>>>> the neighbor using unicast Neighbor Solicitation messages to verify >>>>> that the forward path is still working. >>>>> >>>>> https://datatracker.ietf.org/doc/html/rfc4861#section-7.3.1 >>>>> >>>>> Or do we reply to those somewhere else? >>>>> >>>>>> >>>>>> A recent patch [2] from Ilya partially addressed the same datapath >>>>>> flow explosion issue by matching on eth.mcast6 for MLD packets. >>>>>> With this patch, we now completely address the datapath flow >>>>>> explosion issue for IPv6 traffic provided all the logical ports >>>>>> of a logical switch are not configured with port security. >>>>>> >>>>>> [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") >>>>>> [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD and IGMP.") >>>>>> >>>>>> Reported-at: https://issues.redhat.com/browse/FDP-728 >>>>>> Reported-by: Mike Pattrick <mkp@redhat.com> >>>>>> Signed-off-by: Numan Siddique <numans@ovn.org> >>>>> >>>>> Best regards, Ilya Maximets. >>>> >>> >> >
On 8/12/24 14:22, Ilya Maximets wrote: > On 8/12/24 14:14, Dumitru Ceara wrote: >> On 8/12/24 14:10, Ilya Maximets wrote: >>> On 8/12/24 14:07, Dumitru Ceara wrote: >>>> On 8/12/24 14:05, Ilya Maximets wrote: >>>>> On 8/12/24 13:55, Ilya Maximets wrote: >>>>>> On 8/12/24 06:16, numans@ovn.org wrote: >>>>>>> From: Numan Siddique <numans@ovn.org> >>>>>>> >>>>>>> IPv6 Neigh Solicitation (NS) responder logical flows match on ip6.dst >>>>>>> field. These flows when translated to datapath flows also match on >>>>>>> ip6.dst, which means a separate datapath flow per destination IP >>>>>>> address. This may cause significant performance issues in some >>>>>>> setups (particularly ovs-dpdk telco deployments). >>>>>>> >>>>>>> This patch addresses this issue by matching on eth.mcast6 so that >>>>>>> datapath flows for normal IPv6 traffic doesn't have to match on >>>>>>> ip6.dst. IPv6 NS packets are generally multicast. A new logical >>>>>>> match "nd_ns_mcast" for this purpose. >>>>>>> >>>>>>> After this patch, We no longer respond to IPv6 NS unicast packets. >>>>>>> This behavior now matches the IPv4 ARP responder flows. Note >>>>>>> that after the commit [1] which was recently added we now only >>>>>>> respond to IPv4 ARP broadcast packets. >>>>>> >>>>>> But we still need to reply to unicast NS requests for router ports, right? >>>>>> Unlike ARP, >>>>> >>>>> Actually, ARP has a similar mechanism - Unicast Poll, for the same purpose >>>>> of validating cache entries without fully removing them. See: >>>>> https://datatracker.ietf.org/doc/html/rfc1122#section-2 >>>>> >>>>> So, we should be responding for unicast ARPs for router ports as well. >>>>> >>>> >>>> Both for ARP request and NS we only proxy in the switch pipeline. It's >>>> perfectly fine to not reply in the OVN pipeline and let packets reach >>>> the VIFs. The VIFs will reply to the unicast requests. This is >>>> transparent for the requester. >>> >>> I didn't ask about VIFs, my question is about router port IPs, that are >>> only known to OVN., i.e. there is no other entity that would reply. >>> >> >> This patch doesn't touch any of the router port related flows. Router >> ports still reply to all neighbor solicitations (regardless if they're >> unicast or multicast). > > Could you point me to the code? I'm a little confused, because of > this code snippet: > > build_lswitch_arp_nd_responder_known_ips: > > /* > * Add ARP/ND reply flows if either the > * - port is up and it doesn't have 'unknown' address defined or it > * doesn't have the option disable_arp_nd_rsp=true. > * - port type is router or <--- THIS > * - port type is localport > */ > if (check_lsp_is_up && > !lsp_is_up(op->nbsp) && !lsp_is_router(op->nbsp) && > strcmp(op->nbsp->type, "localport")) { > return; > } > > This suggests that we're replying to ARP/ND requests for router ports > in the switch pipline right here where we're now not replying to unicast > requests. > We're proxy-ing for router port owned IPs too to avoid flooding (and encapsulation) of ARP request/NS packets. But if they're unicast there's no flooding, so we either: - let the VIF reply - reply in the router pipeline for router owned IPs: all calls to build_lrouter_arp_flow() and build_lrouter_nd_flow(). > >> >>>> >>>> I didn't review Numan's patch in detail but the idea behind it is correct. >>>> >>>> Regards, >>>> Dumitru >>>> >>>>>> IPv6 will use unicast NS requests for Reachability Confirmation: >>>>>> >>>>>> 7.3.1. Reachability Confirmation >>>>>> >>>>>> ... >>>>>> >>>>>> In some cases (e.g., UDP-based protocols and routers forwarding >>>>>> packets to hosts), such reachability information may not be readily >>>>>> available from upper-layer protocols. When no hints are available >>>>>> and a node is sending packets to a neighbor, the node actively probes >>>>>> the neighbor using unicast Neighbor Solicitation messages to verify >>>>>> that the forward path is still working. >>>>>> >>>>>> https://datatracker.ietf.org/doc/html/rfc4861#section-7.3.1 >>>>>> >>>>>> Or do we reply to those somewhere else? >>>>>> >>>>>>> >>>>>>> A recent patch [2] from Ilya partially addressed the same datapath >>>>>>> flow explosion issue by matching on eth.mcast6 for MLD packets. >>>>>>> With this patch, we now completely address the datapath flow >>>>>>> explosion issue for IPv6 traffic provided all the logical ports >>>>>>> of a logical switch are not configured with port security. >>>>>>> >>>>>>> [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") >>>>>>> [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD and IGMP.") >>>>>>> >>>>>>> Reported-at: https://issues.redhat.com/browse/FDP-728 >>>>>>> Reported-by: Mike Pattrick <mkp@redhat.com> >>>>>>> Signed-off-by: Numan Siddique <numans@ovn.org> >>>>>> >>>>>> Best regards, Ilya Maximets. >>>>> >>>> >>> >> >
On 8/12/24 14:29, Dumitru Ceara wrote: > On 8/12/24 14:22, Ilya Maximets wrote: >> On 8/12/24 14:14, Dumitru Ceara wrote: >>> On 8/12/24 14:10, Ilya Maximets wrote: >>>> On 8/12/24 14:07, Dumitru Ceara wrote: >>>>> On 8/12/24 14:05, Ilya Maximets wrote: >>>>>> On 8/12/24 13:55, Ilya Maximets wrote: >>>>>>> On 8/12/24 06:16, numans@ovn.org wrote: >>>>>>>> From: Numan Siddique <numans@ovn.org> >>>>>>>> >>>>>>>> IPv6 Neigh Solicitation (NS) responder logical flows match on ip6.dst >>>>>>>> field. These flows when translated to datapath flows also match on >>>>>>>> ip6.dst, which means a separate datapath flow per destination IP >>>>>>>> address. This may cause significant performance issues in some >>>>>>>> setups (particularly ovs-dpdk telco deployments). >>>>>>>> >>>>>>>> This patch addresses this issue by matching on eth.mcast6 so that >>>>>>>> datapath flows for normal IPv6 traffic doesn't have to match on >>>>>>>> ip6.dst. IPv6 NS packets are generally multicast. A new logical >>>>>>>> match "nd_ns_mcast" for this purpose. >>>>>>>> >>>>>>>> After this patch, We no longer respond to IPv6 NS unicast packets. >>>>>>>> This behavior now matches the IPv4 ARP responder flows. Note >>>>>>>> that after the commit [1] which was recently added we now only >>>>>>>> respond to IPv4 ARP broadcast packets. >>>>>>> >>>>>>> But we still need to reply to unicast NS requests for router ports, right? >>>>>>> Unlike ARP, >>>>>> >>>>>> Actually, ARP has a similar mechanism - Unicast Poll, for the same purpose >>>>>> of validating cache entries without fully removing them. See: >>>>>> https://datatracker.ietf.org/doc/html/rfc1122#section-2 >>>>>> >>>>>> So, we should be responding for unicast ARPs for router ports as well. >>>>>> >>>>> >>>>> Both for ARP request and NS we only proxy in the switch pipeline. It's >>>>> perfectly fine to not reply in the OVN pipeline and let packets reach >>>>> the VIFs. The VIFs will reply to the unicast requests. This is >>>>> transparent for the requester. >>>> >>>> I didn't ask about VIFs, my question is about router port IPs, that are >>>> only known to OVN., i.e. there is no other entity that would reply. >>>> >>> >>> This patch doesn't touch any of the router port related flows. Router >>> ports still reply to all neighbor solicitations (regardless if they're >>> unicast or multicast). >> >> Could you point me to the code? I'm a little confused, because of >> this code snippet: >> >> build_lswitch_arp_nd_responder_known_ips: >> >> /* >> * Add ARP/ND reply flows if either the >> * - port is up and it doesn't have 'unknown' address defined or it >> * doesn't have the option disable_arp_nd_rsp=true. >> * - port type is router or <--- THIS >> * - port type is localport >> */ >> if (check_lsp_is_up && >> !lsp_is_up(op->nbsp) && !lsp_is_router(op->nbsp) && >> strcmp(op->nbsp->type, "localport")) { >> return; >> } >> >> This suggests that we're replying to ARP/ND requests for router ports >> in the switch pipline right here where we're now not replying to unicast >> requests. >> > > We're proxy-ing for router port owned IPs too to avoid flooding (and > encapsulation) of ARP request/NS packets. But if they're unicast > there's no flooding, so we either: > > - let the VIF reply > - reply in the router pipeline for router owned IPs: all calls to > build_lrouter_arp_flow() and build_lrouter_nd_flow(). OK, I see. Thanks for the pointers! The comment I quoted is still misleading a bit, I think. > >> >>> >>>>> >>>>> I didn't review Numan's patch in detail but the idea behind it is correct. >>>>> >>>>> Regards, >>>>> Dumitru >>>>> >>>>>>> IPv6 will use unicast NS requests for Reachability Confirmation: >>>>>>> >>>>>>> 7.3.1. Reachability Confirmation >>>>>>> >>>>>>> ... >>>>>>> >>>>>>> In some cases (e.g., UDP-based protocols and routers forwarding >>>>>>> packets to hosts), such reachability information may not be readily >>>>>>> available from upper-layer protocols. When no hints are available >>>>>>> and a node is sending packets to a neighbor, the node actively probes >>>>>>> the neighbor using unicast Neighbor Solicitation messages to verify >>>>>>> that the forward path is still working. >>>>>>> >>>>>>> https://datatracker.ietf.org/doc/html/rfc4861#section-7.3.1 >>>>>>> >>>>>>> Or do we reply to those somewhere else? >>>>>>> >>>>>>>> >>>>>>>> A recent patch [2] from Ilya partially addressed the same datapath >>>>>>>> flow explosion issue by matching on eth.mcast6 for MLD packets. >>>>>>>> With this patch, we now completely address the datapath flow >>>>>>>> explosion issue for IPv6 traffic provided all the logical ports >>>>>>>> of a logical switch are not configured with port security. >>>>>>>> >>>>>>>> [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") >>>>>>>> [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD and IGMP.") >>>>>>>> >>>>>>>> Reported-at: https://issues.redhat.com/browse/FDP-728 >>>>>>>> Reported-by: Mike Pattrick <mkp@redhat.com> >>>>>>>> Signed-off-by: Numan Siddique <numans@ovn.org> >>>>>>> >>>>>>> Best regards, Ilya Maximets. >>>>>> >>>>> >>>> >>> >> >
On 8/12/24 06:16, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > IPv6 Neigh Solicitation (NS) responder logical flows match on ip6.dst > field. These flows when translated to datapath flows also match on > ip6.dst, which means a separate datapath flow per destination IP > address. This may cause significant performance issues in some > setups (particularly ovs-dpdk telco deployments). > > This patch addresses this issue by matching on eth.mcast6 so that > datapath flows for normal IPv6 traffic doesn't have to match on > ip6.dst. IPv6 NS packets are generally multicast. A new logical > match "nd_ns_mcast" for this purpose. > > After this patch, We no longer respond to IPv6 NS unicast packets. > This behavior now matches the IPv4 ARP responder flows. Note > that after the commit [1] which was recently added we now only > respond to IPv4 ARP broadcast packets. Hi, Numan. Since we sorted out the question I had in the other thread, here is some code review (I didn't actually review the test). It would be great if you include some of the rationale for the change from commit [1] right in here, so it's easier to understand the whole picture without jumping between commits. > > A recent patch [2] from Ilya partially addressed the same datapath > flow explosion issue by matching on eth.mcast6 for MLD packets. > With this patch, we now completely address the datapath flow > explosion issue for IPv6 traffic provided all the logical ports > of a logical switch are not configured with port security. > > [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") > [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD and IGMP.") > > Reported-at: https://issues.redhat.com/browse/FDP-728 > Reported-by: Mike Pattrick <mkp@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > lib/logical-fields.c | 3 ++ > northd/northd.c | 11 ++-- > tests/ovn-northd.at | 4 +- > tests/ovn.at | 2 +- > tests/system-ovn.at | 126 +++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 138 insertions(+), 8 deletions(-) > > diff --git a/lib/logical-fields.c b/lib/logical-fields.c > index 2c9d3c61bf..f1ba1a7ac1 100644 > --- a/lib/logical-fields.c > +++ b/lib/logical-fields.c > @@ -293,6 +293,9 @@ ovn_init_symtab(struct shash *symtab) > "icmp6.type == {135, 136} && icmp6.code == 0 && ip.ttl == 255"); > expr_symtab_add_predicate(symtab, "nd_ns", > "icmp6.type == 135 && icmp6.code == 0 && ip.ttl == 255"); > + expr_symtab_add_predicate(symtab, "nd_ns_mcast", > + "eth.mcastv6 && icmp6.type == 135 && icmp6.code == 0 && " Should this be an 'ip6.mcast' instead? It should be more correct this way. The expression parser will remove redundancy, if any. > + "ip.ttl == 255"); > expr_symtab_add_predicate(symtab, "nd_na", > "icmp6.type == 136 && icmp6.code == 0 && ip.ttl == 255"); > expr_symtab_add_predicate(symtab, "nd_rs", > diff --git a/northd/northd.c b/northd/northd.c > index 5ad30d854b..c0cda85e36 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -9638,11 +9638,12 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, > * but always respond with the unicast IPv6 address. */ The comment above is talking about matching unicast traffic, which is no longer the case. > for (size_t j = 0; j < op->lsp_addrs[i].n_ipv6_addrs; j++) { > ds_clear(match); > - ds_put_format(match, > - "nd_ns && ip6.dst == {%s, %s} && nd.target == %s", > - op->lsp_addrs[i].ipv6_addrs[j].addr_s, > - op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s, > - op->lsp_addrs[i].ipv6_addrs[j].addr_s); > + ds_put_format( > + match, > + "nd_ns_mcast && ip6.dst == {%s, %s} && nd.target == %s", > + op->lsp_addrs[i].ipv6_addrs[j].addr_s, The combination of 'nd_ns_mcast' and the unicast 'addr_s' is incorrect, AFAIU. Should we only match on 'sn_addr_s' here? > + op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s, > + op->lsp_addrs[i].ipv6_addrs[j].addr_s); > > ds_clear(actions); > ds_put_format(actions, > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index e4c8822656..509cfd2d30 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -9490,9 +9490,9 @@ AT_CAPTURE_FILE([S1flows]) > AT_CHECK([grep -e "ls_in_arp_rsp" S1flows | ovn_strip_lflows], [0], [dnl > table=??(ls_in_arp_rsp ), priority=0 , match=(1), action=(next;) > table=??(ls_in_arp_rsp ), priority=100 , match=(arp.tpa == 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff && inport == "S1-vm1"), action=(next;) > - table=??(ls_in_arp_rsp ), priority=100 , match=(nd_ns && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport == "S1-vm1"), action=(next;) > + table=??(ls_in_arp_rsp ), priority=100 , match=(nd_ns_mcast && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport == "S1-vm1"), action=(next;) > table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa == 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff), action=(eth.dst = eth.src; eth.src = 50:54:00:00:00:10; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 50:54:00:00:00:10; arp.tpa = arp.spa; arp.spa = 192.168.0.10; outport = inport; flags.loopback = 1; output;) > - table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), action=(nd_na { eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10; nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = 1; output; };) > + table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns_mcast && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), action=(nd_na { eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10; nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = 1; output; };) > ]) > > #Set the disable_arp_nd_rsp option and verify the flow > diff --git a/tests/ovn.at b/tests/ovn.at > index a1d689e849..96a89c7d8c 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -14417,7 +14417,7 @@ test_ns_na() { > local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5 > > packet=$(fmt_pkt " > - Ether(dst='ff:ff:ff:ff:ff:ff', src='${src_mac}') / > + Ether(dst='33:33:ff:ff:ff:ff', src='${src_mac}') / > IPv6(src='${src_ip}', dst='ff02::1:ff00:2') / > ICMPv6ND_NS(tgt='${dst_ip}') > ") > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index c54b0f3a5f..980472f4e0 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -13750,3 +13750,129 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > /.*terminating with signal 15.*/d"]) > AT_CLEANUP > ]) > + > +dnl This test sends IPv6 TCP packets between two lports of the same > +dnl logical switch and checks that the datapath flows don't match on > +dnl IPv6 source and destination addresses. > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn -- IPv6 switching - datapath flows]) > +AT_KEYWORDS([IPv6]) > + > +ovn_start > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_BR([br-int]) > + > +dnl Set external-ids in br-int needed for ovn-controller > +check ovs-vsctl \ > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true > + > +dnl Start ovn-controller > +start_daemon ovn-controller > + > +check ovn-nbctl ls-add sw0 > + > +check ovn-nbctl lsp-add sw0 vm0 > +check ovn-nbctl lsp-set-addresses vm0 "f0:00:0f:01:02:03 10.0.0.3 1000::3" > + > +check ovn-nbctl lsp-add sw0 vm1 > +check ovn-nbctl lsp-set-addresses vm1 "f0:00:0f:01:02:04 10.0.0.4 1000::4" > + > +check ovn-nbctl --wait=hv sync > + > +ovn-sbctl dump-flows sw0 > /tmp/c.txt > + > +ADD_NAMESPACES(vm0) > +ADD_VETH(vm0, vm0, br-int, "1000::3/64", "f0:00:0f:01:02:03", "1000::1", "nodad") > + > +ADD_NAMESPACES(vm1) > +ADD_VETH(vm1, vm1, br-int, "1000::4/64", "f0:00:0f:01:02:04", "1000::1", "nodad") > + > +dnl Checks the datapath flows and makes sure that there are no matches to > +dnl IPv6 src or dst fields. > +check_dp_flows() { > + ipv6_src_dst_match=$1 > + dnl Dump the flows. Will be helpful while debugging. > + ovs-appctl dpctl/dump-flows > + > + eth_match1="eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04)" > + eth_match2="eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03)" > + ipv6_src_match="ipv6(src=" > + ipv6_dst_match="ipv6(dst=" > + > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" -c], [0], [dnl > +2 > +]) > + > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" -c], [0], [dnl > +2 > +]) > + > + if [[ "$ipv6_src_dst_match" == "yes" ]]; then > + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04) > + dnl matches on IPv6 src or dst. > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep -e "$ipv6_src_match" -e "$ipv6_dst_match" -c], [0], [dnl > +2 > +]) > + > + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03) > + dnl matches on IPv6 src or dst. > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep -e "$ipv6_src_match" -e "$ipv6_dst_match" -c], [0], [dnl > +2 > +]) > + else > + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04) > + dnl doesn't match on IPv6 src or dst. > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep "$ipv6_src_match"], [1], []) > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep "$ipv6_dst_match"], [1], []) > + > + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03) > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep "$ipv6_src_match"], [1], []) > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep "$ipv6_dst_match"], [1], []) > + fi > +} > + > +NS_CHECK_EXEC([vm0], [ping6 -q -c 3 -i 0.3 -w 2 1000::4 | FORMAT_PING], \ > +[0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > +# Start IPv6 TCP server on vm0. > +NETNS_DAEMONIZE([vm0], [nc -6 -l -k 1000::3 4242], [nc-vm1-1.pid]) > +NS_CHECK_EXEC([vm1], [nc -6 1000::3 4242 -z], [0], [ignore], [ignore]) > + > +check_dp_flows no > + > +# Now configure port security on vm0. dp flows should now match on IPv6 fields. > +check ovn-nbctl lsp-set-port-security vm0 "f0:00:0f:01:02:03 10.0.0.3 1000::3" > +check ovn-nbctl --wait=hv sync > + > +NS_CHECK_EXEC([vm0], [ping6 -q -c 3 -i 0.3 -w 2 1000::4 | FORMAT_PING], \ > +[0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > +# ovs-appctl dpctl/del-flows || : Leftover? > +NS_CHECK_EXEC([vm1], [nc -6 1000::3 4242 -z], [0], [ignore], [ignore]) > +check_dp_flows yes > + > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > +/failed to query port patch-.*/d > +/.*terminating with signal 15.*/d"]) > +AT_CLEANUP > +])
On Mon, Aug 12, 2024 at 9:10 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 8/12/24 06:16, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > IPv6 Neigh Solicitation (NS) responder logical flows match on ip6.dst > > field. These flows when translated to datapath flows also match on > > ip6.dst, which means a separate datapath flow per destination IP > > address. This may cause significant performance issues in some > > setups (particularly ovs-dpdk telco deployments). > > > > This patch addresses this issue by matching on eth.mcast6 so that > > datapath flows for normal IPv6 traffic doesn't have to match on > > ip6.dst. IPv6 NS packets are generally multicast. A new logical > > match "nd_ns_mcast" for this purpose. > > > > After this patch, We no longer respond to IPv6 NS unicast packets. > > This behavior now matches the IPv4 ARP responder flows. Note > > that after the commit [1] which was recently added we now only > > respond to IPv4 ARP broadcast packets. > > Hi, Numan. Since we sorted out the question I had in the other thread, > here is some code review (I didn't actually review the test). > > It would be great if you include some of the rationale for the change > from commit [1] right in here, so it's easier to understand the whole > picture without jumping between commits. > > > > > A recent patch [2] from Ilya partially addressed the same datapath > > flow explosion issue by matching on eth.mcast6 for MLD packets. > > With this patch, we now completely address the datapath flow > > explosion issue for IPv6 traffic provided all the logical ports > > of a logical switch are not configured with port security. > > > > [1] - c48ed1736a58 ("Do not reply on unicast arps for IPv4 targets.") > > [2] - 43c34f2e6676 ("logical-fields: Add missing multicast matches for MLD and IGMP.") > > > > Reported-at: https://issues.redhat.com/browse/FDP-728 > > Reported-by: Mike Pattrick <mkp@redhat.com> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > lib/logical-fields.c | 3 ++ > > northd/northd.c | 11 ++-- > > tests/ovn-northd.at | 4 +- > > tests/ovn.at | 2 +- > > tests/system-ovn.at | 126 +++++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 138 insertions(+), 8 deletions(-) > > > > diff --git a/lib/logical-fields.c b/lib/logical-fields.c > > index 2c9d3c61bf..f1ba1a7ac1 100644 > > --- a/lib/logical-fields.c > > +++ b/lib/logical-fields.c > > @@ -293,6 +293,9 @@ ovn_init_symtab(struct shash *symtab) > > "icmp6.type == {135, 136} && icmp6.code == 0 && ip.ttl == 255"); > > expr_symtab_add_predicate(symtab, "nd_ns", > > "icmp6.type == 135 && icmp6.code == 0 && ip.ttl == 255"); > > + expr_symtab_add_predicate(symtab, "nd_ns_mcast", > > + "eth.mcastv6 && icmp6.type == 135 && icmp6.code == 0 && " > > Should this be an 'ip6.mcast' instead? It should be more correct this way. > The expression parser will remove redundancy, if any. > > > + "ip.ttl == 255"); > > expr_symtab_add_predicate(symtab, "nd_na", > > "icmp6.type == 136 && icmp6.code == 0 && ip.ttl == 255"); > > expr_symtab_add_predicate(symtab, "nd_rs", > > diff --git a/northd/northd.c b/northd/northd.c > > index 5ad30d854b..c0cda85e36 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -9638,11 +9638,12 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, > > * but always respond with the unicast IPv6 address. */ > > The comment above is talking about matching unicast traffic, which is > no longer the case. > > > for (size_t j = 0; j < op->lsp_addrs[i].n_ipv6_addrs; j++) { > > ds_clear(match); > > - ds_put_format(match, > > - "nd_ns && ip6.dst == {%s, %s} && nd.target == %s", > > - op->lsp_addrs[i].ipv6_addrs[j].addr_s, > > - op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s, > > - op->lsp_addrs[i].ipv6_addrs[j].addr_s); > > + ds_put_format( > > + match, > > + "nd_ns_mcast && ip6.dst == {%s, %s} && nd.target == %s", > > + op->lsp_addrs[i].ipv6_addrs[j].addr_s, > > The combination of 'nd_ns_mcast' and the unicast 'addr_s' is incorrect, AFAIU. > Should we only match on 'sn_addr_s' here? > > > + op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s, > > + op->lsp_addrs[i].ipv6_addrs[j].addr_s); > > > > ds_clear(actions); > > ds_put_format(actions, > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index e4c8822656..509cfd2d30 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -9490,9 +9490,9 @@ AT_CAPTURE_FILE([S1flows]) > > AT_CHECK([grep -e "ls_in_arp_rsp" S1flows | ovn_strip_lflows], [0], [dnl > > table=??(ls_in_arp_rsp ), priority=0 , match=(1), action=(next;) > > table=??(ls_in_arp_rsp ), priority=100 , match=(arp.tpa == 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff && inport == "S1-vm1"), action=(next;) > > - table=??(ls_in_arp_rsp ), priority=100 , match=(nd_ns && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport == "S1-vm1"), action=(next;) > > + table=??(ls_in_arp_rsp ), priority=100 , match=(nd_ns_mcast && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport == "S1-vm1"), action=(next;) > > table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa == 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff), action=(eth.dst = eth.src; eth.src = 50:54:00:00:00:10; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 50:54:00:00:00:10; arp.tpa = arp.spa; arp.spa = 192.168.0.10; outport = inport; flags.loopback = 1; output;) > > - table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), action=(nd_na { eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10; nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = 1; output; };) > > + table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns_mcast && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), action=(nd_na { eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10; nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = 1; output; };) > > ]) > > > > #Set the disable_arp_nd_rsp option and verify the flow > > diff --git a/tests/ovn.at b/tests/ovn.at > > index a1d689e849..96a89c7d8c 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -14417,7 +14417,7 @@ test_ns_na() { > > local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5 > > > > packet=$(fmt_pkt " > > - Ether(dst='ff:ff:ff:ff:ff:ff', src='${src_mac}') / > > + Ether(dst='33:33:ff:ff:ff:ff', src='${src_mac}') / > > IPv6(src='${src_ip}', dst='ff02::1:ff00:2') / > > ICMPv6ND_NS(tgt='${dst_ip}') > > ") > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index c54b0f3a5f..980472f4e0 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -13750,3 +13750,129 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > > /.*terminating with signal 15.*/d"]) > > AT_CLEANUP > > ]) > > + > > +dnl This test sends IPv6 TCP packets between two lports of the same > > +dnl logical switch and checks that the datapath flows don't match on > > +dnl IPv6 source and destination addresses. > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([ovn -- IPv6 switching - datapath flows]) > > +AT_KEYWORDS([IPv6]) > > + > > +ovn_start > > +OVS_TRAFFIC_VSWITCHD_START() > > +ADD_BR([br-int]) > > + > > +dnl Set external-ids in br-int needed for ovn-controller > > +check ovs-vsctl \ > > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > > + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > > + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true > > + > > +dnl Start ovn-controller > > +start_daemon ovn-controller > > + > > +check ovn-nbctl ls-add sw0 > > + > > +check ovn-nbctl lsp-add sw0 vm0 > > +check ovn-nbctl lsp-set-addresses vm0 "f0:00:0f:01:02:03 10.0.0.3 1000::3" > > + > > +check ovn-nbctl lsp-add sw0 vm1 > > +check ovn-nbctl lsp-set-addresses vm1 "f0:00:0f:01:02:04 10.0.0.4 1000::4" > > + > > +check ovn-nbctl --wait=hv sync > > + > > +ovn-sbctl dump-flows sw0 > /tmp/c.txt > > + > > +ADD_NAMESPACES(vm0) > > +ADD_VETH(vm0, vm0, br-int, "1000::3/64", "f0:00:0f:01:02:03", "1000::1", "nodad") > > + > > +ADD_NAMESPACES(vm1) > > +ADD_VETH(vm1, vm1, br-int, "1000::4/64", "f0:00:0f:01:02:04", "1000::1", "nodad") > > + > > +dnl Checks the datapath flows and makes sure that there are no matches to > > +dnl IPv6 src or dst fields. > > +check_dp_flows() { > > + ipv6_src_dst_match=$1 > > + dnl Dump the flows. Will be helpful while debugging. > > + ovs-appctl dpctl/dump-flows > > + > > + eth_match1="eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04)" > > + eth_match2="eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03)" > > + ipv6_src_match="ipv6(src=" > > + ipv6_dst_match="ipv6(dst=" > > + > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" -c], [0], [dnl > > +2 > > +]) > > + > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" -c], [0], [dnl > > +2 > > +]) > > + > > + if [[ "$ipv6_src_dst_match" == "yes" ]]; then > > + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04) > > + dnl matches on IPv6 src or dst. > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep -e "$ipv6_src_match" -e "$ipv6_dst_match" -c], [0], [dnl > > +2 > > +]) > > + > > + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03) > > + dnl matches on IPv6 src or dst. > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep -e "$ipv6_src_match" -e "$ipv6_dst_match" -c], [0], [dnl > > +2 > > +]) > > + else > > + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04) > > + dnl doesn't match on IPv6 src or dst. > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep "$ipv6_src_match"], [1], []) > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep "$ipv6_dst_match"], [1], []) > > + > > + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03) > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep "$ipv6_src_match"], [1], []) > > + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep "$ipv6_dst_match"], [1], []) > > + fi > > +} > > + > > +NS_CHECK_EXEC([vm0], [ping6 -q -c 3 -i 0.3 -w 2 1000::4 | FORMAT_PING], \ > > +[0], [dnl > > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > > +]) > > + > > +# Start IPv6 TCP server on vm0. > > +NETNS_DAEMONIZE([vm0], [nc -6 -l -k 1000::3 4242], [nc-vm1-1.pid]) > > +NS_CHECK_EXEC([vm1], [nc -6 1000::3 4242 -z], [0], [ignore], [ignore]) > > + > > +check_dp_flows no > > + > > +# Now configure port security on vm0. dp flows should now match on IPv6 fields. > > +check ovn-nbctl lsp-set-port-security vm0 "f0:00:0f:01:02:03 10.0.0.3 1000::3" > > +check ovn-nbctl --wait=hv sync > > + > > +NS_CHECK_EXEC([vm0], [ping6 -q -c 3 -i 0.3 -w 2 1000::4 | FORMAT_PING], \ > > +[0], [dnl > > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > > +]) > > + > > +# ovs-appctl dpctl/del-flows || : > > Leftover? Thanks for the review comments. I've addressed all of them v2 is submitted here - https://patchwork.ozlabs.org/project/ovn/patch/20240815030257.1091794-1-numans@ovn.org/ Numan > > > +NS_CHECK_EXEC([vm1], [nc -6 1000::3 4242 -z], [0], [ignore], [ignore]) > > +check_dp_flows yes > > + > > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > > + > > +as ovn-sb > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > + > > +as ovn-nb > > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > > + > > +as northd > > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > > + > > +as > > +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > > +/failed to query port patch-.*/d > > +/.*terminating with signal 15.*/d"]) > > +AT_CLEANUP > > +]) > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/logical-fields.c b/lib/logical-fields.c index 2c9d3c61bf..f1ba1a7ac1 100644 --- a/lib/logical-fields.c +++ b/lib/logical-fields.c @@ -293,6 +293,9 @@ ovn_init_symtab(struct shash *symtab) "icmp6.type == {135, 136} && icmp6.code == 0 && ip.ttl == 255"); expr_symtab_add_predicate(symtab, "nd_ns", "icmp6.type == 135 && icmp6.code == 0 && ip.ttl == 255"); + expr_symtab_add_predicate(symtab, "nd_ns_mcast", + "eth.mcastv6 && icmp6.type == 135 && icmp6.code == 0 && " + "ip.ttl == 255"); expr_symtab_add_predicate(symtab, "nd_na", "icmp6.type == 136 && icmp6.code == 0 && ip.ttl == 255"); expr_symtab_add_predicate(symtab, "nd_rs", diff --git a/northd/northd.c b/northd/northd.c index 5ad30d854b..c0cda85e36 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -9638,11 +9638,12 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, * but always respond with the unicast IPv6 address. */ for (size_t j = 0; j < op->lsp_addrs[i].n_ipv6_addrs; j++) { ds_clear(match); - ds_put_format(match, - "nd_ns && ip6.dst == {%s, %s} && nd.target == %s", - op->lsp_addrs[i].ipv6_addrs[j].addr_s, - op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s, - op->lsp_addrs[i].ipv6_addrs[j].addr_s); + ds_put_format( + match, + "nd_ns_mcast && ip6.dst == {%s, %s} && nd.target == %s", + op->lsp_addrs[i].ipv6_addrs[j].addr_s, + op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s, + op->lsp_addrs[i].ipv6_addrs[j].addr_s); ds_clear(actions); ds_put_format(actions, diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index e4c8822656..509cfd2d30 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -9490,9 +9490,9 @@ AT_CAPTURE_FILE([S1flows]) AT_CHECK([grep -e "ls_in_arp_rsp" S1flows | ovn_strip_lflows], [0], [dnl table=??(ls_in_arp_rsp ), priority=0 , match=(1), action=(next;) table=??(ls_in_arp_rsp ), priority=100 , match=(arp.tpa == 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff && inport == "S1-vm1"), action=(next;) - table=??(ls_in_arp_rsp ), priority=100 , match=(nd_ns && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport == "S1-vm1"), action=(next;) + table=??(ls_in_arp_rsp ), priority=100 , match=(nd_ns_mcast && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport == "S1-vm1"), action=(next;) table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa == 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff), action=(eth.dst = eth.src; eth.src = 50:54:00:00:00:10; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 50:54:00:00:00:10; arp.tpa = arp.spa; arp.spa = 192.168.0.10; outport = inport; flags.loopback = 1; output;) - table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), action=(nd_na { eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10; nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = 1; output; };) + table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns_mcast && ip6.dst == {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), action=(nd_na { eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10; nd.tll = 50:54:00:00:00:10; outport = inport; flags.loopback = 1; output; };) ]) #Set the disable_arp_nd_rsp option and verify the flow diff --git a/tests/ovn.at b/tests/ovn.at index a1d689e849..96a89c7d8c 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -14417,7 +14417,7 @@ test_ns_na() { local inport=$1 src_mac=$2 dst_mac=$3 src_ip=$4 dst_ip=$5 packet=$(fmt_pkt " - Ether(dst='ff:ff:ff:ff:ff:ff', src='${src_mac}') / + Ether(dst='33:33:ff:ff:ff:ff', src='${src_mac}') / IPv6(src='${src_ip}', dst='ff02::1:ff00:2') / ICMPv6ND_NS(tgt='${dst_ip}') ") diff --git a/tests/system-ovn.at b/tests/system-ovn.at index c54b0f3a5f..980472f4e0 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -13750,3 +13750,129 @@ OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d /.*terminating with signal 15.*/d"]) AT_CLEANUP ]) + +dnl This test sends IPv6 TCP packets between two lports of the same +dnl logical switch and checks that the datapath flows don't match on +dnl IPv6 source and destination addresses. +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn -- IPv6 switching - datapath flows]) +AT_KEYWORDS([IPv6]) + +ovn_start +OVS_TRAFFIC_VSWITCHD_START() +ADD_BR([br-int]) + +dnl Set external-ids in br-int needed for ovn-controller +check ovs-vsctl \ + -- set Open_vSwitch . external-ids:system-id=hv1 \ + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true + +dnl Start ovn-controller +start_daemon ovn-controller + +check ovn-nbctl ls-add sw0 + +check ovn-nbctl lsp-add sw0 vm0 +check ovn-nbctl lsp-set-addresses vm0 "f0:00:0f:01:02:03 10.0.0.3 1000::3" + +check ovn-nbctl lsp-add sw0 vm1 +check ovn-nbctl lsp-set-addresses vm1 "f0:00:0f:01:02:04 10.0.0.4 1000::4" + +check ovn-nbctl --wait=hv sync + +ovn-sbctl dump-flows sw0 > /tmp/c.txt + +ADD_NAMESPACES(vm0) +ADD_VETH(vm0, vm0, br-int, "1000::3/64", "f0:00:0f:01:02:03", "1000::1", "nodad") + +ADD_NAMESPACES(vm1) +ADD_VETH(vm1, vm1, br-int, "1000::4/64", "f0:00:0f:01:02:04", "1000::1", "nodad") + +dnl Checks the datapath flows and makes sure that there are no matches to +dnl IPv6 src or dst fields. +check_dp_flows() { + ipv6_src_dst_match=$1 + dnl Dump the flows. Will be helpful while debugging. + ovs-appctl dpctl/dump-flows + + eth_match1="eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04)" + eth_match2="eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03)" + ipv6_src_match="ipv6(src=" + ipv6_dst_match="ipv6(dst=" + + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" -c], [0], [dnl +2 +]) + + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" -c], [0], [dnl +2 +]) + + if [[ "$ipv6_src_dst_match" == "yes" ]]; then + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04) + dnl matches on IPv6 src or dst. + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep -e "$ipv6_src_match" -e "$ipv6_dst_match" -c], [0], [dnl +2 +]) + + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03) + dnl matches on IPv6 src or dst. + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep -e "$ipv6_src_match" -e "$ipv6_dst_match" -c], [0], [dnl +2 +]) + else + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:03,dst=f0:00:0f:01:02:04) + dnl doesn't match on IPv6 src or dst. + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep "$ipv6_src_match"], [1], []) + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match1" | grep "$ipv6_dst_match"], [1], []) + + dnl Check that the dp flow for eth(src=f0:00:0f:01:02:04,dst=f0:00:0f:01:02:03) + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep "$ipv6_src_match"], [1], []) + AT_CHECK([ovs-appctl dpctl/dump-flows | grep "$eth_match2" | grep "$ipv6_dst_match"], [1], []) + fi +} + +NS_CHECK_EXEC([vm0], [ping6 -q -c 3 -i 0.3 -w 2 1000::4 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + +# Start IPv6 TCP server on vm0. +NETNS_DAEMONIZE([vm0], [nc -6 -l -k 1000::3 4242], [nc-vm1-1.pid]) +NS_CHECK_EXEC([vm1], [nc -6 1000::3 4242 -z], [0], [ignore], [ignore]) + +check_dp_flows no + +# Now configure port security on vm0. dp flows should now match on IPv6 fields. +check ovn-nbctl lsp-set-port-security vm0 "f0:00:0f:01:02:03 10.0.0.3 1000::3" +check ovn-nbctl --wait=hv sync + +NS_CHECK_EXEC([vm0], [ping6 -q -c 3 -i 0.3 -w 2 1000::4 | FORMAT_PING], \ +[0], [dnl +3 packets transmitted, 3 received, 0% packet loss, time 0ms +]) + +# ovs-appctl dpctl/del-flows || : +NS_CHECK_EXEC([vm1], [nc -6 1000::3 4242 -z], [0], [ignore], [ignore]) +check_dp_flows yes + +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([ovn-northd]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d +/failed to query port patch-.*/d +/.*terminating with signal 15.*/d"]) +AT_CLEANUP +])