> On Nov 10, 2015, at 1:52 PM, Joe Stringer <[email protected]> wrote:
>
> On 9 November 2015 at 17:17, Jarno Rajahalme <[email protected]
> <mailto:[email protected]>> wrote:
>>
>>> On Nov 7, 2015, at 12:05 PM, Joe Stringer <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Joe Stringer <[email protected]>
>>> ---
>>> 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
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
