diff mbox

[ovs-dev,3/3] ofproto-dpif: Validate ct action support.

Message ID 1446926737-56001-3-git-send-email-joestringer@nicira.com
State Superseded, archived
Headers show

Commit Message

Joe Stringer Nov. 7, 2015, 8:05 p.m. UTC 
Disallow installing rules that execute ct() if conntrack is unsupported
in the datapath.

Reported-by: Ravindra Kenchappa <ravindra.kenchappa@hpe.com>
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 ofproto/ofproto-dpif.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

Comments

Jarno Rajahalme Nov. 10, 2015, 1:25 a.m. UTC | #1 
> On Nov 7, 2015, at 12:05 PM, Joe Stringer <joestringer@nicira.com> wrote:
> 
> Disallow installing rules that execute ct() if conntrack is unsupported
> in the datapath.
> 
> Reported-by: Ravindra Kenchappa <ravindra.kenchappa@hpe.com>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
> ofproto/ofproto-dpif.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 2f75b93d9694..e09c28bb15ed 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4048,6 +4048,44 @@ check_flow(const struct ofproto_dpif *ofproto, const struct miniflow *flow,
> }
> 
> 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;
> +}


We already loop through the actions for a similar purpose in ofproto_check_ofpacts(). Maybe make something like is_action_supported() accessible via the ofproto class and call that for the conntrack action from ofproto_check_ofpacts()?

> +
> +static enum ofperr
> rule_check(struct rule *rule)
> {
>     struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto);
> @@ -4057,7 +4095,11 @@ rule_check(struct rule *rule)
>     if (err) {
>         return err;
>     }
> -    return check_flow(ofproto, &rule->cr.match.mask->masks, true);
> +    err = check_flow(ofproto, &rule->cr.match.mask->masks, true);
> +    if (err) {
> +        return err;
> +    }
> +    return check_actions(ofproto, rule->actions);
> }
> 
> static enum ofperr
> -- 
> 2.1.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Joe Stringer Nov. 10, 2015, 9:56 p.m. UTC | #2 
On 9 November 2015 at 17:25, Jarno Rajahalme <jarno@ovn.org> wrote:
>
>> On Nov 7, 2015, at 12:05 PM, Joe Stringer <joestringer@nicira.com> wrote:
>>
>> Disallow installing rules that execute ct() if conntrack is unsupported
>> in the datapath.
>>
>> Reported-by: Ravindra Kenchappa <ravindra.kenchappa@hpe.com>
>> Signed-off-by: Joe Stringer <joestringer@nicira.com>
>> ---
>> ofproto/ofproto-dpif.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 2f75b93d9694..e09c28bb15ed 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -4048,6 +4048,44 @@ check_flow(const struct ofproto_dpif *ofproto, const struct miniflow *flow,
>> }
>>
>> 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;
>> +}
>
>
> We already loop through the actions for a similar purpose in ofproto_check_ofpacts(). Maybe make something like is_action_supported() accessible via the ofproto class and call that for the conntrack action from ofproto_check_ofpacts()?

Makes sense, I'll rework this patch do to it like this.
Joe Stringer Nov. 11, 2015, 6:21 p.m. UTC | #3 
On 10 November 2015 at 13:56, Joe Stringer <joestringer@nicira.com> wrote:
> On 9 November 2015 at 17:25, Jarno Rajahalme <jarno@ovn.org> wrote:
>>
>>> On Nov 7, 2015, at 12:05 PM, Joe Stringer <joestringer@nicira.com> wrote:
>>>
>>> Disallow installing rules that execute ct() if conntrack is unsupported
>>> in the datapath.
>>>
>>> Reported-by: Ravindra Kenchappa <ravindra.kenchappa@hpe.com>
>>> Signed-off-by: Joe Stringer <joestringer@nicira.com>
>>> ---
>>> ofproto/ofproto-dpif.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 43 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index 2f75b93d9694..e09c28bb15ed 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -4048,6 +4048,44 @@ check_flow(const struct ofproto_dpif *ofproto, const struct miniflow *flow,
>>> }
>>>
>>> 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;
>>> +}
>>
>>
>> We already loop through the actions for a similar purpose in ofproto_check_ofpacts(). Maybe make something like is_action_supported() accessible via the ofproto class and call that for the conntrack action from ofproto_check_ofpacts()?
>
> Makes sense, I'll rework this patch do to it like this.

Taking another look at this, I'm on the fence about whether it's
better conceptually than what we have currently. For one, it
effectively shifts some of the error checking that is currently part
of the ofproto's ->rule_construct() method into a separate method on
the ofproto class, to avoid an extra iteration on the actions. This
spreads the error checking across more locations, and makes the rule
lifecycle slightly more complex (actions must be checked before rule
construction).

I'll send out the series with this feedback applied anyway, and we can
look at it again.
Jarno Rajahalme Nov. 11, 2015, 9:52 p.m. UTC | #4 
> On Nov 11, 2015, at 10:21 AM, Joe Stringer <joestringer@nicira.com> wrote:
> 
> On 10 November 2015 at 13:56, Joe Stringer <joestringer@nicira.com> wrote:
>> On 9 November 2015 at 17:25, Jarno Rajahalme <jarno@ovn.org> wrote:
>>> 
>>>> On Nov 7, 2015, at 12:05 PM, Joe Stringer <joestringer@nicira.com> wrote:
>>>> 
>>>> Disallow installing rules that execute ct() if conntrack is unsupported
>>>> in the datapath.
>>>> 
>>>> Reported-by: Ravindra Kenchappa <ravindra.kenchappa@hpe.com>
>>>> Signed-off-by: Joe Stringer <joestringer@nicira.com>
>>>> ---
>>>> ofproto/ofproto-dpif.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 43 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>>> index 2f75b93d9694..e09c28bb15ed 100644
>>>> --- a/ofproto/ofproto-dpif.c
>>>> +++ b/ofproto/ofproto-dpif.c
>>>> @@ -4048,6 +4048,44 @@ check_flow(const struct ofproto_dpif *ofproto, const struct miniflow *flow,
>>>> }
>>>> 
>>>> 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;
>>>> +}
>>> 
>>> 
>>> We already loop through the actions for a similar purpose in ofproto_check_ofpacts(). Maybe make something like is_action_supported() accessible via the ofproto class and call that for the conntrack action from ofproto_check_ofpacts()?
>> 
>> Makes sense, I'll rework this patch do to it like this.
> 
> Taking another look at this, I'm on the fence about whether it's
> better conceptually than what we have currently. For one, it
> effectively shifts some of the error checking that is currently part
> of the ofproto's ->rule_construct() method into a separate method on
> the ofproto class, to avoid an extra iteration on the actions. This
> spreads the error checking across more locations, and makes the rule
> lifecycle slightly more complex (actions must be checked before rule
> construction).
> 

I would do the check call only for actions we know might not be supported.

  Jarno

> I'll send out the series with this feedback applied anyway, and we can
> look at it again.
diff mbox

Patch

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 2f75b93d9694..e09c28bb15ed 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4048,6 +4048,44 @@  check_flow(const struct ofproto_dpif *ofproto, const struct miniflow *flow,
 }
 
 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);
@@ -4057,7 +4095,11 @@  rule_check(struct rule *rule)
     if (err) {
         return err;
     }
-    return check_flow(ofproto, &rule->cr.match.mask->masks, true);
+    err = check_flow(ofproto, &rule->cr.match.mask->masks, true);
+    if (err) {
+        return err;
+    }
+    return check_actions(ofproto, rule->actions);
 }
 
 static enum ofperr