[ovs-discuss] [ACL 2/3] vswitchd: Implement local ACL functionality.
Ben Pfaff
blp at nicira.com
Wed Aug 5 18:12:07 UTC 2009
Thanks for passing along the patch. Sorry that it took me a
while to review it.
To begin, I have some high-level comments.
First, acl.[ch] depends on ofproto, so it should not be in lib,
because ofproto is allowed to have dependencies on lib but not
vice-versa (take a look--all the dependencies go one way).
Furthermore, ofproto does not currently have any dependencies on
the cfg.[ch] library, because the standalone OpenFlow
ovs-openflowd doesn't use a configuration file and doesn't
provide a way to specify one. So, I would move it to vswitchd/
and make it vswitchd-specific.
Second, I was surprised by the conceptual organization:
* ACLs are specific to a bridge, but they are tracked in
a global data structure. I think that this code is
correct, but I was expecting to see something like a
"struct acl_engine *" member in either struct bridge or
struct ofproto, where the acl_engine would encapsulate
everything related to the ACL for that bridge and look
at configuration for ACLs for that bridge only.
* I expected to see a function that composes the entire
flow table for a bridge, but instead there is a
function that scrubs and replaces the flow table for a
single port. Again, I don't see correctness problems,
but I think that the code would be simpler and more
obviously correct if the entire classifier were to be
cleared and restocked in one go.
(If you did it this way then you wouldn't need the
ofproto_delete_flows_wildcarded() function.)
* There is one "struct classifier" per ACL port, whereas
I was expecting to see no additional classifiers at
all, following the model used by NOX, which uses
Nicira's OpenFlow extension action NXAST_RESUBMIT to
implement both ingress and egress actions within the
single OpenFlow classifier table.
* It looks like ACLs are actually over bridge interfaces,
not bridge ports. An ordinary port has exactly one
interface, of the same name, so there is no difference
in that case, but bonded ports have multiple
interfaces. It doesn't (as far as I can tell) make
sense to apply an ACL to only one interface of a bonded
port, so I would expect that ACLs should specify port
names and that an ACL over a bonded port would apply to
all of its interfaces in the same way.
Some more detailed code comments:
> +struct acl {
> + const char *port_name; /* Name of port */
It makes me nervous to see this as a "const char *" with a string
passed in by acl_iface_init()'s caller. Then you have to be
absolutely certain that the ACL's lifetime is longer than the
port's or risk use-after-free errors.
I would be happier to see it as a "char *" that acl_iface_init()
xstrdup()s.
> + struct ofproto *ofproto; /* Bridge OpenFlow switch */
> + uint16_t cur_ofp_port; /* The port in rules are installed on */
> + struct classifier out_rules; /* Lookup table for applying egress ACL's */
> + bool has_in_rules; /* Have in rules been applied to this port? */
> + bool has_out_rules; /* Have out rules been applied to this port? */
It sure looks to me like has_in_rules is true if and only if any
rules have been added to ofproto's classifier table, and that
has_out_rules is true if and only if out_rules is nonempty. If
that's the case, then I'd prefer to have these members dropped
and, if necessary, in their place add helper functions that check
for those conditions. Then there is less redundancy and
therefore fewer possible internal inconsistencies.
> +};
> +
> +struct acl_rule {
> + flow_t flow; /* Flow entry for this rule */
> + uint32_t wildcards; /* Wildcards for this rule */
> + unsigned int priority; /* Rule priority */
> + struct cls_rule cr; /* Classifier rule for out ACL's */
> + bool permit; /* Allow further processing or drop packets */
As in the documentation, I like "allow" better than "permit",
because I see it more frequently in a networking context.
> +};
[...]
> +
> +/* The action to be taken if a packet is permitted. */
> +static union ofp_action permit_action;
I'd like this to be a "static const", so that it's read-only
instead of write-once and there's no possible way to use it
without initializing. I guess we don't have compile-time
hton[sl], but we could easily add implementations.
Alternatively a helper function would be OK (but an initializer
would be better):
static union ofp_action *
normal_action(void)
{
static union ofp_action permit_action;
static bool inited;
if (!inited) {
inited = true;
permit_action.type = htons(OFPAT_OUTPUT);
permit_action.output.len = htons(sizeof permit_action);
permit_action.output.port = htons(OFPP_NORMAL);
}
return &permit_action;
}
or even:
static union ofp_action
normal_action(void)
{
static union ofp_action normal_action;
normal_action.type = htons(OFPAT_OUTPUT);
normal_action.output.len = htons(sizeof normal_action);
normal_action.output.port = htons(OFPP_NORMAL);
return normal_action;
}
Regardless, there are other bits of code in the tree that could
use this, since they initialize OFPP_NORMAL actions too.
> +void
> +acl_reconfigure(void)
> +{
> + struct svec new_cfg = SVEC_EMPTY_INITIALIZER;
> + struct shash_node *old_node;
> + struct acl_group *old_rule_set;
> + int i;
> +
> + /* ACL's are switch local, so disable if a controller is configured. */
> + if (cfg_has_section("acl")) {
> + if (cfg_has("mgmt.controller")) {
This notion of whether there is a controller is too simple,
because mgmt.controller is not the only way to configure a
controller, and at any given time some bridges may have a
controller configured and others have no controller.
> + if (acls_active) {
> + VLOG_WARN("CONTROLLER CONFIGURED, DISABLING ACL'S");
Wow, that's really loud. Maybe you could just turn the log level
up to VLOG_ERR and drop the capitals.
> +void
> +acl_iface_init(const char *port_name, struct ofproto *ofproto,
> + struct acl **info)
> +{
> + *info = xmalloc(sizeof **info);
> + (*info)->port_name = port_name;
> + (*info)->ofproto = ofproto;
> + classifier_init(&(*info)->out_rules);
> + (*info)->has_in_rules = false;
> + (*info)->has_out_rules = false;
All the (*info)-> gets to me after a while. Could you assign
*info to a variable to avoid that, please?
> +void
> +acl_iface_reconfigure(struct acl *info, uint16_t ofp_port)
> +{
> + flow_t flow;
> + uint32_t wildcards;
> + const char *acl_group_name;
> + struct svec priorities = SVEC_EMPTY_INITIALIZER;
'priorities' is only used within a single 'if' statement below,
so please move its declaration down there.
> + int i;
> + const char *glob;
> +
[...]
> + /* Wildcarded rules. */
> + if (!info->has_in_rules || !info->has_out_rules) {
> + cfg_get_subsections(&priorities, "acl.default");
> + svec_sort(&priorities);
The output of cfg_get_subsections() is always in sorted order
anyhow. (Hmm, we should document that.)
But I don't think that svec_sort() does what you want anyway,
because it sorts into alphabetical order, not numerical order.
That is, "10" < "2", because '1' < '2'.
> + for (i = 0; i < priorities.n; i++) {
> + glob = cfg_get_string(0, "acl.default.%s.match",
> + priorities.names[i]);
> + if (glob) {
> + if (fnmatch(glob, info->port_name, 0) == 0) {
[...]
> + }
> + } else {
> + VLOG_WARN("invalid glob when processing default acl's,"
> + " skipping");
I don't understand how that's an invalid glob. A missing glob,
maybe. An invalid glob would be indicated by fnmatch() returning
a value other than 0 or FNM_NOMATCH (but the GNU libc
documentation says that glibc never returns a value other than
one of those two). At any rate the error message should name the
key that was missing (or invalid) to keep the administrator from
having to guess.
[...]
> +static void
> +install_rules(const char *group_name, uint16_t ofp_port, bool in_rule,
> + struct acl *info)
> +{
> + struct acl_group *access_rules;
> + int i;
> + flow_t flow;
> + uint32_t wildcards;
> +
> + access_rules = shash_find_data(&acls, group_name);
> + if (access_rules) {
[...]
> + } else {
> + VLOG_WARN("acl group '%s' not found", group_name);
Does this warning indicate a bug in vswitchd or bad
configuration? It's not clear to me from the message.
> + }
> +}
[...]
> +static void
> +update_rules(void)
> +{
[...]
> + for (i = 0; i < groups.n; i++) {
> + svec_clear(&priorities);
> +
> + cfg_get_subsections(&priorities, "acl.group.%s", groups.names[i]);
> + svec_sort(&priorities);
Again, I don't think svec_sort() gives the right sort order here.
[...]
> + for (j = 0; j < priorities.n; j++) {
> + acl = cfg_get_string(0, "acl.group.%s.%s", groups.names[i],
> + priorities.names[j]);
> + if (acl) {
> + final_rule = parse_acl(acl, priorities.n - j,
> + &rule_set->rules[priorities.n - j]);
parse_acl() will store a null pointer into
rule_set->rules[priorities.n - j] on error. Is that OK?
> + if (final_rule) {
> + break;
If this isn't the last rule (if j != priorities.n - 1) it would
be nice to log that later rules are being ignored (because they
could not possibly have any effect).
> + }
> + } else {
> + VLOG_WARN("invalid rule when processing acl's, skipping");
cfg_get_string() only returns a null pointer if there's a missing
key, so this is really a missing rule, right? And we should say
the name of the key that is missing.
[...]
> +}
> +
> +/* Returns true if this is a completely wildcarded rule. */
> +static bool
> +parse_acl(const char *acl_, unsigned int priority, struct acl_rule **rule)
> +{
> + char *acl;
> + char *c;
> + int num_args;
> + char action[7];
> + char proto_str[5];
> + char srcip_str[19];
> + char dstip_str[19];
> + char srcport_str[6];
> + char dstport_str[6];
> + flow_t *flow;
> + uint32_t *wildcards;
> +
> + *rule = xcalloc(1, sizeof **rule);
> + flow = &(*rule)->flow;
Again there's a lot of (*rule)-> in this function and it would be
nicer to assign *rule to a variable.
[...]
> + if (!extract_port(dstport_str, OFPFW_TP_DST, &flow->tp_dst, wildcards)) {
> + goto error;
> + }
> + num_args--;
> + if (!num_args) {
> + goto finished;
> + }
I don't think we can actually get here: num_args should always be
0. Is that true? It would be nice to inserted a NOT_REACHED()
if so, otherwise it looks like we can fall through to the
'error:' label without logging an actual error.
> +error:
> + free(*rule);
> + *rule = NULL;
> +
> +finished:
> + free(acl);
> +
> + if (*rule) {
> + cls_rule_from_flow(&(*rule)->cr, flow, *wildcards, priority);
> + return (*wildcards == OFPFW_ALL);
> + } else {
> + return false;
> + }
> +}
> +
> +/* Returns true if the string parsed correctly. */
> +static bool
> +extract_ip_mask(const char *mask, int wildcard_shift, int of_wildcard_mask,
> + uint32_t *address, uint32_t *wildcards)
> +{
It would be nice if this could be unified with the code to parse
IP and netmasks in ovs-ofctl.c, perhaps in socket-util.c.
[...]
> + if (separator && *(separator + 1)) {
> + cidr = separator + 1;
> + wildcard_mask = atoi(cidr);
> + if (wildcard_mask) {
> + /* Wildcards are opposite of CIDR. */
> + wildcard_mask = 32 - wildcard_mask;
> + *wildcards |= wildcard_mask << wildcard_shift;
This will silently accept a mask that looks like, say,
"255.255.0.0", but misinterpret it badly ((32 - 255) doesn't
produce anything sensible).
> + } else {
> + VLOG_WARN("invalid netmask when processing acl, skipping rule; "
> + "bad address: %s", mask);
0.0.0.0/0 might otherwise be a very explicit way of saying "any",
but this will reject it.
> + ret = false;
> + }
> + }
> +
> + free(ip);
> + return ret;
> +}
> +
> +/* Returns true if the string parsed correctly. */
> +static bool
> +extract_port(const char *str, int wildcard_val, uint16_t *port,
> + uint32_t *wildcards)
> +{
> + unsigned short int port_val;
> +
> + if (strcmp(str, "any") != 0) {
> + port_val = atoi(str);
> + if (port_val > 0 && port_val <= USHRT_MAX) {
USHRT_MAX can vary among C implementations, so please use
UINT16_MAX instead, which is always 65535.
Also 0 is a valid, if rarely used, TCP and UDP port. In
particular hping3 sends data to TCP port 0 by default, so denying
traffic on port 0 could be useful for preventing various kinds of
scans.
Also port_val needs to be some type other than "unsigned short"
if you want to catch out-of-range values ;-)
> + *port = htons(port_val);
> + *wildcards &= ~wildcard_val;
> + } else {
> + VLOG_WARN("invalid port specified, skipping rule; port: %s", str);
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> +static void
> +add_default_acl(struct acl_rule **rule)
> +{
> + struct acl_rule *new_rule;
> +
> + /* Create a lowest priority default rule that denies all traffic. */
> + new_rule = *rule = xcalloc(1, sizeof **rule);
> +
> + new_rule->permit = false;
> + new_rule->priority = 0;
> + new_rule->wildcards = OFPFW_ALL;
> +
> + cls_rule_from_flow(&new_rule->cr, &new_rule->flow, new_rule->wildcards,
> + new_rule->priority);
> +}
> --- /dev/null
> +++ b/lib/acl.h
> @@ -0,0 +1,36 @@
[...]
> +#include "ofproto/ofproto.h"
Should #include <stdint.h> for uint16_t and "flow.h" for flow_t.
> +struct acl;
> +
> +void acl_init(void);
> +void acl_reconfigure(void);
> +
> +bool acl_is_active(void);
> +
> +void acl_iface_init(const char *port_name, struct ofproto *ofproto,
> + struct acl **info);
> +void acl_iface_reconfigure(struct acl *info, uint16_t ofp_port);
> +void acl_iface_destroy(struct acl *info);
> +
> +bool acl_iface_output_filter(struct acl *info, const flow_t *flow);
> +
> +#endif /* acl.h */
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 6b05d13..e89b468 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
[...]
> @@ -440,7 +443,7 @@ bridge_reconfigure(void)
> {
> struct svec old_br, new_br;
> struct bridge *br, *next;
> - size_t i;
> + size_t i,j;
Please add a space: size_t i, j;
> @@ -1652,18 +1674,22 @@ compose_dsts(const struct bridge *br, const flow_t *flow, uint16_t vlan,
>
> *tags |= in_port->stp_state_tag;
> if (out_port == FLOOD_PORT) {
> - /* XXX use ODP_FLOOD if no vlans or bonding. */
> + /* XXX use ODP_FLOOD if no vlans, bonding, or ACL's. */
Comment is inaccurate (not your fault) since there is no
ODP_FLOOD or ODPP_FLOOD. Datapath has port groups instead ;-)
> /* XXX even better, define each VLAN as a datapath port group */
> for (i = 0; i < br->n_ports; i++) {
> struct port *port = br->ports[i];
> if (port != in_port && port_includes_vlan(port, vlan)
> && !port->is_mirror_output_port
> - && set_dst(dst, flow, in_port, port, tags)) {
> + && set_dst(dst, flow, in_port, port, tags)
> + && acl_iface_output_filter(iface_from_dp_ifidx(br,
> + dst->dp_ifidx)->acl, flow)) {
Filtering should really be done per-port, not per-interface (as
mentioned in high-level comments).
> mirrors |= port->dst_mirrors;
> dst++;
> }
> }
> - } else if (out_port && set_dst(dst, flow, in_port, out_port, tags)) {
> + } else if (out_port && set_dst(dst, flow, in_port, out_port, tags)
> + && acl_iface_output_filter(iface_from_dp_ifidx(br,
> + dst->dp_ifidx)->acl, flow)) {
> mirrors |= out_port->dst_mirrors;
> dst++;
> }
More information about the discuss
mailing list