| Message ID | 1447270794-21103-7-git-send-email-joestringer@nicira.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
LGTM, However, I would urge Ben to offer his opinion to this design, Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Nov 11, 2015, at 11:39 AM, Joe Stringer <joestringer@nicira.com> wrote: > > Disallow installing rules that execute ct() if conntrack is unsupported > in the datapath. Also check different variations on the action which may > be denied, such as writing to the ct_{mark,label} fields. > > Reported-by: Ravindra Kenchappa <ravindra.kenchappa@hpe.com> > Signed-off-by: Joe Stringer <joestringer@nicira.com> > --- > ofproto/ofproto-dpif.c | 36 +++++++++++++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 8b1760c95409..d91eb255ef24 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -3698,6 +3698,40 @@ rule_expire(struct rule_dpif *rule) > } > } > > +static enum ofperr > +rule_check_action(const struct ofproto *ofproto_, const struct ofpact *ofpact) > +{ > + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); > + const struct odp_support *support; > + const struct ofpact_conntrack *ct; > + const struct ofpact *a; > + > + if (ofpact->type != OFPACT_CT) { > + return 0; > + } > + > + 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; > +} > + > int > ofproto_dpif_execute_actions__(struct ofproto_dpif *ofproto, > const struct flow *flow, > @@ -5689,7 +5723,7 @@ const struct ofproto_class ofproto_dpif_class = { > port_poll_wait, > port_is_lacp_current, > port_get_lacp_stats, > - NULL, > + rule_check_action, > NULL, /* rule_choose_table */ > rule_alloc, > rule_construct, > -- > 2.1.4 >
On Wed, Nov 11, 2015 at 02:25:20PM -0800, Jarno Rajahalme wrote: > LGTM, > > However, I would urge Ben to offer his opinion to this design, I don't object to the checking, but I think it can be done in the existing ->rule_construct().
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 8b1760c95409..d91eb255ef24 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3698,6 +3698,40 @@ rule_expire(struct rule_dpif *rule) } } +static enum ofperr +rule_check_action(const struct ofproto *ofproto_, const struct ofpact *ofpact) +{ + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); + const struct odp_support *support; + const struct ofpact_conntrack *ct; + const struct ofpact *a; + + if (ofpact->type != OFPACT_CT) { + return 0; + } + + 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; +} + int ofproto_dpif_execute_actions__(struct ofproto_dpif *ofproto, const struct flow *flow, @@ -5689,7 +5723,7 @@ const struct ofproto_class ofproto_dpif_class = { port_poll_wait, port_is_lacp_current, port_get_lacp_stats, - NULL, + rule_check_action, NULL, /* rule_choose_table */ rule_alloc, rule_construct,
Disallow installing rules that execute ct() if conntrack is unsupported in the datapath. Also check different variations on the action which may be denied, such as writing to the ct_{mark,label} fields. Reported-by: Ravindra Kenchappa <ravindra.kenchappa@hpe.com> Signed-off-by: Joe Stringer <joestringer@nicira.com> --- ofproto/ofproto-dpif.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)