Re: [ovs-dev] [PATCH ovn v3] Learn the mac binding only if required

2019-09-27 Thread Numan Siddique
On Fri, Sep 27, 2019 at 11:51 AM Han Zhou  wrote:

>
>
> On Tue, Sep 24, 2019 at 1:39 PM  wrote:
> > diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml
> > index 6115e84b5..c98db48d2 100644
> > --- a/ovn-architecture.7.xml
> > +++ b/ovn-architecture.7.xml
> > @@ -970,6 +970,24 @@
> >  this temporary use.)
> >
> >  
> > +
> > +R = lookup_arp(P, A,
> M);
> > +R = lookup_nd(P, A,
> M);
> > +
> > +  
> > +Implemented by storing arguments into OpenFlow fields, then
> > +resubmitting to table 67, which ovn-controller
> > +populates with flows generated from the
> MAC_Binding
> > +table in the OVN Southbound database.  If there is a match
> in table
> > +66, then its actions set the logical flow flag
> MLF_LOOKUP_MAC.
>
> A typo here: s/66/67
>
> Otherwise looks good to me.
>
> Acked-by: Han Zhou 
>

Thanks for the review. I fixed by the typo and applied this patch to master.

Numan
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH ovn v3] Learn the mac binding only if required

2019-09-27 Thread Han Zhou
On Tue, Sep 24, 2019 at 1:39 PM  wrote:
> diff --git a/ovn-architecture.7.xml b/ovn-architecture.7.xml
> index 6115e84b5..c98db48d2 100644
> --- a/ovn-architecture.7.xml
> +++ b/ovn-architecture.7.xml
> @@ -970,6 +970,24 @@
>  this temporary use.)
>
>  
> +
> +R = lookup_arp(P, A,
M);
> +R = lookup_nd(P, A,
M);
> +
> +  
> +Implemented by storing arguments into OpenFlow fields, then
> +resubmitting to table 67, which ovn-controller
> +populates with flows generated from the
MAC_Binding
> +table in the OVN Southbound database.  If there is a match
in table
> +66, then its actions set the logical flow flag
MLF_LOOKUP_MAC.

A typo here: s/66/67

Otherwise looks good to me.

Acked-by: Han Zhou 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH ovn v3] Learn the mac binding only if required

2019-09-24 Thread 0-day Robot
Bleep bloop.  Greetings Numan Siddique, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 98 characters long (recommended limit is 79)
#990 FILE: ovn-architecture.7.xml:974:
R = lookup_arp(P, A, 
M);

WARNING: Line is 97 characters long (recommended limit is 79)
#991 FILE: ovn-architecture.7.xml:975:
R = lookup_nd(P, A, 
M);

WARNING: Line is 87 characters long (recommended limit is 79)
#998 FILE: ovn-architecture.7.xml:982:
66, then its actions set the logical flow flag 
MLF_LOOKUP_MAC.

WARNING: Line is 91 characters long (recommended limit is 79)
#1019 FILE: ovn-sb.xml:1401:
  R = lookup_arp(P, A, 
M);

WARNING: Line is 92 characters long (recommended limit is 79)
#1054 FILE: ovn-sb.xml:1585:
R = lookup_nd(P, A, 
M);

Lines checked: 1547, Warnings: 5, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


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

2019-09-24 Thread nusiddiq
From: Numan Siddique 

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_NEIGHBOR and LEARN_NEIGHBOR before
IP_INPUT in the router ingress pipeline.c. The LOOKUP_NEIGHBOR stage
adds flows to do the lookup in the mac_binding table and the LEARN_NEIGHBOR
adds flows to learn the neighbors only if require.

The lflow module of ovn-controller adds OF flows in table 67 
(OFTABLE_MAC_LOOKUP)
for each mac_binding entry with the match reg0 = ip && eth.src = mac with
the action - load:1->reg10[6]

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_REG10[6]

This patch should also address the issue reported in 'Reported-at'

Reported-at: https://bugzilla.redhat.com/1729846
Reported-by: Haidong Li 
CC: Han ZHou 
CC: Dumitru Ceara 
Tested-by: Dumitru Ceara 
Signed-off-by: Numan Siddique 
---

v2 -> v3

  * Addressed review comments from Han.

v1 -> v2

   * Addressed review comments from Han - Storing the result
 of lookup_arp/lookup_nd in a register.

 controller/lflow.c   |  37 -
 controller/lflow.h   |   1 +
 include/ovn/actions.h|  13 ++
 include/ovn/logical-fields.h |   4 +
 lib/actions.c| 114 ++
 northd/ovn-northd.8.xml  | 212 -
 northd/ovn-northd.c  | 205 ++---
 ovn-architecture.7.xml   |  18 +++
 ovn-sb.xml   |  57 +++
 tests/ovn.at | 290 ++-
 tests/test-ovn.c |   1 +
 utilities/ovn-trace.c|  69 +
 12 files changed, 844 insertions(+), 177 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index d0335a83a..e3ed20cd4 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -687,6 +687,7 @@ consider_logical_flow(
 .egress_ptable = OFTABLE_LOG_EGRESS_PIPELINE,
 .output_ptable = output_ptable,
 .mac_bind_ptable = OFTABLE_MAC_BINDING,
+.mac_lookup_ptable = OFTABLE_MAC_LOOKUP,
 };
 ovnacts_encode(ovnacts.data, ovnacts.size, , );
 ovnacts_free(ovnacts.data, ovnacts.size);
@@ -777,7 +778,9 @@ consider_neighbor_flow(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
 return;
 }
 
-struct match match = MATCH_CATCHALL_INITIALIZER;
+struct match get_arp_match = MATCH_CATCHALL_INITIALIZER;
+struct match lookup_arp_match = MATCH_CATCHALL_INITIALIZER;
+
 if (strchr(b->ip, '.')) {
 ovs_be32 ip;
 if (!ip_parse(b->ip, )) {
@@ -785,7 +788,9 @@ consider_neighbor_flow(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
 VLOG_WARN_RL(, "bad 'ip' %s", b->ip);
 return;
 }
-match_set_reg(, 0, ntohl(ip));
+match_set_reg(_arp_match, 0, ntohl(ip));
+match_set_reg(_arp_match, 0, ntohl(ip));
+match_set_dl_type(_arp_match, htons(ETH_TYPE_ARP));
 } else {
 struct in6_addr ip6;
 if (!ipv6_parse(b->ip, )) {
@@ -795,17 +800,35 @@ consider_neighbor_flow(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
 }
 ovs_be128 value;
 memcpy(, , sizeof(value));
-match_set_xxreg(, 0, ntoh128(value));
+match_set_xxreg(_arp_match, 0, ntoh128(value));
+
+match_set_xxreg(_arp_match, 0, ntoh128(value));
+match_set_dl_type(_arp_match, htons(ETH_TYPE_IPV6));
+match_set_nw_proto(_arp_match, 58);
+match_set_icmp_code(_arp_match, 0);
 }
 
-match_set_metadata(, htonll(pb->datapath->tunnel_key));
-match_set_reg(, MFF_LOG_OUTPORT - MFF_REG0, pb->tunnel_key);
+match_set_metadata(_arp_match, htonll(pb->datapath->tunnel_key));
+match_set_reg(_arp_match, MFF_LOG_OUTPORT - MFF_REG0, pb->tunnel_key);
+
+match_set_metadata(_arp_match, htonll(pb->datapath->tunnel_key));
+match_set_reg(_arp_match, MFF_LOG_INPORT - MFF_REG0,
+  pb->tunnel_key);
 
 uint64_t stub[1024 / 8];
 struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);