[ovs-dev] [PATCH] ofproto-dpif: fix issue with non-reversible actions on a patch ports

Eelco Chaudron echaudro at redhat.com
Tue Jun 15 13:47:58 UTC 2021



On 14 Jun 2021, at 20:15, Ilya Maximets wrote:

> On 6/14/21 4:28 PM, Eelco Chaudron wrote:
>> For patch ports, the is_last_action value is not propagated and is
>> always set to true. This causes non-reversible actions to modify the
>> packet, and the original content is not preserved when processing
>> the remaining actions.
>>
>> This patch propagates the is_last_action flag for patch port related
>> actions. It also includes a test case
>>
>> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
>> ---
>
> Hi, Eelco.  I didn't review the code, but this change fails a lot
> of tests in our CI: https://github.com/ovsrobot/ovs/actions/runs/936204455
> Basically it introduces extra clone() actions where was no clones
> before and where these clones are not really needed.


Tried to understand a bit more the is_last_action code, and I think the original patch, feee58b95 ("ofproto-dpif-xlate: Keep track of the last action”), was not right.

In do_xlate_actions() the following code determines if it’s the last action, i.e. “last = “:


    OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
        struct ofpact_controller *controller;
        const struct ofpact_metadata *metadata;
        const struct ofpact_set_field *set_field;
        const struct mf_field *mf;
        bool last = is_last_action && ofpact_last(a, ofpacts, ofpacts_len)
                    && !ctx->action_set.size;



I think it should be as follows:


@@ -6744,7 +6744,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
         const struct ofpact_set_field *set_field;
         const struct mf_field *mf;
         bool last = is_last_action && ofpact_last(a, ofpacts, ofpacts_len)
-                    && ctx->action_set.size;
+                    && !ctx->action_set.size;

         if (ctx->error) {
             break;

As action_set is used to store OpenFlow write_action actions, which set, this is not the last action at the end of the pipeline.

Does this look good to you? With this change all (except one) tests pass. The one failure is also a bug, which is fixed easily.


Let me know, and I sent a v2 fixing this all…

Cheers,

Eelco



More information about the dev mailing list