| Message ID | 1441996587-615-2-git-send-email-blp@nicira.com |
|---|---|
| State | Accepted |
| Headers | show |
> On Sep 11, 2015, at 11:36 AM, Ben Pfaff <blp@nicira.com> wrote: > > @@ -744,22 +744,23 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, > > /* Port security flows have priority 50 (see below) and will continue > * to the next table if packet source is acceptable. */ > - > - /* Otherwise drop the packet. */ > - ovn_lflow_add(&lflows, od, P_IN, S_IN_PORT_SEC, 0, "1", "drop;"); > } > > /* Ingress table 0: Ingress port security (priority 50). */ > struct ovn_port *op; > HMAP_FOR_EACH (op, key_node, ports) { > + if (!lport_is_enabled(op->nb)) { > + continue; > + } Do you think it's worth mentioning here that this effectively drops packets coming from disabled ports? Acked-by: Justin Pettit <jpettit@nicira.com> --Justin
On Fri, Sep 11, 2015 at 12:57:18PM -0700, Justin Pettit wrote: > > > On Sep 11, 2015, at 11:36 AM, Ben Pfaff <blp@nicira.com> wrote: > > > > @@ -744,22 +744,23 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, > > > > /* Port security flows have priority 50 (see below) and will continue > > * to the next table if packet source is acceptable. */ > > - > > - /* Otherwise drop the packet. */ > > - ovn_lflow_add(&lflows, od, P_IN, S_IN_PORT_SEC, 0, "1", "drop;"); > > } > > > > /* Ingress table 0: Ingress port security (priority 50). */ > > struct ovn_port *op; > > HMAP_FOR_EACH (op, key_node, ports) { > > + if (!lport_is_enabled(op->nb)) { > > + continue; > > + } > > Do you think it's worth mentioning here that this effectively drops packets coming from disabled ports? OK, I added a comment: if (!lport_is_enabled(op->nb)) { /* Drop packets from disabled logical ports (since logical flow * tables are default-drop). */ continue; } > Acked-by: Justin Pettit <jpettit@nicira.com> Thanks, I'll apply this in a minute.
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index a6572df..da7303e 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -744,22 +744,23 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, /* Port security flows have priority 50 (see below) and will continue * to the next table if packet source is acceptable. */ - - /* Otherwise drop the packet. */ - ovn_lflow_add(&lflows, od, P_IN, S_IN_PORT_SEC, 0, "1", "drop;"); } /* Ingress table 0: Ingress port security (priority 50). */ struct ovn_port *op; HMAP_FOR_EACH (op, key_node, ports) { + if (!lport_is_enabled(op->nb)) { + continue; + } + struct ds match = DS_EMPTY_INITIALIZER; ds_put_cstr(&match, "inport == "); json_string_escape(op->key, &match); build_port_security("eth.src", op->nb->port_security, op->nb->n_port_security, &match); - ovn_lflow_add(&lflows, op->od, P_IN, S_IN_PORT_SEC, 50, ds_cstr(&match), - lport_is_enabled(op->nb) ? "next;" : "drop;"); + ovn_lflow_add(&lflows, op->od, P_IN, S_IN_PORT_SEC, 50, + ds_cstr(&match), "next;"); ds_destroy(&match); } @@ -816,8 +817,10 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, ds_destroy(&actions); ds_destroy(&match); } else if (!strcmp(op->nb->macs[i], "unknown")) { - ovn_multicast_add(&mcgroups, &mc_unknown, op); - op->od->has_unknown = true; + if (lport_is_enabled(op->nb)) { + ovn_multicast_add(&mcgroups, &mc_unknown, op); + op->od->has_unknown = true; + } } else { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
There's no need to add a priority-0 "drop" flow, because OVN logical flow tables always drop non-matching packets. There's no need to add a "drop" flow for ingress port security on disabled logical ports, because no other flow would allow those packets; it's more efficient to omit the logical flow entirely. Finally, there's no need to add disabled logical ports to the MC_UNKNOWN multicast group, since packets won't be delivered to a disabled logical port anyway. (This is just an optimization; the packets were dropped in the egress pipeline anyway.) Found by inspection. Signed-off-by: Ben Pfaff <blp@nicira.com> --- ovn/northd/ovn-northd.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)