[ovs-dev] [nxm 36/42] ofproto: Implement Nicira Extended Match flexible flow match (NXM).

[Justin Pettit]

On Oct 28, 2010, at 10:28 AM, Ben Pfaff wrote:

>     /* A nonexistent table ID was specified in the "command" field of struct
>      * ofp_flow_mod, when the nxt_flow_mod_table_id extension is enabled. */
> -    NXFMFC_BAD_TABLE_ID
> +    NXFMFC_BAD_TABLE_ID = 0x101

I don't think you brought this functionality over from the "wdp" branch yet.  I can understand the desire to reserve a spot.  Maybe an indication that it isn't yet supported in this branch may be useful?

> +/* Values for the 'subtype' member of struct nicira_stat_header. */

Do you mean "nicira_stat_msg"?

> +/* ## ------------------ ## */
> +/* ## Nicira extensions. ## */
> +/* ## ------------------ ## */

What about calling this something like "Nicira match extenstions"?  Everything in this file is a Nicira extension, so it's a bit confusing.  Also, it would be nice to put the same sort of title above the "OpenFlow" match extensions.

> +enum {
> +    NXFF_OPENFLOW10 = 0,         /* Standard OpenFlow 1.0 compatible. */
> +    NXFF_TUN_ID_FROM_COOKIE = 1, /* Obtain tunnel ID from cookie. */
> +    NXFF_NXM = 2                 /* Nicira extended match. */
> +};

I think it would make things clearer if you mention that NXFF_TUN_ID_FROM_COOKIE is a superset of NXFF_OPENFLOW10.

> +/* NXT_SET_FLOW_FORMAT request. */
> +struct nxt_set_flow_format {
> +    struct ofp_header header;
> +    ovs_be32 vendor;            /* NX_VENDOR_ID. */
> +    ovs_be32 subtype;           /* NXT_ENABLE_NXM */
> +    ovs_be32 format;            /* One of NXFF_*. */
> +};

I believe "subtype" is supposed to be "NXT_SET_FLOW_FORMAT". 

What do you think about deprecating NXT_TUN_ID_FROM_COOKIE?  Of course, the whole tunnel id from cookie thing should get deprecated, but may as well start here.

> +/* Nicira vendor stats request of type NXST_FLOW. */
> +struct nx_flow_stats_request {

To be consistent with the earlier messages, I'd point out that this is analogous to OFPST_FLOW.

> +/* Nicira vendor stats request of type NXST_AGGREGATE. */
> +struct nx_aggregate_stats_request {

Same thing here, but being analogous to OFPST_AGGREGATE.

> +/* For each NXM_OF_* field, define NFI_NXM_OF_* as consecutive integers
> + * starting from zero. */
> +enum nxm_field_index {
> +#define DEFINE_FIELD(HEADER, WILDCARD, DL_TYPE, NW_PROTO) NFI_NXM_##HEADER,
> +#include "nx-match.def"
> +    N_NXM_FIELDS
> +};

I don't believe the comment is correct that this is limited to "OF_*" fields, since NX_TUN_ID will also be picked up.

> +static void
> +nxm_init(void)
> +{
> ...
> +        /* Verify that the header values are unique. */
> +        switch (0) {
> +#define DEFINE_FIELD(HEADER, WILDCARD, DL_TYPE, NW_PROTO) \
> +        case NXM_##HEADER: break;
> +#include "nx-match.def"
> +        }

It took me a minute to realize that this blows up in a compilation error due to multiple case statements with the same value.  I think an extra sentence stating that would help those of us who tend to be a bit slower.

> +static int
> +parse_nxm_entry(struct cls_rule *rule, const struct nxm_field *f,
> +                const void *value, const void *mask)
> +{
> ...
> +        /* ICMP header. */
> +    case NFI_NXM_OF_ICMP_TYPE:
> +        flow->tp_src = htons(get_unaligned_u16(value));
> +        return 0;
> +    case NFI_NXM_OF_ICMP_CODE:
> +        flow->tp_dst = htons(get_unaligned_u16(value));
> +        return 0;

Aren't these ICMP values stored in the NXM structure as a single byte?

> +        /* ARP header. */
> +    case NFI_NXM_OF_ARP_OP:
> +        if (htons(get_unaligned_u16(value)) > 255) {
> +            return NXM_BAD_VALUE;
> +        } else {
> +            flow->nw_proto = htons(get_unaligned_u16(value));
> +            return 0;
> +        }

Shouldn't those two htons() calls be ntohs()?

> +static uint32_t
> +nx_entry_ok(const void *p, unsigned int match_len)
> +{
> ...
> +    header_len = NXM_LENGTH(header);


This isn't really a "header_len", is it?  Wouldn't "nxm_length" or something be more correct?

> +int
> +nx_pull_match(struct ofpbuf *b, unsigned int match_len, uint16_t priority,
> +              struct cls_rule *rule)
> +{
> ...
> +    while ((header = nx_entry_ok(p, match_len)) != 0) {

Based on the way nx_entry_oky() is written, I believe someone could send an all zero nx_match entry and terminate this loop early.  The function will return NXM_INVALID, but it won't log like anything like some of the other invalid entries will.

> +        unsigned length = NXM_LENGTH(header);
> +        const struct nxm_field *f;
> +        int error;
> +
> +        f = nxm_field_lookup(header);
> +        if (!f) {
> +            error = NXM_BAD_TYPE;
> +        } else if (!nxm_prereqs_ok(f, &rule->flow)) {
> +            error = NXM_BAD_PREREQ;
> +        } else if (f->wildcard && !(rule->wc.wildcards & f->wildcard)) {
> +            error = NXM_DUP_TYPE;
> +        } else {
> +            rule->wc.wildcards &= ~f->wildcard;
> +            error = parse_nxm_entry(rule, f, p + 4, p + 4 + length / 2);
> +        }


It doesn't seem like enough is done to sanity-check the lengths against what they are supposed to do.  For example, if the sender were to send an IP match with the "hasmask" bit set, but with the length of one without the "hasmask" bit set, I believe parse_nxm_entry() will read a couple bytes past the end of the buffer.  We know how long each match entry should be, so it should be pretty easy to make sure that the length in the provided header matches the definition for nxm_field.

It would probably be good to run a fuzzer against this function at some point, since it seems ripe for abuse.

> +/* nx_put_match() and helpers. */

It wasn't immediately obvious to me which of the helper functions took wildcarded header versions and which didn't.  Rather than document each one individually, it may be helpful to indicate that the ones that end in "w" take the wildcarded version and the others don't.

> +char *
> +nx_match_to_string(const uint8_t *p, unsigned int match_len)
> +{
> ...
> +            ds_put_format(&s, "%#x:%d", NXM_VENDOR(header), NXM_FIELD(header));

In nx_pull_match(), the vendor is printed in decimal.  Since these aren't the OUIs and the number is pretty small, I think decimal makes sense.

> +static int
> +handle_nxst_aggregate(struct ofproto *ofproto, struct ofconn *ofconn,
> +                      struct ofpbuf *b)
> +{
> ...
> +    buf = start_nxstats_reply(&request->nsm, sizeof *reply);

Do you think it's worth renaming start_stats_reply() and its ilk to start_ofp_stats_reply() and such?

> +    reply = ofpbuf_put_uninit(buf, sizeof *reply);
> +    query_aggregate_stats(ofproto, &target, request->out_port,
> +                          request->table_id, reply);

This isn't directly related to this commit, but I think the "out_port" argument for query_aggregate_stats() should probably be changed to ovs_be16, since it's supposed to be in network-byte order.

> static int
> +handle_vendor_stats_request(struct ofproto *p, struct ofconn *ofconn,
> +                            struct ofp_stats_request *osr, size_t arg_size)
> +{
> ...
> +    nsm = (struct nicira_stats_msg *) osr;
> +    b.data = nsm;
> +    b.size = ntohs(osr->header.length);
> +    switch (ntohl(nsm->subtype))

I think it would be clearer if "nsm->header.length" were used instead of "osr->header.length".

> static int
> +handle_nxt_set_flow_format(struct ofconn *ofconn,
> +                           struct nxt_set_flow_format *msg)
> +{
> ...
> +    if (msg->format == NXFF_OPENFLOW10
> +        || msg->format == NXFF_TUN_ID_FROM_COOKIE
> +        || msg->format == NXFF_NXM) {

The "format" member is ovs_be32.  Shouldn't "msg->format" have an ntohl() done on it first?

Thanks for implementing this, Ben.  This is fantastic new functionality, and I think will make our lives a lot easier going forward.

--Justin