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

[Ilya Maximets]

On 7/21/21 12:52 AM, Ben Pfaff wrote:
> 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.

OK.  Thanks for the explanation!  That make sense.  For some reason I
mixed up ofpacts with OpenFlow actions in my head and so I missed the
translation layer that handles conversion to the appropriate OpenFlow
version.

TBH, it's really hard to track the call chain that connects the
freeze_unroll_actions() and encode_SET_FIELD().  I still didn't figure
that out by reading the code.  I guess, the only option for me is to
attach debugger, break the execution and look at the stack, unless you
can draw a call tree for me.

In the end, it's hard for me to review the logic behind this change,
but the patch seems to do what described in the commit message and
the implementation looks correct, tests work fine.  With that in mind:

Acked-by: Ilya Maximets <i.maximets at ovn.org>

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