[ovs-dev] [PATCHv2 0/6] Improve ct match/action verification.

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


> On Nov 11, 2015, at 11:39 AM, Joe Stringer <joestringer at nicira.com> wrote:
> 
> There are currently a few holes in how OVS verifies connection tracking fields
> and actions, pointed out by Ravindra Kenchappa. This series aims to verify
> ct_state,ct_zone,ct_mark,ct_label match fields and the ct() action more
> strictly.
> 
> Patches 1-2 are straight up fixes for the field verification. Patches 3-6 have
> been changed a bit based on v1 feedback, although I'm not entirely sure if
> they're the right approach, so I welcome further feedback.
> 
> In v2, the 'ofproto' is extended to have an additional function on the ofproto
> for action verification which is separate from the verification done in
> rule_construct(). This is mainly proposed to avoid introducing another loop
> across rule actions during rule_construct(). Prior to rule_construct(),
> ofproto_check_ofpacts() loops across to check that groups are valid. This
> series adds a function to the ofproto which will be called from this point for
> every action in each flowmod that is processed, allowing ofproto
> implementations to reject specific actions based on ofproto-specific criteria,
> for instance ofproto-dpif supports underlying datapaths that may not support
> connection tracking. I would appreciate feedback specifically on whether this
> error checking is worth splitting out from the rule_construct() phase. My
> current inclination is that it increases the complexity of the ofproto rule
> construction lifecycle, and avoiding an additional iteration would not actually
> provide any benefit as the new function must be chased through a class for
> every action, even if the implementation does not care about checking the
> majority of action types. Regardless, I have included the patch so we can
> review how this change looks.
> 

A high level comment to this point: ofproto_check_ofpacts() is already checking only specific actions, the rest being (mostly correctly) assumed to be supported. So, in this series I would expect that the new ofproto provider call would only be made for actions that are known to not always be supported, i.e., for the CT action only for now.

  Jarno

> Joe Stringer (6):
>  ofproto-dpif: Reject partial ct_labels if unsupported.
>  ofproto-dpif: Validate ct_* field masks.
>  ofproto-dpif: Shortcut common case in rule_check().
>  ofp-actions: Refactor ofpact_get_mf_dst().
>  ofproto-provider: Add action validation.
>  ofproto: Validate ct actions support.
> 
> lib/flow.h                 | 14 ++++-----
> lib/ofp-actions.c          | 18 ++++++-----
> lib/ofp-actions.h          |  1 +
> ofproto/ofproto-dpif.c     | 74 +++++++++++++++++++++++++++++++++-------------
> ofproto/ofproto-provider.h | 14 +++++++++
> ofproto/ofproto.c          |  9 ++++++
> 6 files changed, 94 insertions(+), 36 deletions(-)
> 
> -- 
> 2.1.4
> 




More information about the dev mailing list