With one comment to consider below: Acked-by: Jarno Rajahalme <[email protected]>
> On Nov 11, 2015, at 11:39 AM, 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 performing the check on masks instead of the flows. > > Reported-by: Ravindra Kenchappa <[email protected]> > Signed-off-by: Joe Stringer <[email protected]> > --- > 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 > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
