On Oct 25, 2013, at 1:56 PM, Ben Pfaff <b...@nicira.com> wrote:

> On Fri, Oct 25, 2013 at 01:44:17PM -0700, Jarno Rajahalme wrote:
>> 
>> On Oct 25, 2013, at 1:32 PM, Ben Pfaff <b...@nicira.com> wrote:
>> 
>>> On Thu, Oct 24, 2013 at 02:08:24PM -0700, Jarno Rajahalme wrote:
>>>> 
>>>> 
>>>> On Oct 21, 2013, at 3:52 PM, Ben Pfaff <b...@nicira.com> wrote:
>>>> 
>>>>> Commit e3b5693319c (Fix table checking for goto table instruction.) moved
>>>>> action checking into modify_flows__(), for good reason, but as a side
>>>>> effect made modify_flows__() abandon and never commit the ofopgroup that 
>>>>> it
>>>>> started, if action checking failed.  This commit fixes the problem.
>>>>> 
>>>>> The following commands, run under "make sandbox", illustrate the problem.
>>>>> Without this change, the final command hangs because the barrier request
>>>>> that ovs-ofctl sends never gets a response (because barriers wait for all
>>>>> ofopgroups to complete, which never happens).  With this commit, the
>>>>> commands complete quickly:
>>>>> 
>>>>> ovs-vsctl add-br br0
>>>>> ovs-vsctl set bridge br0 
>>>>> protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13
>>>>> ovs-ofctl add-flow -O OpenFlow11 br0 
>>>>> table=1,action=mod_tp_dst:79,goto_table:2
>>>>> ovs-ofctl add-flow -O OpenFlow11 br0 
>>>>> table=1,action=mod_tp_dst:79,goto_table:1
>>>>> 
>>>>> Reported-by: Jarno Rajahalme <jrajaha...@vmware.com>
>>>>> Signed-off-by: Ben Pfaff <b...@nicira.com>
>>>>> ---
>>>>> ofproto/ofproto.c |   19 ++++++++++++-------
>>>>> tests/ofproto.at  |   26 ++++++++++++++++++++++++++
>>>>> 2 files changed, 38 insertions(+), 7 deletions(-)
>>>>> 
>>>>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>>>>> index f67e1fb..8dba732 100644
>>>>> --- a/ofproto/ofproto.c
>>>>> +++ b/ofproto/ofproto.c
>>>>> @@ -4041,6 +4041,18 @@ modify_flows__(struct ofproto *ofproto, struct 
>>>>> ofconn *ofconn,
>>>>>   enum ofperr error;
>>>>>   size_t i;
>>>>> 
>>>>> +    /* Verify actions before we start to modify any rules, to avoid 
>>>>> partial
>>>>> +     * flow table modifications. */
>>>>> +    for (i = 0; i < rules->n; i++) {
>>>>> +        struct rule *rule = rules->rules[i];
>>>>> +
>>>>> +        error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, 
>>>>> &fm->match.flow,
>>>>> +                              u16_to_ofp(ofproto->max_ports), 
>>>>> rule->table_id);
>>>>> +        if (error) {
>>>>> +            return error;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>> 
>>>> This fixes the problem I had, thank you!
>>>> 
>>>> While we are at this, we should use ofproto_check_ofpacts() instead
>>>> and maybe avoid repeating the same check over and over again. How
>>>> about this incremental:
>>> 
>>> Can we really avoid repeating the check?  Since I proposed this
>>> change, ofpacts_check() now checks consistency of the flow and the
>>> actions, and since the flows vary among the rules that we are
>>> checking, I imagine that some of them could be inconsistent within a
>>> single table, even if others are not.
>>> 
>> 
>> It seems to me that we are checking the new actions against the new flow
>> (both from the new flow mod) in the context of the old rule's table_id, i.e.
>> the check calls do not really vary by the rule (other than rule's table id) 
>> at all.
> 
> I don't understand yet.
> 
> Let me provide an example.  Suppose we do a flow_mod that changes all
> of the actions in table 0 from whatever they were previously to
> "mod_tp_src:80".  If the first rule whose change we validate in that
> table satisfies the prerequisites for mod_tp_src, but other rules in
> the table do not satisfy the prerequisites, then I think that we would
> allow the flow_mod to go through without noticing the problem.

But the old rule is never passed for the check, see:

       error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
                              u16_to_ofp(ofproto->max_ports), rule->table_id);

The flow mod comes in with the flow (&fm->match.flow) so the exact same 
validation is being repeated over and over again, if the rule->table_id remains 
the same.

Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to