Message ID | 1447270794-21103-2-git-send-email-joestringer@nicira.com |
---|---|
State | Awaiting Upstream |
Headers | show |
Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Nov 11, 2015, at 11:39 AM, Joe Stringer <joestringer@nicira.com> wrote: > > If only half of a ct_label is present in a miniflow/minimask (eg, only > matching on one specific bit), then rule_check() would allow the flow > even if ct_label was unsupported, because it required both 64-bit fields > that comprise the ct_label to be present in the miniflow before > performing the check. > > Fix this by populating the stack copy of the label directly from the > miniflow fields if available (or zero each 64-bit word if unavailable). > > Suggested-by: Jarno Rajahalme <jrajahalme@nicira.com> > Signed-off-by: Joe Stringer <joestringer@nicira.com> > --- > lib/flow.h | 14 ++++++-------- > ofproto/ofproto-dpif.c | 8 ++------ > 2 files changed, 8 insertions(+), 14 deletions(-) > > diff --git a/lib/flow.h b/lib/flow.h > index efd34a0da1a4..e7f851ea3dd1 100644 > --- a/lib/flow.h > +++ b/lib/flow.h > @@ -788,14 +788,12 @@ miniflow_get__(const struct miniflow *mf, size_t idx) > [FLOW_U64_OFFREM(FIELD) / sizeof(TYPE)] \ > : 0) > > -/* Get a pointer to the ovs_u128 value of struct flow 'FIELD' from miniflow > - * 'FLOW'. */ > -#define MINIFLOW_GET_U128_PTR(FLOW, FIELD) \ > - ((MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD)) \ > - && (MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD) + 1))) \ > - ? &((OVS_FORCE const ovs_u128 *)miniflow_get__(FLOW, FLOW_U64_OFFSET(FIELD))) \ > - [FLOW_U64_OFFREM(FIELD) / sizeof(ovs_u128)] \ > - : NULL) > +#define MINIFLOW_GET_U128(FLOW, FIELD) \ > + (ovs_u128) { .u64 = { \ > + (MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD)) ? \ > + *miniflow_get__(FLOW, FLOW_U64_OFFSET(FIELD)) : 0), \ > + (MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD) + 1) ? \ > + *miniflow_get__(FLOW, FLOW_U64_OFFSET(FIELD) + 1) : 0) } } > > #define MINIFLOW_GET_U8(FLOW, FIELD) \ > MINIFLOW_GET_TYPE(FLOW, uint8_t, FIELD) > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 5cc64cbca1f5..ab1b6a2f7d8e 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -4015,17 +4015,13 @@ static enum ofperr > rule_check(struct rule *rule) > { > uint16_t ct_state, ct_zone; > - const ovs_u128 *labelp; > - ovs_u128 ct_label = { { 0, 0 } }; > + 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); > - labelp = MINIFLOW_GET_U128_PTR(rule->cr.match.flow, ct_label); > - if (labelp) { > - ct_label = *labelp; > - } > + ct_label = MINIFLOW_GET_U128(rule->cr.match.flow, ct_label); > > if (ct_state || ct_zone || ct_mark > || !ovs_u128_is_zero(&ct_label)) { > -- > 2.1.4 >
diff --git a/lib/flow.h b/lib/flow.h index efd34a0da1a4..e7f851ea3dd1 100644 --- a/lib/flow.h +++ b/lib/flow.h @@ -788,14 +788,12 @@ miniflow_get__(const struct miniflow *mf, size_t idx) [FLOW_U64_OFFREM(FIELD) / sizeof(TYPE)] \ : 0) -/* Get a pointer to the ovs_u128 value of struct flow 'FIELD' from miniflow - * 'FLOW'. */ -#define MINIFLOW_GET_U128_PTR(FLOW, FIELD) \ - ((MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD)) \ - && (MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD) + 1))) \ - ? &((OVS_FORCE const ovs_u128 *)miniflow_get__(FLOW, FLOW_U64_OFFSET(FIELD))) \ - [FLOW_U64_OFFREM(FIELD) / sizeof(ovs_u128)] \ - : NULL) +#define MINIFLOW_GET_U128(FLOW, FIELD) \ + (ovs_u128) { .u64 = { \ + (MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD)) ? \ + *miniflow_get__(FLOW, FLOW_U64_OFFSET(FIELD)) : 0), \ + (MINIFLOW_IN_MAP(FLOW, FLOW_U64_OFFSET(FIELD) + 1) ? \ + *miniflow_get__(FLOW, FLOW_U64_OFFSET(FIELD) + 1) : 0) } } #define MINIFLOW_GET_U8(FLOW, FIELD) \ MINIFLOW_GET_TYPE(FLOW, uint8_t, FIELD) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 5cc64cbca1f5..ab1b6a2f7d8e 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4015,17 +4015,13 @@ static enum ofperr rule_check(struct rule *rule) { uint16_t ct_state, ct_zone; - const ovs_u128 *labelp; - ovs_u128 ct_label = { { 0, 0 } }; + 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); - labelp = MINIFLOW_GET_U128_PTR(rule->cr.match.flow, ct_label); - if (labelp) { - ct_label = *labelp; - } + ct_label = MINIFLOW_GET_U128(rule->cr.match.flow, ct_label); if (ct_state || ct_zone || ct_mark || !ovs_u128_is_zero(&ct_label)) {
If only half of a ct_label is present in a miniflow/minimask (eg, only matching on one specific bit), then rule_check() would allow the flow even if ct_label was unsupported, because it required both 64-bit fields that comprise the ct_label to be present in the miniflow before performing the check. Fix this by populating the stack copy of the label directly from the miniflow fields if available (or zero each 64-bit word if unavailable). Suggested-by: Jarno Rajahalme <jrajahalme@nicira.com> Signed-off-by: Joe Stringer <joestringer@nicira.com> --- lib/flow.h | 14 ++++++-------- ofproto/ofproto-dpif.c | 8 ++------ 2 files changed, 8 insertions(+), 14 deletions(-)