| Message ID | 1447270794-21103-3-git-send-email-joestringer@nicira.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
With one comment to consider below: Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Nov 11, 2015, at 11:39 AM, 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 performing the check on masks instead of the flows. > > Reported-by: Ravindra Kenchappa <ravindra.kenchappa@hpe.com> > Signed-off-by: Joe Stringer <joestringer@nicira.com> > --- > ofproto/ofproto-dpif.c | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index ab1b6a2f7d8e..ee2d267ab7b8 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -4014,30 +4014,26 @@ rule_dealloc(struct rule *rule_) > static enum ofperr > rule_check(struct rule *rule) > { > + struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto); > + const struct odp_support *support; > uint16_t ct_state, ct_zone; > ovs_u128 ct_label; > 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); > - ct_label = MINIFLOW_GET_U128(rule->cr.match.flow, ct_label); > + support = &ofproto_dpif_get_support(ofproto)->odp; > + ct_state = MINIFLOW_GET_U16(&rule->cr.match.mask->masks, ct_state); > + ct_zone = MINIFLOW_GET_U16(&rule->cr.match.mask->masks, ct_zone); > + ct_mark = MINIFLOW_GET_U32(&rule->cr.match.mask->masks, ct_mark); > + ct_label = MINIFLOW_GET_U128(&rule->cr.match.mask->masks, ct_label); > > - 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; > - > - 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) { > - return OFPERR_OFPBMC_BAD_MASK; > - } > + if ((ct_state && !support->ct_state) How about first checking the support, and then getting the miniflow data only if needed? I see that you still need to check the state for unsupported bits, but most other cases would get rid of the stack copies altogether. Also vs_u128_is_zero, if it would take it’s argument by value. > + || (ct_state & CS_UNSUPPORTED_MASK) > + || (ct_zone && !support->ct_zone) > + || (ct_mark && !support->ct_mark) > + || (!ovs_u128_is_zero(&ct_label) && !support->ct_label)) { > + return OFPERR_OFPBMC_BAD_MASK; > } > + > return 0; > } > > -- > 2.1.4 >
On 11 November 2015 at 14:20, Jarno Rajahalme <jarno@ovn.org> wrote: > With one comment to consider below: > > Acked-by: Jarno Rajahalme <jarno@ovn.org> > >> On Nov 11, 2015, at 11:39 AM, 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 performing the check on masks instead of the flows. >> >> Reported-by: Ravindra Kenchappa <ravindra.kenchappa@hpe.com> >> Signed-off-by: Joe Stringer <joestringer@nicira.com> >> --- >> ofproto/ofproto-dpif.c | 32 ++++++++++++++------------------ >> 1 file changed, 14 insertions(+), 18 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index ab1b6a2f7d8e..ee2d267ab7b8 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -4014,30 +4014,26 @@ rule_dealloc(struct rule *rule_) >> static enum ofperr >> rule_check(struct rule *rule) >> { >> + struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto); >> + const struct odp_support *support; > >> uint16_t ct_state, ct_zone; >> ovs_u128 ct_label; >> 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); >> - ct_label = MINIFLOW_GET_U128(rule->cr.match.flow, ct_label); >> + support = &ofproto_dpif_get_support(ofproto)->odp; >> + ct_state = MINIFLOW_GET_U16(&rule->cr.match.mask->masks, ct_state); >> + ct_zone = MINIFLOW_GET_U16(&rule->cr.match.mask->masks, ct_zone); >> + ct_mark = MINIFLOW_GET_U32(&rule->cr.match.mask->masks, ct_mark); >> + ct_label = MINIFLOW_GET_U128(&rule->cr.match.mask->masks, ct_label); >> >> - 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; >> - >> - 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) { >> - return OFPERR_OFPBMC_BAD_MASK; >> - } >> + if ((ct_state && !support->ct_state) > > How about first checking the support, and then getting the miniflow data only if needed? I see that you still need to check the state for unsupported bits, but most other cases would get rid of the stack copies altogether. Also vs_u128_is_zero, if it would take it’s argument by value. Thanks for the review. I figured the former could be in a separate patch (3/6). The latter also could be submitted separately.
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index ab1b6a2f7d8e..ee2d267ab7b8 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4014,30 +4014,26 @@ rule_dealloc(struct rule *rule_) static enum ofperr rule_check(struct rule *rule) { + struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto); + const struct odp_support *support; uint16_t ct_state, ct_zone; ovs_u128 ct_label; 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); - ct_label = MINIFLOW_GET_U128(rule->cr.match.flow, ct_label); + support = &ofproto_dpif_get_support(ofproto)->odp; + ct_state = MINIFLOW_GET_U16(&rule->cr.match.mask->masks, ct_state); + ct_zone = MINIFLOW_GET_U16(&rule->cr.match.mask->masks, ct_zone); + ct_mark = MINIFLOW_GET_U32(&rule->cr.match.mask->masks, ct_mark); + ct_label = MINIFLOW_GET_U128(&rule->cr.match.mask->masks, ct_label); - 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; - - 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) { - return OFPERR_OFPBMC_BAD_MASK; - } + if ((ct_state && !support->ct_state) + || (ct_state & CS_UNSUPPORTED_MASK) + || (ct_zone && !support->ct_zone) + || (ct_mark && !support->ct_mark) + || (!ovs_u128_is_zero(&ct_label) && !support->ct_label)) { + return OFPERR_OFPBMC_BAD_MASK; } + return 0; }
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 performing the check on masks instead of the flows. Reported-by: Ravindra Kenchappa <ravindra.kenchappa@hpe.com> Signed-off-by: Joe Stringer <joestringer@nicira.com> --- ofproto/ofproto-dpif.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-)