Re: [ovs-dev] [PATCH ovn v2] Learn the mac binding only if required
Thanks Han for the reviews. Please see below for some comments. Thanks Numan On Wed, Sep 18, 2019 at 4:56 AM Han Zhou wrote: > > On Mon, Sep 16, 2019 at 10:17 AM wrote: > > > > 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_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 > > CC: Han ZHou > > CC: Dumitru Ceara > > Tested-by: Dumitru Ceara > > Signed-off-by: Numan Siddique > > --- > > > > 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
Re: [ovs-dev] [PATCH ovn v2] Learn the mac binding only if required
On Mon, Sep 16, 2019 at 10:17 AM wrote: > > 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_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 > CC: Han ZHou > CC: Dumitru Ceara > Tested-by: Dumitru Ceara > Signed-off-by: Numan Siddique > --- > > 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.
Re: [ovs-dev] [PATCH ovn v2] Learn the mac binding only if required
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 91 characters long (recommended limit is 79) #1031 FILE: ovn-sb.xml:1401: R = lookup_arp(P, A, M); WARNING: Line is 92 characters long (recommended limit is 79) #1066 FILE: ovn-sb.xml:1585: R = lookup_nd(P, A, M); Lines checked: 1559, Warnings: 2, 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 v2] Learn the mac binding only if required
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_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
CC: Han ZHou
CC: Dumitru Ceara
Tested-by: Dumitru Ceara
Signed-off-by: Numan Siddique
---
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(-)
diff --git a/controller/lflow.c b/controller/lflow.c
index d0335a83a..762752753 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,34 @@ 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);
+
