[ovs-dev] [PATCH ovn v2] Learn the mac binding only if required

[Han Zhou]

On Mon, Sep 16, 2019 at 10:17 AM <nusiddiq at redhat.com> wrote:
>
> From: Numan Siddique <nusiddiq at redhat.com>
>
> OVN has the actions - put_arp and put_nd to learn the mac bindings from
the
> ARP/ND packets. These actions update the Southbound MAC_Binding table.
> These actions translates to controller actions. Whenever pinctrl thread
> receives such packets, it wakes up the main ovn-controller thread.
> If the MAC_Binding table is already upto date, this results
> in unnecessary CPU cyles. There are some security implications as well.
> A rogue VM can flood broadcast ARP request/reply packets and this
> could cause DoS issues. A physical switch may send periodic GARPs
> and these packets hit ovn-controllers.
>
> This patch solves these problems by learning the mac bindings only if
> required. There is no need to apply the put_arp/put_nd action if the
> Southbound MAC_Binding row is upto date.
>
> New actions - lookup_arp and lookup_nd are added which looks up the
> IP, MAC pair in the mac_binding table and stores the result in a
> register. 1 if lookup is successful, 0 otherwise.
>
> ovn-northd adds 2 new stages - lookup_arp and put_arp before ip_input
> in the router ingress pipeline.
>
> The logical flows looks something like:
>
> table=1 (lr_in_lookup_arp), priority=100  , match=(arp),
>          reg9[4] = lookup_arp(inport, arp.spa, arp.sha); next;)
>
> table=1 (lr_in_lookup_arp), priority=0    , match=(1), action=(next;)
> ...
> table=2 (lr_in_put_arp   ), priority=100  ,
>          match=(arp.op == 2 && reg9[4] == 0),
>          action=(put_arp(inport, arp.spa, arp.sha);)
> table=2 (lr_in_put_arp   ), priority=90   , match=(arp.op == 2),
action=(drop;)
> table=2 (lr_in_put_arp   ), priority=0    , match=(1), action=(next;)
>
> The lflow module of ovn-controller adds OF flows in table 31
(OFTABLE_MAC_LOOKUP)
> for each mac_binding entry with the match reg0 = ip && eth.src = mac with
> the action - load:1->reg2[0]
>
> Eg:
> table=31,
priority=100,arp,reg0=0xaca8006f,reg14=0x3,metadata=0x3,dl_src=00:44:00:00:00:04
>           actions=load:1->NXM_NX_REG2[0]
>
> This patch should also address the issue reported in 'Reported-at'
>
> Reported-at: https://bugzilla.redhat.com/1729846
> Reported-by: Haidong Li <haili at redhat.com>
> CC: Han ZHou <hzhou8 at ebay.com>
> CC: Dumitru Ceara <dceara at redhat.com>
> Tested-by: Dumitru Ceara <dceara at redhat.com>
> Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> ---
>
> v1 -> v2
> =======
>    * Addressed review comments from Han - Storing the result
>      of lookup_arp/lookup_nd in a register.
>
>  controller/lflow.c           |  36 ++++-
>  controller/lflow.h           |   1 +
>  include/ovn/actions.h        |  13 ++
>  include/ovn/logical-fields.h |   3 +
>  lib/actions.c                | 115 ++++++++++++++
>  northd/ovn-northd.8.xml      | 251 ++++++++++++++++++++----------
>  northd/ovn-northd.c          | 205 ++++++++++++++-----------
>  ovn-sb.xml                   |  57 +++++++
>  tests/ovn.at                 | 290 ++++++++++++++++++++++++++++++++++-
>  tests/test-ovn.c             |   1 +
>  utilities/ovn-trace.c        |  69 +++++++++
>  11 files changed, 861 insertions(+), 180 deletions(-)
>

Hi Numan,

This looks great. I spent more time on the review and here are my comments:

1. #define MFF_LOG_LOOKUP_MAC MFF_REG2

This new logical field is overlapping with the logical registers, as
defined:

/* Logical registers.
   *
   * Make sure these don't overlap with the logical fields! */
  #define MFF_LOG_REG0 MFF_REG0
  #define MFF_N_LOG_REGS 10

REG0 - REG9 are already reserved by above definition. If we have to use
one, it should be REG9, and then update MFF_N_LOG_REGS to 9. However, I
think it is better to use just one bit from MFF_LOG_FLAGS. The bits of the
flag is defined as MLF macros, and we can define a new one for
MLF_LOOKUP_MAC_BIT.
In addition, this change needs to be documented in ovn-architecture.

2. #define OFTABLE_MAC_LOOKUP           31

Table 31 is the last table of logical flows. This mac lookup table is not
part of logical flows, so it is better to use table 67.

3. lookup_arp/lookup_nd needs to be documented in ovn-architecture, at the
same place where put_arp/get_arp, etc. are documented.

4. In ovn-northd.xml, the term "MAC learning" is better to be changed to
something like "MAC-binding learning", or just "Neigbour learning", because
"MAC learning" usually means MAC-port table populating in L2 switch in
networking terminology. It is better to avoid the ambiguity.

5. For the pipeline, introducing the neigbour learning stage makes the
pipeline more clean. However, I think it could be a little more efficient.
Table 1 matches ARP and ND, and table 2 redo the match again. Would it be
better to have the default flow in table 1 set reg9[4] = 1, so that in
table 2 we can have a high priority flow just match "reg9[4] = 1" with
action "next"? This way, for non-ARP/ND packets, which is the most case,
are more efficient? Also, this makes the table 2 more clean with just one
task: neighbour learning, and leave the tasks of dropping the
packets/replying ARP/ND to the next stages.

Thanks,
Han