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

Ben Pfaff blp at nicira.com
Wed Nov 10 01:08:24 UTC 2010


On Tue, Nov 09, 2010 at 11:25:15AM -0800, Justin Pettit wrote:
> 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?

OK, added.

> > +/* Values for the 'subtype' member of struct nicira_stat_header. */
> 
> Do you mean "nicira_stat_msg"?

Yes, thanks.

> > +/* ## ------------------ ## */
> > +/* ## 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.

OK, done.

> > +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.

OK, done.

> > +/* 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". 

Yes, thanks.

> 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.

I guess I don't see any point to deprecating it in a piecemeal way.  I
don't think anyone is going to update their code to use
NXT_SET_FLOW_FORMAT instead of NXT_TUN_ID_FROM_COOKIE.

> > +/* 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.

Done, thank.

> > +/* Nicira vendor stats request of type NXST_AGGREGATE. */
> > +struct nx_aggregate_stats_request {
> 
> Same thing here, but being analogous to OFPST_AGGREGATE.

Ditto.

> > +/* 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.

You're right, I fixed the comment.

> > +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.

OK, comment added.

> > +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?

You're right (and the corresponding protocol headers are just one byte
each).

Fixed.

> > +        /* 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()?

Yes, thanks, fixed.

> > +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?

It's the length taken from the header but yes the name is confusing.

I changed it to "payload_len".

> > +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.

Good spotting, I didn't even think about that.  It's an important case
to handle because it could happen accidentally by misinterpreting
padding with zero bytes.

I added a check for a payload_len of 0, which isn't ever valid.

> > +        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.

This kind of situation is covered because nxm_field_lookup() checks that
'header' is one that we know how to handle.  'header' includes vendor
and type, but it also (critically, as you point out) includes length and
hasmask.

I added a comment.

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

I don't think it's as easy to abuse as all that, but it's always good to
try a fuzzer (do you know of a good general-purpose one?).

> > +/* 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.

OK, I added a comment, thanks.

> > +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.

OK, changed here.

> > +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?

Can't hurt, so I added a commit that does that.

> > +    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.

Thanks, I added a commit to fix that too.

> > 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".

OK, done.

> > 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?

Oops, thanks.  Obviously I haven't had a chance to fix the new
functionality here.




More information about the dev mailing list