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

Tue Sep 24 20:25:54 UTC 2019

Thanks Han for the reviews.

Please see below for some comments.

Thanks
Numan

On Wed, Sep 18, 2019 at 4:56 AM Han Zhou <zhouhan at gmail.com> wrote:

>
> 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.
>

Agree. I will do.

>
> 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.
>

I thought about this. Since the action lookup_arp/lookup_nd uses inport
where as the action put_arp use outport , i thought it is better to handle
this with in table 31.
But I think it should be fine if we refer to inport (reg14) in table 67. I
will modify to 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.
>

Ack.

>
> 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.
>

Ack.

>
> 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.
>

Makes sense to me. Please take a look at v3.
I have added default flow in table to set reg9[5] as it would be odd to set
reg9[4] since no lookup is done.
In table 2 a priority-100 flow is added which advances to the next table if
reg9[5] == 1 || reg9[4] == 1.

Thanks
Numan

> Thanks,
> Han
>