On 11 November 2015 at 14:20, Jarno Rajahalme <ja...@ovn.org> wrote:
> With one comment to consider below:
>
> Acked-by: Jarno Rajahalme <ja...@ovn.org>
>
>> On Nov 11, 2015, at 11:39 AM, Joe Stringer <joestrin...@nicira.com> wrote:
>>
>> When inserting rules that match on connection tracking fields, datapath
>> support must be checked before allowing or denying the rule insertion.
>> Previously we only disallowed flows that had non-zero values for the
>> ct_* field, but allowed non-zero masks. This meant that, eg:
>>
>> ct_state=-trk,...
>>
>> Would be allowed, while
>>
>> ct_state=+trk,...
>>
>> Would be disallowed, due to lack of datapath support.
>>
>> Fix this by performing the check on masks instead of the flows.
>>
>> Reported-by: Ravindra Kenchappa <ravindra.kencha...@hpe.com>
>> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
>> ---
>> ofproto/ofproto-dpif.c | 32 ++++++++++++++------------------
>> 1 file changed, 14 insertions(+), 18 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index ab1b6a2f7d8e..ee2d267ab7b8 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -4014,30 +4014,26 @@ rule_dealloc(struct rule *rule_)
>> static enum ofperr
>> rule_check(struct rule *rule)
>> {
>> + struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto);
>> + const struct odp_support *support;
>
>> uint16_t ct_state, ct_zone;
>> ovs_u128 ct_label;
>> uint32_t ct_mark;
>>
>> - ct_state = MINIFLOW_GET_U16(rule->cr.match.flow, ct_state);
>> - ct_zone = MINIFLOW_GET_U16(rule->cr.match.flow, ct_zone);
>> - ct_mark = MINIFLOW_GET_U32(rule->cr.match.flow, ct_mark);
>> - ct_label = MINIFLOW_GET_U128(rule->cr.match.flow, ct_label);
>> + support = &ofproto_dpif_get_support(ofproto)->odp;
>> + ct_state = MINIFLOW_GET_U16(&rule->cr.match.mask->masks, ct_state);
>> + ct_zone = MINIFLOW_GET_U16(&rule->cr.match.mask->masks, ct_zone);
>> + ct_mark = MINIFLOW_GET_U32(&rule->cr.match.mask->masks, ct_mark);
>> + ct_label = MINIFLOW_GET_U128(&rule->cr.match.mask->masks, ct_label);
>>
>> - if (ct_state || ct_zone || ct_mark
>> - || !ovs_u128_is_zero(&ct_label)) {
>> - struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto);
>> - const struct odp_support *support =
>> &ofproto_dpif_get_support(ofproto)->odp;
>> -
>> - if ((ct_state && !support->ct_state)
>> - || (ct_zone && !support->ct_zone)
>> - || (ct_mark && !support->ct_mark)
>> - || (!ovs_u128_is_zero(&ct_label) && !support->ct_label)) {
>> - return OFPERR_OFPBMC_BAD_FIELD;
>> - }
>> - if (ct_state & CS_UNSUPPORTED_MASK) {
>> - return OFPERR_OFPBMC_BAD_MASK;
>> - }
>> + if ((ct_state && !support->ct_state)
>
> How about first checking the support, and then getting the miniflow data only
> if needed? I see that you still need to check the state for unsupported bits,
> but most other cases would get rid of the stack copies altogether. Also
> vs_u128_is_zero, if it would take it’s argument by value.
Thanks for the review. I figured the former could be in a separate
patch (3/6). The latter also could be submitted separately.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
