[ovs-dev] [PATCHv2 2/6] ofproto-dpif: Validate ct_* field masks.

Jarno Rajahalme jarno at ovn.org
Wed Nov 11 22:20:00 UTC 2015


With one comment to consider below:

Acked-by: Jarno Rajahalme <jarno at ovn.org>

> On Nov 11, 2015, at 11:39 AM, Joe Stringer <joestringer at nicira.com> 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 performing the check on masks instead of the flows.
> 
> Reported-by: Ravindra Kenchappa <ravindra.kenchappa at hpe.com>
> Signed-off-by: Joe Stringer <joestringer at nicira.com>
> ---
> ofproto/ofproto-dpif.c | 32 ++++++++++++++------------------
> 1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index ab1b6a2f7d8e..ee2d267ab7b8 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4014,30 +4014,26 @@ rule_dealloc(struct rule *rule_)
> static enum ofperr
> rule_check(struct rule *rule)
> {
> +    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;
> 
> -    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);
> -    ct_label = MINIFLOW_GET_U128(rule->cr.match.flow, ct_label);
> +    support = &ofproto_dpif_get_support(ofproto)->odp;
> +    ct_state = MINIFLOW_GET_U16(&rule->cr.match.mask->masks, ct_state);
> +    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);
> 
> -    if (ct_state || ct_zone || ct_mark
> -        || !ovs_u128_is_zero(&ct_label)) {
> -        struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->ofproto);
> -        const struct odp_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;
> -        }
> -        if (ct_state & CS_UNSUPPORTED_MASK) {
> -            return OFPERR_OFPBMC_BAD_MASK;
> -        }
> +    if ((ct_state && !support->ct_state)

How about first checking the support, and then getting the miniflow data only if needed? I see that you still need to check the state for unsupported bits, but most other cases would get rid of the stack copies altogether. Also vs_u128_is_zero, if it would take it’s argument by value.

> +        || (ct_state & CS_UNSUPPORTED_MASK)
> +        || (ct_zone && !support->ct_zone)
> +        || (ct_mark && !support->ct_mark)
> +        || (!ovs_u128_is_zero(&ct_label) && !support->ct_label)) {
> +        return OFPERR_OFPBMC_BAD_MASK;
>     }
> +
>     return 0;
> }
> 
> -- 
> 2.1.4
> 




More information about the dev mailing list