[ovs-dev] [PATCH 1/3] ofproto-dpif: Validate ct_* field masks.

[Jarno Rajahalme]

> On Nov 10, 2015, at 1:52 PM, Joe Stringer <joestringer at nicira.com> wrote:
> 
> On 9 November 2015 at 17:17, Jarno Rajahalme <jarno at ovn.org <mailto:jarno at ovn.org>> wrote:
>> 
>>> On Nov 7, 2015, at 12:05 PM, Joe Stringer <joestringer at 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 denying nonzero masks whenever there is no datapath support
>>> for connection tracking.
>>> 
>>> Reported-by: Ravindra Kenchappa <ravindra.kenchappa at hpe.com>
>>> Signed-off-by: Joe Stringer <joestringer at nicira.com>
>>> ---
>>> ofproto/ofproto-dpif.c | 33 ++++++++++++++++++++++++---------
>>> 1 file changed, 24 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index 5cc64cbca1f5..2f75b93d9694 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -4012,40 +4012,55 @@ rule_dealloc(struct rule *rule_)
>>> }
>>> 
>>> static enum ofperr
>>> -rule_check(struct rule *rule)
>>> +check_flow(const struct ofproto_dpif *ofproto, const struct miniflow *flow,
>>> +           bool mask)
>>> {
>>> +    ovs_u128 ct_label = { { 0, 0 } };
>>>    uint16_t ct_state, ct_zone;
>>>    const ovs_u128 *labelp;
>>> -    ovs_u128 ct_label = { { 0, 0 } };
>>>    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);
>>> -    labelp = MINIFLOW_GET_U128_PTR(rule->cr.match.flow, ct_label);
>>> +    ct_state = MINIFLOW_GET_U16(flow, ct_state);
>>> +    ct_zone = MINIFLOW_GET_U16(flow, ct_zone);
>>> +    ct_mark = MINIFLOW_GET_U32(flow, ct_mark);
>>> +    labelp = MINIFLOW_GET_U128_PTR(flow, ct_label);
>>>    if (labelp) {
>>>        ct_label = *labelp;
>>>    }
>>> 
>> 
>> Checking MINIFLOW_GET_U128_PTR(), I think it has a problem if only half of it is present in the miniflow/mask, which would be typical when you match on e.g. one bit. How about this instead:
>> 
>> #define MINIFLOW_GET_U128(FLOW, FIELD)                                  \
>>    (ovs_u128) { .u64 = {                                               \
>>            (MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD)) ?            \
>>             *miniflow_get__(FLOW, FLOW_U64_OFFSET(FIELD)) : 0),        \
>>            (MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD) + 1) ?        \
>>             *miniflow_get__(FLOW, FLOW_U64_OFFSET(FIELD) + 1) : 0) } }
> 
> OK, I'll put this refactor in a separate patch.
> 
>>>    if (ct_state || ct_zone || ct_mark
>>>        || !ovs_u128_is_zero(&ct_label)) {
>> 
>> 
>> This function would be a bit cheaper in the eventuality where the datapath supports all the features if we first check if any of the features is missing, and then check for the presence of non-zero miniflow values.
> 
> Is there any reason to hold out on this, or do you think we should
> just do it now?
> 

Do it now, so the implementation automatically gets more efficient as we get the features in the datapath.

>>> -        struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto);
>>> -        const struct odp_support *support = &ofproto_dpif_get_support(ofproto)->odp;
>>> +        const struct odp_support *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;
>> 
>> Is it OK to return BAD_FIELD if we are checking a mask? Maybe this function should just return a bool and the caller then returns the error code?
> 
> As per your other message, we can simplify this patch by just checking
> against the mask instead.
> 
> In regards to BAD_FIELD, I suppose it's a little more accurate for us
> to use OFPERR_OFPBMC_BAD_MASK as we recognize the field but don't
> support any possible mask. I can update this as well when I resubmit.

OK.

  Jarno