diff mbox series

[ovs-dev] ovn-northd: Add flow to use eth.src if nd.tll is 0 in put_nd() action.

Message ID 20220423181452.3698721-1-numans@ovn.org
State Accepted
Headers show
Series [ovs-dev] ovn-northd: Add flow to use eth.src if nd.tll is 0 in put_nd() action. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Numan Siddique April 23, 2022, 6:14 p.m. UTC 
From: Numan Siddique <numans@ovn.org>

Presently OVN assumes that an IPv6 Neigh Adv packet will have the
target link layer option preset.  But that's not the case all the time.
This field is optional and as per rfc4861 (quoted below).

Target link-layer address
                     The link-layer address for the target, i.e., the
                     sender of the advertisement.  This option MUST be
                     included on link layers that have addresses when
                     responding to multicast solicitations.  When
                     responding to a unicast Neighbor Solicitation this
                     option SHOULD be included.

                     The option MUST be included for multicast
                     solicitations in order to avoid infinite Neighbor
                     Solicitation "recursion" when the peer node does
                     not have a cache entry to return a Neighbor
                     Advertisements message.  When responding to unicast
                     solicitations, the option can be omitted since the
                     sender of the solicitation has the correct link-
                     layer address; otherwise, it would not be able to
                     send the unicast solicitation in the first place.
                     However, including the link-layer address in this
                     case adds little overhead and eliminates a
                     potential race condition where the sender deletes
                     the cached link-layer address prior to receiving a
                     response to a previous solicitation.

If target link layer option is not present, then ovn-controller learns
the mac binding with 00:00:00:00:00:00 address which is not correct.

This patch fixes the issue by adding the below logical flow in router
pipeline:

table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na && nd.tll == 0),
         action=(put_nd(inport, nd.target, eth.src); next;)

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2078026
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/northd.c         |  6 ++++++
 northd/ovn-northd.8.xml |  6 ++++++
 tests/ovn-northd.at     | 23 +++++++++++++++++++++++
 3 files changed, 35 insertions(+)

Comments

Dumitru Ceara April 25, 2022, 12:58 p.m. UTC | #1 
On 4/23/22 20:14, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> Presently OVN assumes that an IPv6 Neigh Adv packet will have the
> target link layer option preset.  But that's not the case all the time.
> This field is optional and as per rfc4861 (quoted below).
> 
> Target link-layer address
>                      The link-layer address for the target, i.e., the
>                      sender of the advertisement.  This option MUST be
>                      included on link layers that have addresses when
>                      responding to multicast solicitations.  When
>                      responding to a unicast Neighbor Solicitation this
>                      option SHOULD be included.
> 
>                      The option MUST be included for multicast
>                      solicitations in order to avoid infinite Neighbor
>                      Solicitation "recursion" when the peer node does
>                      not have a cache entry to return a Neighbor
>                      Advertisements message.  When responding to unicast
>                      solicitations, the option can be omitted since the
>                      sender of the solicitation has the correct link-
>                      layer address; otherwise, it would not be able to
>                      send the unicast solicitation in the first place.
>                      However, including the link-layer address in this
>                      case adds little overhead and eliminates a
>                      potential race condition where the sender deletes
>                      the cached link-layer address prior to receiving a
>                      response to a previous solicitation.
> 
> If target link layer option is not present, then ovn-controller learns
> the mac binding with 00:00:00:00:00:00 address which is not correct.
> 
> This patch fixes the issue by adding the below logical flow in router
> pipeline:
> 
> table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na && nd.tll == 0),
>          action=(put_nd(inport, nd.target, eth.src); next;)
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2078026
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---

Hi Numan,

Great catch!

>  northd/northd.c         |  6 ++++++
>  northd/ovn-northd.8.xml |  6 ++++++
>  tests/ovn-northd.at     | 23 +++++++++++++++++++++++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/northd/northd.c b/northd/northd.c
> index bc195146d0..8c4187eae1 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -10943,6 +10943,12 @@ build_neigh_learning_flows_for_lrouter(
>                            copp_meter_get(COPP_ARP, od->nbr->copp,
>                                           meter_groups));
>  
> +        ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
> +                          "nd_na && nd.tll == 0",
> +                          "put_nd(inport, nd.target, eth.src); next;",
> +                          copp_meter_get(COPP_ND_NA, od->nbr->copp,
> +                                         meter_groups));
> +

I don't think it's a huge problem but while we're at it, rfc 4861
"Neighbor Solicitation Message Format" specifies that:

      Source link-layer address
                     The link-layer address for the sender.  MUST NOT be
                     included when the source IP address is the
                     unspecified address.  Otherwise, on link layers
                     that have addresses this option MUST be included in
                     multicast solicitations and SHOULD be included in
                     unicast solicitations.

Should we add a flow to skip put_nd() when ip6.src is ::?

Maybe it's good to skip nd.sll == 0 too, so something like:

ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
              "nd_ns && (ip6.src == 0 || nd.sll == 0)",
              "next;");

What do you think?

>          ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
>                            "nd_na", "put_nd(inport, nd.target, nd.tll); next;",
>                            copp_meter_get(COPP_ND_NA, od->nbr->copp,
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index db4f4d267c..d95e9c3fd2 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2307,6 +2307,12 @@ next;
>          <code>put_arp(inport, arp.spa, arp.sha); next;</code>
>        </li>
>  
> +      <li>
> +        A priority-95 flow with the match <code>nd_na  &amp;&amp;
> +        nd.tll == 0</code> and applies the action
> +        <code>put_nd(inport, nd.target, eth.src); next;</code>
> +      </li>
> +
>        <li>
>          A priority-90 flow with the match <code>nd_na</code> and
>          applies the action
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index adb3043853..163287edfa 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -6664,3 +6664,26 @@ ovn-nbctl --may-exist static-mac-binding-add lr0-p0 192.168.10.100 00:00:22:33:5
>  wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66"
>  
>  AT_CLEANUP
> +
> +AT_SETUP([LR neighbor lookup and learning flows])

This should be wrapped in "OVN_FOR_EACH_NORTHD()".  This actually
applies to the "LR NB Static_MAC_Binding table" test too but that's
not introduced by your patch.

> +ovn_start
> +
> +# Create logical routers
> +ovn-nbctl --wait=sb lr-add lr0
> +
> +ovn-sbctl dump-flows lr0 > lrflows
> +AT_CAPTURE_FILE([lrflows])
> +
> +AT_CHECK([cat lrflows | grep -e lr_in_lookup_neighbor -e lr_in_learn_neighbor | sort], [0], [dnl
> +  table=1 (lr_in_lookup_neighbor), priority=0    , match=(1), action=(reg9[[2]] = 1; next;)
> +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(arp.op == 2), action=(reg9[[2]] = lookup_arp(inport, arp.spa, arp.sha); next;)
> +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(nd_na), action=(reg9[[2]] = lookup_nd(inport, nd.target, nd.tll); next;)
> +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(nd_ns), action=(reg9[[2]] = lookup_nd(inport, ip6.src, nd.sll); next;)
> +  table=2 (lr_in_learn_neighbor), priority=100  , match=(reg9[[2]] == 1), action=(next;)
> +  table=2 (lr_in_learn_neighbor), priority=90   , match=(arp), action=(put_arp(inport, arp.spa, arp.sha); next;)
> +  table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_na), action=(put_nd(inport, nd.target, nd.tll); next;)
> +  table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_ns), action=(put_nd(inport, ip6.src, nd.sll); next;)
> +  table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na && nd.tll == 0), action=(put_nd(inport, nd.target, eth.src); next;)
> +])
> +
> +AT_CLEANUP

Regards,
Dumitru
Numan Siddique April 25, 2022, 2:06 p.m. UTC | #2 
On Mon, Apr 25, 2022, 8:59 AM Dumitru Ceara <dceara@redhat.com> wrote:

> On 4/23/22 20:14, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > Presently OVN assumes that an IPv6 Neigh Adv packet will have the
> > target link layer option preset.  But that's not the case all the time.
> > This field is optional and as per rfc4861 (quoted below).
> >
> > Target link-layer address
> >                      The link-layer address for the target, i.e., the
> >                      sender of the advertisement.  This option MUST be
> >                      included on link layers that have addresses when
> >                      responding to multicast solicitations.  When
> >                      responding to a unicast Neighbor Solicitation this
> >                      option SHOULD be included.
> >
> >                      The option MUST be included for multicast
> >                      solicitations in order to avoid infinite Neighbor
> >                      Solicitation "recursion" when the peer node does
> >                      not have a cache entry to return a Neighbor
> >                      Advertisements message.  When responding to unicast
> >                      solicitations, the option can be omitted since the
> >                      sender of the solicitation has the correct link-
> >                      layer address; otherwise, it would not be able to
> >                      send the unicast solicitation in the first place.
> >                      However, including the link-layer address in this
> >                      case adds little overhead and eliminates a
> >                      potential race condition where the sender deletes
> >                      the cached link-layer address prior to receiving a
> >                      response to a previous solicitation.
> >
> > If target link layer option is not present, then ovn-controller learns
> > the mac binding with 00:00:00:00:00:00 address which is not correct.
> >
> > This patch fixes the issue by adding the below logical flow in router
> > pipeline:
> >
> > table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na && nd.tll
> == 0),
> >          action=(put_nd(inport, nd.target, eth.src); next;)
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2078026
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
>
> Hi Numan,
>
> Great catch!
>
> >  northd/northd.c         |  6 ++++++
> >  northd/ovn-northd.8.xml |  6 ++++++
> >  tests/ovn-northd.at     | 23 +++++++++++++++++++++++
> >  3 files changed, 35 insertions(+)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index bc195146d0..8c4187eae1 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -10943,6 +10943,12 @@ build_neigh_learning_flows_for_lrouter(
> >                            copp_meter_get(COPP_ARP, od->nbr->copp,
> >                                           meter_groups));
> >
> > +        ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
> > +                          "nd_na && nd.tll == 0",
> > +                          "put_nd(inport, nd.target, eth.src); next;",
> > +                          copp_meter_get(COPP_ND_NA, od->nbr->copp,
> > +                                         meter_groups));
> > +
>
> I don't think it's a huge problem but while we're at it, rfc 4861
> "Neighbor Solicitation Message Format" specifies that:
>
>       Source link-layer address
>                      The link-layer address for the sender.  MUST NOT be
>                      included when the source IP address is the
>                      unspecified address.  Otherwise, on link layers
>                      that have addresses this option MUST be included in
>                      multicast solicitations and SHOULD be included in
>                      unicast solicitations.
>
> Should we add a flow to skip put_nd() when ip6.src is ::?
>
> Maybe it's good to skip nd.sll == 0 too, so something like:
>
> ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
>               "nd_ns && (ip6.src == 0 || nd.sll == 0)",
>               "next;");
>
> What do you think?
>

Hi Dumitru,

Thanks for the review. Agree with you.

Does a follow up patch sounds good to you ?



> >          ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
> >                            "nd_na", "put_nd(inport, nd.target, nd.tll);
> next;",
> >                            copp_meter_get(COPP_ND_NA, od->nbr->copp,
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index db4f4d267c..d95e9c3fd2 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -2307,6 +2307,12 @@ next;
> >          <code>put_arp(inport, arp.spa, arp.sha); next;</code>
> >        </li>
> >
> > +      <li>
> > +        A priority-95 flow with the match <code>nd_na  &amp;&amp;
> > +        nd.tll == 0</code> and applies the action
> > +        <code>put_nd(inport, nd.target, eth.src); next;</code>
> > +      </li>
> > +
> >        <li>
> >          A priority-90 flow with the match <code>nd_na</code> and
> >          applies the action
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index adb3043853..163287edfa 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -6664,3 +6664,26 @@ ovn-nbctl --may-exist static-mac-binding-add
> lr0-p0 192.168.10.100 00:00:22:33:5
> >  wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0
> ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66"
> >
> >  AT_CLEANUP
> > +
> > +AT_SETUP([LR neighbor lookup and learning flows])
>
> This should be wrapped in "OVN_FOR_EACH_NORTHD()".


Ack


This actually
> applies to the "LR NB Static_MAC_Binding table" test too but that's
> not introduced by your patch.
>

I guess this can be a follow up patch too.

Numan


> > +ovn_start
> > +
> > +# Create logical routers
> > +ovn-nbctl --wait=sb lr-add lr0
> > +
> > +ovn-sbctl dump-flows lr0 > lrflows
> > +AT_CAPTURE_FILE([lrflows])
> > +
> > +AT_CHECK([cat lrflows | grep -e lr_in_lookup_neighbor -e
> lr_in_learn_neighbor | sort], [0], [dnl
> > +  table=1 (lr_in_lookup_neighbor), priority=0    , match=(1),
> action=(reg9[[2]] = 1; next;)
> > +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(arp.op == 2),
> action=(reg9[[2]] = lookup_arp(inport, arp.spa, arp.sha); next;)
> > +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(nd_na),
> action=(reg9[[2]] = lookup_nd(inport, nd.target, nd.tll); next;)
> > +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(nd_ns),
> action=(reg9[[2]] = lookup_nd(inport, ip6.src, nd.sll); next;)
> > +  table=2 (lr_in_learn_neighbor), priority=100  , match=(reg9[[2]] ==
> 1), action=(next;)
> > +  table=2 (lr_in_learn_neighbor), priority=90   , match=(arp),
> action=(put_arp(inport, arp.spa, arp.sha); next;)
> > +  table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_na),
> action=(put_nd(inport, nd.target, nd.tll); next;)
> > +  table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_ns),
> action=(put_nd(inport, ip6.src, nd.sll); next;)
> > +  table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na &&
> nd.tll == 0), action=(put_nd(inport, nd.target, eth.src); next;)
> > +])
> > +
> > +AT_CLEANUP
>
> Regards,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Dumitru Ceara April 25, 2022, 2:20 p.m. UTC | #3 
On 4/25/22 16:06, Numan Siddique wrote:
> On Mon, Apr 25, 2022, 8:59 AM Dumitru Ceara <dceara@redhat.com> wrote:
> 
>> On 4/23/22 20:14, numans@ovn.org wrote:
>>> From: Numan Siddique <numans@ovn.org>
>>>
>>> Presently OVN assumes that an IPv6 Neigh Adv packet will have the
>>> target link layer option preset.  But that's not the case all the time.
>>> This field is optional and as per rfc4861 (quoted below).
>>>
>>> Target link-layer address
>>>                      The link-layer address for the target, i.e., the
>>>                      sender of the advertisement.  This option MUST be
>>>                      included on link layers that have addresses when
>>>                      responding to multicast solicitations.  When
>>>                      responding to a unicast Neighbor Solicitation this
>>>                      option SHOULD be included.
>>>
>>>                      The option MUST be included for multicast
>>>                      solicitations in order to avoid infinite Neighbor
>>>                      Solicitation "recursion" when the peer node does
>>>                      not have a cache entry to return a Neighbor
>>>                      Advertisements message.  When responding to unicast
>>>                      solicitations, the option can be omitted since the
>>>                      sender of the solicitation has the correct link-
>>>                      layer address; otherwise, it would not be able to
>>>                      send the unicast solicitation in the first place.
>>>                      However, including the link-layer address in this
>>>                      case adds little overhead and eliminates a
>>>                      potential race condition where the sender deletes
>>>                      the cached link-layer address prior to receiving a
>>>                      response to a previous solicitation.
>>>
>>> If target link layer option is not present, then ovn-controller learns
>>> the mac binding with 00:00:00:00:00:00 address which is not correct.
>>>
>>> This patch fixes the issue by adding the below logical flow in router
>>> pipeline:
>>>
>>> table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na && nd.tll
>> == 0),
>>>          action=(put_nd(inport, nd.target, eth.src); next;)
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2078026
>>> Signed-off-by: Numan Siddique <numans@ovn.org>
>>> ---
>>
>> Hi Numan,
>>
>> Great catch!
>>
>>>  northd/northd.c         |  6 ++++++
>>>  northd/ovn-northd.8.xml |  6 ++++++
>>>  tests/ovn-northd.at     | 23 +++++++++++++++++++++++
>>>  3 files changed, 35 insertions(+)
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index bc195146d0..8c4187eae1 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -10943,6 +10943,12 @@ build_neigh_learning_flows_for_lrouter(
>>>                            copp_meter_get(COPP_ARP, od->nbr->copp,
>>>                                           meter_groups));
>>>
>>> +        ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
>>> +                          "nd_na && nd.tll == 0",
>>> +                          "put_nd(inport, nd.target, eth.src); next;",
>>> +                          copp_meter_get(COPP_ND_NA, od->nbr->copp,
>>> +                                         meter_groups));
>>> +
>>
>> I don't think it's a huge problem but while we're at it, rfc 4861
>> "Neighbor Solicitation Message Format" specifies that:
>>
>>       Source link-layer address
>>                      The link-layer address for the sender.  MUST NOT be
>>                      included when the source IP address is the
>>                      unspecified address.  Otherwise, on link layers
>>                      that have addresses this option MUST be included in
>>                      multicast solicitations and SHOULD be included in
>>                      unicast solicitations.
>>
>> Should we add a flow to skip put_nd() when ip6.src is ::?
>>
>> Maybe it's good to skip nd.sll == 0 too, so something like:
>>
>> ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
>>               "nd_ns && (ip6.src == 0 || nd.sll == 0)",
>>               "next;");
>>
>> What do you think?
>>
> 
> Hi Dumitru,
> 
> Thanks for the review. Agree with you.
> 
> Does a follow up patch sounds good to you ?
> 
> 

Sure.

> 
>>>          ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
>>>                            "nd_na", "put_nd(inport, nd.target, nd.tll);
>> next;",
>>>                            copp_meter_get(COPP_ND_NA, od->nbr->copp,
>>> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
>>> index db4f4d267c..d95e9c3fd2 100644
>>> --- a/northd/ovn-northd.8.xml
>>> +++ b/northd/ovn-northd.8.xml
>>> @@ -2307,6 +2307,12 @@ next;
>>>          <code>put_arp(inport, arp.spa, arp.sha); next;</code>
>>>        </li>
>>>
>>> +      <li>
>>> +        A priority-95 flow with the match <code>nd_na  &amp;&amp;
>>> +        nd.tll == 0</code> and applies the action
>>> +        <code>put_nd(inport, nd.target, eth.src); next;</code>
>>> +      </li>
>>> +
>>>        <li>
>>>          A priority-90 flow with the match <code>nd_na</code> and
>>>          applies the action
>>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
>>> index adb3043853..163287edfa 100644
>>> --- a/tests/ovn-northd.at
>>> +++ b/tests/ovn-northd.at
>>> @@ -6664,3 +6664,26 @@ ovn-nbctl --may-exist static-mac-binding-add
>> lr0-p0 192.168.10.100 00:00:22:33:5
>>>  wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0
>> ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66"
>>>
>>>  AT_CLEANUP
>>> +
>>> +AT_SETUP([LR neighbor lookup and learning flows])
>>
>> This should be wrapped in "OVN_FOR_EACH_NORTHD()".
> 
> 
> Ack
> 
> 
> This actually
>> applies to the "LR NB Static_MAC_Binding table" test too but that's
>> not introduced by your patch.
>>
> 
> I guess this can be a follow up patch too.
> 

Sounds good.

So if you could please wrap the new test case you added with
OVN_FOR_EACH_NORTHD() then:

Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks,
Dumitru

> Numan
> 
> 
>>> +ovn_start
>>> +
>>> +# Create logical routers
>>> +ovn-nbctl --wait=sb lr-add lr0
>>> +
>>> +ovn-sbctl dump-flows lr0 > lrflows
>>> +AT_CAPTURE_FILE([lrflows])
>>> +
>>> +AT_CHECK([cat lrflows | grep -e lr_in_lookup_neighbor -e
>> lr_in_learn_neighbor | sort], [0], [dnl
>>> +  table=1 (lr_in_lookup_neighbor), priority=0    , match=(1),
>> action=(reg9[[2]] = 1; next;)
>>> +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(arp.op == 2),
>> action=(reg9[[2]] = lookup_arp(inport, arp.spa, arp.sha); next;)
>>> +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(nd_na),
>> action=(reg9[[2]] = lookup_nd(inport, nd.target, nd.tll); next;)
>>> +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(nd_ns),
>> action=(reg9[[2]] = lookup_nd(inport, ip6.src, nd.sll); next;)
>>> +  table=2 (lr_in_learn_neighbor), priority=100  , match=(reg9[[2]] ==
>> 1), action=(next;)
>>> +  table=2 (lr_in_learn_neighbor), priority=90   , match=(arp),
>> action=(put_arp(inport, arp.spa, arp.sha); next;)
>>> +  table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_na),
>> action=(put_nd(inport, nd.target, nd.tll); next;)
>>> +  table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_ns),
>> action=(put_nd(inport, ip6.src, nd.sll); next;)
>>> +  table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na &&
>> nd.tll == 0), action=(put_nd(inport, nd.target, eth.src); next;)
>>> +])
>>> +
>>> +AT_CLEANUP
>>
>> Regards,
>> Dumitru
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
>
Numan Siddique April 25, 2022, 4:14 p.m. UTC | #4 
On Mon, Apr 25, 2022 at 10:20 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 4/25/22 16:06, Numan Siddique wrote:
> > On Mon, Apr 25, 2022, 8:59 AM Dumitru Ceara <dceara@redhat.com> wrote:
> >
> >> On 4/23/22 20:14, numans@ovn.org wrote:
> >>> From: Numan Siddique <numans@ovn.org>
> >>>
> >>> Presently OVN assumes that an IPv6 Neigh Adv packet will have the
> >>> target link layer option preset.  But that's not the case all the time.
> >>> This field is optional and as per rfc4861 (quoted below).
> >>>
> >>> Target link-layer address
> >>>                      The link-layer address for the target, i.e., the
> >>>                      sender of the advertisement.  This option MUST be
> >>>                      included on link layers that have addresses when
> >>>                      responding to multicast solicitations.  When
> >>>                      responding to a unicast Neighbor Solicitation this
> >>>                      option SHOULD be included.
> >>>
> >>>                      The option MUST be included for multicast
> >>>                      solicitations in order to avoid infinite Neighbor
> >>>                      Solicitation "recursion" when the peer node does
> >>>                      not have a cache entry to return a Neighbor
> >>>                      Advertisements message.  When responding to unicast
> >>>                      solicitations, the option can be omitted since the
> >>>                      sender of the solicitation has the correct link-
> >>>                      layer address; otherwise, it would not be able to
> >>>                      send the unicast solicitation in the first place.
> >>>                      However, including the link-layer address in this
> >>>                      case adds little overhead and eliminates a
> >>>                      potential race condition where the sender deletes
> >>>                      the cached link-layer address prior to receiving a
> >>>                      response to a previous solicitation.
> >>>
> >>> If target link layer option is not present, then ovn-controller learns
> >>> the mac binding with 00:00:00:00:00:00 address which is not correct.
> >>>
> >>> This patch fixes the issue by adding the below logical flow in router
> >>> pipeline:
> >>>
> >>> table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na && nd.tll
> >> == 0),
> >>>          action=(put_nd(inport, nd.target, eth.src); next;)
> >>>
> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2078026
> >>> Signed-off-by: Numan Siddique <numans@ovn.org>
> >>> ---
> >>
> >> Hi Numan,
> >>
> >> Great catch!
> >>
> >>>  northd/northd.c         |  6 ++++++
> >>>  northd/ovn-northd.8.xml |  6 ++++++
> >>>  tests/ovn-northd.at     | 23 +++++++++++++++++++++++
> >>>  3 files changed, 35 insertions(+)
> >>>
> >>> diff --git a/northd/northd.c b/northd/northd.c
> >>> index bc195146d0..8c4187eae1 100644
> >>> --- a/northd/northd.c
> >>> +++ b/northd/northd.c
> >>> @@ -10943,6 +10943,12 @@ build_neigh_learning_flows_for_lrouter(
> >>>                            copp_meter_get(COPP_ARP, od->nbr->copp,
> >>>                                           meter_groups));
> >>>
> >>> +        ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
> >>> +                          "nd_na && nd.tll == 0",
> >>> +                          "put_nd(inport, nd.target, eth.src); next;",
> >>> +                          copp_meter_get(COPP_ND_NA, od->nbr->copp,
> >>> +                                         meter_groups));
> >>> +
> >>
> >> I don't think it's a huge problem but while we're at it, rfc 4861
> >> "Neighbor Solicitation Message Format" specifies that:
> >>
> >>       Source link-layer address
> >>                      The link-layer address for the sender.  MUST NOT be
> >>                      included when the source IP address is the
> >>                      unspecified address.  Otherwise, on link layers
> >>                      that have addresses this option MUST be included in
> >>                      multicast solicitations and SHOULD be included in
> >>                      unicast solicitations.
> >>
> >> Should we add a flow to skip put_nd() when ip6.src is ::?
> >>
> >> Maybe it's good to skip nd.sll == 0 too, so something like:
> >>
> >> ovn_lflow_add(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
> >>               "nd_ns && (ip6.src == 0 || nd.sll == 0)",
> >>               "next;");
> >>
> >> What do you think?
> >>
> >
> > Hi Dumitru,
> >
> > Thanks for the review. Agree with you.
> >
> > Does a follow up patch sounds good to you ?
> >
> >
>
> Sure.
>
> >
> >>>          ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
> >>>                            "nd_na", "put_nd(inport, nd.target, nd.tll);
> >> next;",
> >>>                            copp_meter_get(COPP_ND_NA, od->nbr->copp,
> >>> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> >>> index db4f4d267c..d95e9c3fd2 100644
> >>> --- a/northd/ovn-northd.8.xml
> >>> +++ b/northd/ovn-northd.8.xml
> >>> @@ -2307,6 +2307,12 @@ next;
> >>>          <code>put_arp(inport, arp.spa, arp.sha); next;</code>
> >>>        </li>
> >>>
> >>> +      <li>
> >>> +        A priority-95 flow with the match <code>nd_na  &amp;&amp;
> >>> +        nd.tll == 0</code> and applies the action
> >>> +        <code>put_nd(inport, nd.target, eth.src); next;</code>
> >>> +      </li>
> >>> +
> >>>        <li>
> >>>          A priority-90 flow with the match <code>nd_na</code> and
> >>>          applies the action
> >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> >>> index adb3043853..163287edfa 100644
> >>> --- a/tests/ovn-northd.at
> >>> +++ b/tests/ovn-northd.at
> >>> @@ -6664,3 +6664,26 @@ ovn-nbctl --may-exist static-mac-binding-add
> >> lr0-p0 192.168.10.100 00:00:22:33:5
> >>>  wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0
> >> ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66"
> >>>
> >>>  AT_CLEANUP
> >>> +
> >>> +AT_SETUP([LR neighbor lookup and learning flows])
> >>
> >> This should be wrapped in "OVN_FOR_EACH_NORTHD()".
> >
> >
> > Ack
> >
> >
> > This actually
> >> applies to the "LR NB Static_MAC_Binding table" test too but that's
> >> not introduced by your patch.
> >>
> >
> > I guess this can be a follow up patch too.
> >
>
> Sounds good.
>
> So if you could please wrap the new test case you added with
> OVN_FOR_EACH_NORTHD() then:
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks.  I did the changes in the test file and applied the patch to
the main branch and backported to branch-22.03 and branch-21.12.

I think this needs to be backported further down.  I'll look at this
and the follow up patch next week.

Numan

>
> Thanks,
> Dumitru
>
> > Numan
> >
> >
> >>> +ovn_start
> >>> +
> >>> +# Create logical routers
> >>> +ovn-nbctl --wait=sb lr-add lr0
> >>> +
> >>> +ovn-sbctl dump-flows lr0 > lrflows
> >>> +AT_CAPTURE_FILE([lrflows])
> >>> +
> >>> +AT_CHECK([cat lrflows | grep -e lr_in_lookup_neighbor -e
> >> lr_in_learn_neighbor | sort], [0], [dnl
> >>> +  table=1 (lr_in_lookup_neighbor), priority=0    , match=(1),
> >> action=(reg9[[2]] = 1; next;)
> >>> +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(arp.op == 2),
> >> action=(reg9[[2]] = lookup_arp(inport, arp.spa, arp.sha); next;)
> >>> +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(nd_na),
> >> action=(reg9[[2]] = lookup_nd(inport, nd.target, nd.tll); next;)
> >>> +  table=1 (lr_in_lookup_neighbor), priority=100  , match=(nd_ns),
> >> action=(reg9[[2]] = lookup_nd(inport, ip6.src, nd.sll); next;)
> >>> +  table=2 (lr_in_learn_neighbor), priority=100  , match=(reg9[[2]] ==
> >> 1), action=(next;)
> >>> +  table=2 (lr_in_learn_neighbor), priority=90   , match=(arp),
> >> action=(put_arp(inport, arp.spa, arp.sha); next;)
> >>> +  table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_na),
> >> action=(put_nd(inport, nd.target, nd.tll); next;)
> >>> +  table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_ns),
> >> action=(put_nd(inport, ip6.src, nd.sll); next;)
> >>> +  table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na &&
> >> nd.tll == 0), action=(put_nd(inport, nd.target, eth.src); next;)
> >>> +])
> >>> +
> >>> +AT_CLEANUP
> >>
> >> Regards,
> >> Dumitru
> >>
> >> _______________________________________________
> >> dev mailing list
> >> dev@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >>
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index bc195146d0..8c4187eae1 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10943,6 +10943,12 @@  build_neigh_learning_flows_for_lrouter(
                           copp_meter_get(COPP_ARP, od->nbr->copp,
                                          meter_groups));
 
+        ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 95,
+                          "nd_na && nd.tll == 0",
+                          "put_nd(inport, nd.target, eth.src); next;",
+                          copp_meter_get(COPP_ND_NA, od->nbr->copp,
+                                         meter_groups));
+
         ovn_lflow_metered(lflows, od, S_ROUTER_IN_LEARN_NEIGHBOR, 90,
                           "nd_na", "put_nd(inport, nd.target, nd.tll); next;",
                           copp_meter_get(COPP_ND_NA, od->nbr->copp,
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index db4f4d267c..d95e9c3fd2 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -2307,6 +2307,12 @@  next;
         <code>put_arp(inport, arp.spa, arp.sha); next;</code>
       </li>
 
+      <li>
+        A priority-95 flow with the match <code>nd_na  &amp;&amp;
+        nd.tll == 0</code> and applies the action
+        <code>put_nd(inport, nd.target, eth.src); next;</code>
+      </li>
+
       <li>
         A priority-90 flow with the match <code>nd_na</code> and
         applies the action
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index adb3043853..163287edfa 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -6664,3 +6664,26 @@  ovn-nbctl --may-exist static-mac-binding-add lr0-p0 192.168.10.100 00:00:22:33:5
 wait_row_count Static_MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66"
 
 AT_CLEANUP
+
+AT_SETUP([LR neighbor lookup and learning flows])
+ovn_start
+
+# Create logical routers
+ovn-nbctl --wait=sb lr-add lr0
+
+ovn-sbctl dump-flows lr0 > lrflows
+AT_CAPTURE_FILE([lrflows])
+
+AT_CHECK([cat lrflows | grep -e lr_in_lookup_neighbor -e lr_in_learn_neighbor | sort], [0], [dnl
+  table=1 (lr_in_lookup_neighbor), priority=0    , match=(1), action=(reg9[[2]] = 1; next;)
+  table=1 (lr_in_lookup_neighbor), priority=100  , match=(arp.op == 2), action=(reg9[[2]] = lookup_arp(inport, arp.spa, arp.sha); next;)
+  table=1 (lr_in_lookup_neighbor), priority=100  , match=(nd_na), action=(reg9[[2]] = lookup_nd(inport, nd.target, nd.tll); next;)
+  table=1 (lr_in_lookup_neighbor), priority=100  , match=(nd_ns), action=(reg9[[2]] = lookup_nd(inport, ip6.src, nd.sll); next;)
+  table=2 (lr_in_learn_neighbor), priority=100  , match=(reg9[[2]] == 1), action=(next;)
+  table=2 (lr_in_learn_neighbor), priority=90   , match=(arp), action=(put_arp(inport, arp.spa, arp.sha); next;)
+  table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_na), action=(put_nd(inport, nd.target, nd.tll); next;)
+  table=2 (lr_in_learn_neighbor), priority=90   , match=(nd_ns), action=(put_nd(inport, ip6.src, nd.sll); next;)
+  table=2 (lr_in_learn_neighbor), priority=95   , match=(nd_na && nd.tll == 0), action=(put_nd(inport, nd.target, eth.src); next;)
+])
+
+AT_CLEANUP