[ovs-discuss] Set-field instruction

[John Hurley]

Hi Ben/all,

To let you know I got to the bottom of the issue I reported.

Basically the ofpacts_put_openflow11_instructions() function does not like
being called in isolation - i.e. it expects to be called when creating the
entire flow mod message and not just the instructions.

This is due to the version check in the nxm_reg_load_to_nxast() function
trying to cast a NULL value:

struct ofp_header *oh = (struct ofp_header *)openflow->l2;
        switch(oh->version) {

I was calling this in my ofproto-provider to decode the ofpact field in the
'rule' struct that is passed on when a new flow is to added.

A fix for this would be to remove the above version check which I don't
think is needed as it is only called from functions that should be OF 1.2
or 1.3.
However, instead of adapting the OVS lib code I added a fake ofp_header to
the ofpbuf passed to ofpacts_put_openflow11_instructions() which matches
the version check. This unwraps the ofpacts correctly. I then removed this
fake header to leave only the instructions in the buffer.

Thanks
John



On Mon, Sep 2, 2013 at 10:04 AM, John Hurley <john.hurley at netronome.com>wrote:

> The issue is triggered by sending an OF message with the action
> 'set-field' - seems to fail with whatever field I set.
>
> When this is packaged to OVS structs the ofpact_from_openflow11() function
> is called (lib/ofp-actions.c) to create the ofpacts struct.  This can
> handle the set-field action in the following segment:
>
> case OFPUTIL_OFPAT12_SET_FIELD:
>         return nxm_reg_load_from_openflow12_set_field(
>             (const struct ofp12_action_set_field *)a, out);
>
>
> When the rule is passsed to ofproto-provider I use the
> ofpacts_put_openflow11_instructions() function to extract the OF
> instructions data from the passed ofpacts struct. This works fine for all
> other actions except the set field. It seems that this has not been
> implemented but maybe there is another way around this. This is a section
> from ofpact_to_openflow11 which is called from
> ofpacts_put_openflow11_instructions()....
>
> case OFPACT_POP_MPLS:
>         ofputil_put_OFPAT11_POP_MPLS(out)->ethertype =
>             ofpact_get_POP_MPLS(a)->ethertype;
>
>         break;
>
>     case OFPACT_CLEAR_ACTIONS:
>     case OFPACT_GOTO_TABLE:
>         NOT_REACHED();
>
>     case OFPACT_CONTROLLER:
>     case OFPACT_OUTPUT_REG:
>     case OFPACT_BUNDLE:
>     case OFPACT_REG_MOVE:
>     case OFPACT_REG_LOAD:
>     case OFPACT_STACK_PUSH:
>     case OFPACT_STACK_POP:
>     case OFPACT_SET_TUNNEL:
>     case OFPACT_POP_QUEUE:
>     case OFPACT_FIN_TIMEOUT:
>     case OFPACT_RESUBMIT:
>     case OFPACT_LEARN:
>     case OFPACT_MULTIPATH:
>     case OFPACT_NOTE:
>     case OFPACT_EXIT:
>     case OFPACT_SAMPLE:
>         ofpact_to_nxast(a, out);
>         break;
>
> where reg_load is hit when by the set-field ofpacts parsing.  So when it
> hits this case it ends up calling ofpact_to_nxast() which causes the system
> to crash. is this the correct function that should be hit or is the
> REG_LOAD (set-field) case not yet supported?
>
>
> Thanks for responding,
>
> John
>
>
> On Sat, Aug 31, 2013 at 3:59 PM, John Hurley <john.hurley at netronome.com>wrote:
>
>> The issue is triggered by sending an OF message with the action
>> 'set-field' - seems to fail with whatever field I set.
>>
>> When this is packaged to OVS structs the ofpact_from_openflow11()
>> function is called (lib/ofp-actions.c) to create the ofpacts struct.  This
>> can handle the set-field action in the following segment:
>>
>> case OFPUTIL_OFPAT12_SET_FIELD:
>>         return nxm_reg_load_from_openflow12_set_field(
>>             (const struct ofp12_action_set_field *)a, out);
>>
>>
>> When the rule is passsed to ofproto-provider I use the
>> ofpacts_put_openflow11_instructions() function to extract the OF
>> instructions data from the passed ofpacts struct. This works fine for all
>> other actions except the set field. It seems that this has not been
>> implemented but maybe there is another way around this. This is a section
>> from ofpact_to_openflow11 which is called from
>> ofpacts_put_openflow11_instructions()....
>>
>> case OFPACT_POP_MPLS:
>>         ofputil_put_OFPAT11_POP_MPLS(out)->ethertype =
>>             ofpact_get_POP_MPLS(a)->ethertype;
>>
>>         break;
>>
>>     case OFPACT_CLEAR_ACTIONS:
>>     case OFPACT_GOTO_TABLE:
>>         NOT_REACHED();
>>
>>     case OFPACT_CONTROLLER:
>>     case OFPACT_OUTPUT_REG:
>>     case OFPACT_BUNDLE:
>>     case OFPACT_REG_MOVE:
>>     case OFPACT_REG_LOAD:
>>     case OFPACT_STACK_PUSH:
>>     case OFPACT_STACK_POP:
>>     case OFPACT_SET_TUNNEL:
>>     case OFPACT_POP_QUEUE:
>>     case OFPACT_FIN_TIMEOUT:
>>     case OFPACT_RESUBMIT:
>>     case OFPACT_LEARN:
>>     case OFPACT_MULTIPATH:
>>     case OFPACT_NOTE:
>>     case OFPACT_EXIT:
>>     case OFPACT_SAMPLE:
>>         ofpact_to_nxast(a, out);
>>         break;
>>
>> where reg_load is hit when by the set-field ofpacts parsing.  So when it
>> hits this case it ends up calling ofpact_to_nxast() which causes the system
>> to crash. is this the correct function that should be hit or is the
>> REG_LOAD (set-field) case not yet supported?
>>
>>
>> Thanks for responding,
>>
>> John
>>
>>
>> On Fri, Aug 30, 2013 at 5:42 PM, Ben Pfaff <blp at nicira.com> wrote:
>>
>>> On Fri, Aug 30, 2013 at 10:22:16AM +0100, John Hurley wrote:
>>> > I have been playing about with the latest OVS and writing an
>>> > ofproto-provider module. However, when dealing with some OF 1.3
>>> actions I
>>> > am finding issues. The conversion of the 'ofpacts' struct back to
>>> openflow
>>> > is crashing when 1.2+ actions like 'set-field' are used. The approach
>>> works
>>> > fine for 1.1 actions.
>>> >
>>> > I am using the ofpacts_put_openflow11_instructions() function to do
>>> this.
>>> >
>>> > Digging into the ovs code a bit I see that the set-field action is
>>> handled
>>> > when packaging the actions in ofpact_from_openflow11() but is not
>>> extracted
>>> > again in the above (actually causes ovs to seg fault in
>>> > ofpact_to_openflow11() with no break after matching type
>>> OFPACT_REG_LOAD).
>>> >
>>> > Is this action simply not supported at the current time (should we
>>> expect
>>> > an ofpacts_put_openflow13_instructions() in the future) or am I missing
>>> > something that allows such actions to be handled in OVS as it currently
>>> > stands?
>>>
>>> This sounds like a bug, but I don't see an obvious problem in the
>>> code.  Can you explain how to trigger the issue or where there's a
>>> problem in the code?
>>>
>>> Thanks,
>>>
>>> Ben.
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://openvswitch.org/pipermail/ovs-discuss/attachments/20130905/f17f6208/attachment.html>