[ovs-dev] [PATCH 2/2] ofproto-dpif: Validate ct action support.

[Joe Stringer]

Thanks, I applied this series to master.

On 2 December 2015 at 11:34, Jarno Rajahalme <jarno at ovn.org> wrote:

> LGTM,
>
> Acked-by: Jarno Rajahalme <jarno at ovn.org>
>
> > On Dec 1, 2015, at 4:17 PM, Joe Stringer <joestringer at nicira.com> wrote:
> >
> > Disallow installing rules that execute ct() if conntrack is unsupported
> > in the datapath.
> >
> > Reported-by: Ravindra Kenchappa <ravindra.kenchappa at hpe.com>
> > Signed-off-by: Joe Stringer <joestringer at nicira.com>
> > ---
> > v3: Revert back to checking as part of rule_construct().
> > v2: Refactor to add a new ofproto class action support checker.
> > v1: Initial post.
> > ---
> > ofproto/ofproto-dpif.c | 62
> +++++++++++++++++++++++++++++++++++++++++++++-----
> > 1 file changed, 56 insertions(+), 6 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 9d50e314a4bf..e76d8fe9a6a0 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -4013,24 +4013,23 @@ rule_dealloc(struct rule *rule_)
> > }
> >
> > static enum ofperr
> > -rule_check(struct rule *rule)
> > +check_mask(struct ofproto_dpif *ofproto, const struct miniflow *flow)
> > {
> > -    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;
> >
> >     support = &ofproto_dpif_get_support(ofproto)->odp;
> > -    ct_state = MINIFLOW_GET_U16(&rule->cr.match.mask->masks, ct_state);
> > +    ct_state = MINIFLOW_GET_U16(flow, ct_state);
> >     if (support->ct_state && support->ct_zone && support->ct_mark
> >         && support->ct_label) {
> >         return ct_state & CS_UNSUPPORTED_MASK ? OFPERR_OFPBMC_BAD_MASK :
> 0;
> >     }
> >
> > -    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);
> > +    ct_zone = MINIFLOW_GET_U16(flow, ct_zone);
> > +    ct_mark = MINIFLOW_GET_U32(flow, ct_mark);
> > +    ct_label = MINIFLOW_GET_U128(flow, ct_label);
> >
> >     if ((ct_state && !support->ct_state)
> >         || (ct_state & CS_UNSUPPORTED_MASK)
> > @@ -4044,6 +4043,57 @@ rule_check(struct rule *rule)
> > }
> >
> > static enum ofperr
> > +check_actions(const struct ofproto_dpif *ofproto,
> > +              const struct rule_actions *const actions)
> > +{
> > +    const struct ofpact *ofpact;
> > +
> > +    OFPACT_FOR_EACH (ofpact, actions->ofpacts, actions->ofpacts_len) {
> > +        const struct odp_support *support;
> > +        const struct ofpact_conntrack *ct;
> > +        const struct ofpact *a;
> > +
> > +        if (ofpact->type != OFPACT_CT) {
> > +            continue;
> > +        }
> > +
> > +        ct = CONTAINER_OF(ofpact, struct ofpact_conntrack, ofpact);
> > +        support = &ofproto_dpif_get_support(ofproto)->odp;
> > +
> > +        if (!support->ct_state) {
> > +            return OFPERR_OFPBAC_BAD_TYPE;
> > +        }
> > +        if ((ct->zone_imm || ct->zone_src.field) && !support->ct_zone) {
> > +            return OFPERR_OFPBAC_BAD_ARGUMENT;
> > +        }
> > +
> > +        OFPACT_FOR_EACH(a, ct->actions, ofpact_ct_get_action_len(ct)) {
> > +            const struct mf_field *dst = ofpact_get_mf_dst(a);
> > +
> > +            if (dst && ((dst->id == MFF_CT_MARK && !support->ct_mark)
> > +                        || (dst->id == MFF_CT_LABEL &&
> !support->ct_label))) {
> > +                return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
> > +            }
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static enum ofperr
> > +rule_check(struct rule *rule)
> > +{
> > +    struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto);
> > +    enum ofperr err;
> > +
> > +    err = check_mask(ofproto, &rule->cr.match.mask->masks);
> > +    if (err) {
> > +        return err;
> > +    }
> > +    return check_actions(ofproto, rule->actions);
> > +}
> > +
> > +static enum ofperr
> > rule_construct(struct rule *rule_)
> >     OVS_NO_THREAD_SAFETY_ANALYSIS
> > {
> > --
> > 2.1.4
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>