> On Nov 7, 2015, at 12:05 PM, 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 denying nonzero masks whenever there is no datapath support
> for connection tracking.
>
> Reported-by: Ravindra Kenchappa <[email protected]>
> Signed-off-by: Joe Stringer <[email protected]>
> ---
> ofproto/ofproto-dpif.c | 33 ++++++++++++++++++++++++---------
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 5cc64cbca1f5..2f75b93d9694 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4012,40 +4012,55 @@ rule_dealloc(struct rule *rule_)
> }
>
> static enum ofperr
> -rule_check(struct rule *rule)
> +check_flow(const struct ofproto_dpif *ofproto, const struct miniflow *flow,
> + bool mask)
> {
> + ovs_u128 ct_label = { { 0, 0 } };
> uint16_t ct_state, ct_zone;
> const ovs_u128 *labelp;
> - ovs_u128 ct_label = { { 0, 0 } };
> 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);
> + ct_state = MINIFLOW_GET_U16(flow, ct_state);
> + ct_zone = MINIFLOW_GET_U16(flow, ct_zone);
> + ct_mark = MINIFLOW_GET_U32(flow, ct_mark);
> + labelp = MINIFLOW_GET_U128_PTR(flow, ct_label);
> if (labelp) {
> ct_label = *labelp;
> }
>
Checking MINIFLOW_GET_U128_PTR(), I think it has a problem if only half of it
is present in the miniflow/mask, which would be typical when you match on e.g.
one bit. How about this instead:
#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) } }
> if (ct_state || ct_zone || ct_mark
> || !ovs_u128_is_zero(&ct_label)) {
This function would be a bit cheaper in the eventuality where the datapath
supports all the features if we first check if any of the features is missing,
and then check for the presence of non-zero miniflow values.
> - struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto);
> - const struct odp_support *support =
> &ofproto_dpif_get_support(ofproto)->odp;
> + const struct odp_support *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;
Is it OK to return BAD_FIELD if we are checking a mask? Maybe this function
should just return a bool and the caller then returns the error code?
> }
> - if (ct_state & CS_UNSUPPORTED_MASK) {
> + if (mask && ct_state & CS_UNSUPPORTED_MASK) {
> return OFPERR_OFPBMC_BAD_MASK;
> }
> }
> +
> return 0;
> }
>
> static enum ofperr
> +rule_check(struct rule *rule)
> +{
> + struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto);
> + enum ofperr err;
> +
> + err = check_flow(ofproto, rule->cr.match.flow, false);
> + if (err) {
> + return err;
> + }
> + return check_flow(ofproto, &rule->cr.match.mask->masks, true);
> +}
> +
> +static enum ofperr
> rule_construct(struct rule *rule_)
> OVS_NO_THREAD_SAFETY_ANALYSIS
> {
> --
> 2.1.4
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
