On 9 November 2015 at 17:17, Jarno Rajahalme <[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?
>> - 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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
