[ovs-dev] [PATCH] ofproto-dpif-xlate: Fix continuations with OF instructions in OF1.1+.

[Ben Pfaff]

On Tue, Jul 20, 2021 at 08:29:14PM +0200, Ilya Maximets wrote:
> On 7/7/21 8:51 PM, Ben Pfaff wrote:
> > +        /* From an OpenFlow point of view, goto_table and write_metadata are
> > +         * instructions, not actions.  This means that to use them, we'd have
> > +         * to reformulate the actions as instructions, which is possible, and
> > +         * we'd have slot them into the frozen actions in a specific order,
> > +         * which doesn't seem practical.  Instead, we translate these
> > +         * instructions into equivalent actions. */
> > +        case OFPACT_GOTO_TABLE: {
> > +            struct ofpact_resubmit *resubmit
> > +                = ofpact_put_RESUBMIT(&ctx->frozen_actions);
> > +            resubmit->in_port = OFPP_IN_PORT;
> > +            resubmit->table_id = ofpact_get_GOTO_TABLE(a)->table_id;
> > +            resubmit->with_ct_orig = false;
> > +        }
> > +            continue;
> > +        case OFPACT_WRITE_METADATA: {
> > +            const struct ofpact_metadata *md = ofpact_get_WRITE_METADATA(a);
> > +            const struct mf_field *mf = mf_from_id(MFF_METADATA);
> > +            ovs_assert(mf->n_bytes == sizeof md->metadata);
> > +            ovs_assert(mf->n_bytes == sizeof md->mask);
> > +            ofpact_put_set_field(&ctx->frozen_actions, mf,
> > +                                 &md->metadata, &md->mask);
> 
> I have very little knowledge in this area, so this might be not issue.
> However, 'OpenFlow "instructions", which were introduced in OpenFlow 1.1'
> doesn't match with OFPACT_SET_FIELD that is marked as OpenFlow 1.2+
> action.  I see that all tests for continuation have default bridge
> configuration.  And monitors are OF10 and OF13 in different cases, but
> I'm not  sure if these ovs-ofctl monitors are checking that received
> actions for resume matches the OF version they are using.  And for
> the bridge itself, it supports all versions of OF, so it can interpret
> actions.  But the question is: will that work if the bridge only
> supports OpenFlow 1.1 ?  It should fail to parse OFPACT_SET_FIELD, or
> am I missing something here?

Thanks a lot for the review!

OVS can serialize most actions and instructions to most OpenFlow
versions, even if they are not natively supported in that particular
OpenFlow version.  For example, in the case of OFPACT_SET_FIELD (a
rather important action), the code in ofp-actions.c has complete
fallbacks:

    static void
    encode_SET_FIELD(const struct ofpact_set_field *sf,
                     enum ofp_version ofp_version, struct ofpbuf *out)
    {
        if (ofp_version >= OFP15_VERSION) {
            /* OF1.5+ only has Set-Field (reg_load is redundant so we drop it
             * entirely). */
            set_field_to_set_field(sf, ofp_version, out);
        } else if (sf->ofpact.raw == NXAST_RAW_REG_LOAD ||
                   sf->ofpact.raw == NXAST_RAW_REG_LOAD2) {
            /* It came in as reg_load, send it out the same way. */
            set_field_to_nxast(sf, out);
        } else if (ofp_version < OFP12_VERSION) {
            /* OpenFlow 1.0 and 1.1 don't have Set-Field. */
            set_field_to_legacy_openflow(sf, ofp_version, out);
        } else if (is_all_ones(ofpact_set_field_mask(sf), sf->field->n_bytes)) {
            /* We're encoding to OpenFlow 1.2, 1.3, or 1.4.  The action sets an
             * entire field, so encode it as OFPAT_SET_FIELD. */
            set_field_to_set_field(sf, ofp_version, out);
        } else {
            /* We're encoding to OpenFlow 1.2, 1.3, or 1.4.  The action cannot be
             * encoded as OFPAT_SET_FIELD because it does not set an entire field,
             * so encode it as reg_load. */
            set_field_to_nxast(sf, out);
        }
    }

And even for the instructions we're talking about now, there are
fallbacks for OF1.0, which doesn't have instructions, so that we can
encode them as OF1.0 extension actions instead:

    static void
    encode_WRITE_METADATA(const struct ofpact_metadata *metadata,
                          enum ofp_version ofp_version, struct ofpbuf *out)
    {
        if (ofp_version == OFP10_VERSION) {
            struct nx_action_write_metadata *nawm;

            nawm = put_NXAST_WRITE_METADATA(out);
            nawm->metadata = metadata->metadata;
            nawm->mask = metadata->mask;
        } else {
            struct ofp11_instruction_write_metadata *oiwm;

            oiwm = instruction_put_OFPIT11_WRITE_METADATA(out);
            oiwm->metadata = metadata->metadata;
            oiwm->metadata_mask = metadata->mask;
        }
    }

...

    static void
    encode_GOTO_TABLE(const struct ofpact_goto_table *goto_table,
                      enum ofp_version ofp_version, struct ofpbuf *out)
    {
        if (ofp_version == OFP10_VERSION) {
            struct nx_action_resubmit *nar;

            nar = put_NXAST_RESUBMIT_TABLE(out);
            nar->table = goto_table->table_id;
            nar->in_port = htons(ofp_to_u16(OFPP_IN_PORT));
        } else {
            struct ofp11_instruction_goto_table *oigt;

            oigt = instruction_put_OFPIT11_GOTO_TABLE(out);
            oigt->table_id = goto_table->table_id;
            memset(oigt->pad, 0, sizeof oigt->pad);
        }
    }

What is lacking is a fallback for OF1.1+, which does have instructions,
in the case where we're encoding something that can't use the
instructions but must use actions instead.  That's the case here.  It's
a weird case and I don't know about it elsewhere.

There is another possibility for bugs here.  The actions encoder tries
really hard to encode any action in any version of OpenFlow, with lots
of extensions, etc., to fill in the natural gaps.  However, there are
probably omissions and bugs.  Probably, some controller could add an
action with some version of OpenFlow and, if it used a different version
of OpenFlow for monitoring, later get back a continuation packet-in with
frozen actions that were subtly different because it was encoded in a
different OpenFlow version.  I don't have a perfect solution in mind for
that.  It's a corner case, since "normal" controllers only pick one
particular OpenFlow version.