| Message ID | 1446926737-56001-1-git-send-email-joestringer@nicira.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
> On Nov 7, 2015, at 12:05 PM, Joe Stringer <joestringer@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@hpe.com> > Signed-off-by: Joe Stringer <joestringer@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) } } > 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 > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
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 <jarno@ovn.org> wrote: > > >> On Nov 7, 2015, at 12:05 PM, Joe Stringer <joestringer@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@hpe.com> >> Signed-off-by: Joe Stringer <joestringer@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) } } > > >> 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 >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >
On 9 November 2015 at 17:17, Jarno Rajahalme <jarno@ovn.org> wrote: > >> On Nov 7, 2015, at 12:05 PM, Joe Stringer <joestringer@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@hpe.com> >> Signed-off-by: Joe Stringer <joestringer@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? >> - 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.
> On Nov 10, 2015, at 1:52 PM, Joe Stringer <joestringer@nicira.com> wrote: > > On 9 November 2015 at 17:17, Jarno Rajahalme <jarno@ovn.org <mailto:jarno@ovn.org>> wrote: >> >>> On Nov 7, 2015, at 12:05 PM, Joe Stringer <joestringer@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@hpe.com> >>> Signed-off-by: Joe Stringer <joestringer@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
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; } 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; + 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; } - 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 {
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@hpe.com> Signed-off-by: Joe Stringer <joestringer@nicira.com> --- ofproto/ofproto-dpif.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-)