Joe, It just occurred to me that here we want to check if there is a match on a non-supported field. For that checking the mask alone is sufficient, so that this can be simplified quite a bit.
Jarno > On Nov 9, 2015, at 5:17 PM, 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) } } > > >> 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. > >> - 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? > >> } >> - if (ct_state & CS_UNSUPPORTED_MASK) { >> + if (mask && ct_state & CS_UNSUPPORTED_MASK) { >> return OFPERR_OFPBMC_BAD_MASK; >> } >> } >> + >> return 0; >> } >> >> static enum ofperr >> +rule_check(struct rule *rule) >> +{ >> + struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto); >> + enum ofperr err; >> + >> + err = check_flow(ofproto, rule->cr.match.flow, false); >> + if (err) { >> + return err; >> + } >> + return check_flow(ofproto, &rule->cr.match.mask->masks, true); >> +} >> + >> +static enum ofperr >> rule_construct(struct rule *rule_) >> OVS_NO_THREAD_SAFETY_ANALYSIS >> { >> -- >> 2.1.4 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
