Re: [ovs-dev] [branch-2.7 3/4] ofproto: Add ref counting for variable length mf_fields.

[Jarno Rajahalme]

Acked-by: Jarno Rajahalme  

with two notes:

1. Maybe patch 4 should be applied before this one to avoid creating a 
potential memory leak in the history?

2. taking new references before releasing old ones in modify_flows_start__() 
would seem better. Since the table holds a reference this does not matter in 
practice.

  Jarno

> On Mar 15, 2017, at 4:01 PM, Joe Stringer  wrote:
> 
> From: Yi-Hung Wei 
> 
> Currently, a controller may potentially trigger a segmentation fault if it
> accidentally removes a TLV mapping that is still used by an active flow.
> To resolve this issue, in this patch, we maintain reference counting for each
> dynamically allocated variable length mf_fields, so that vswitchd can use this
> information to properly remove a TLV mapping, and to return an error if the
> controller tries to remove a TLV mapping that is still used by any active 
> flow.
> 
> To keep track of the usage of tun_metadata for each flow, two 'uint64_t'
> bitmaps are introduce for the flow match and flow action respectively. We use
> 'uint64_t' as a bitmap since the 64 geneve TLV tunnel metadata are the only
> available variable length mf_fields for now. We shall adopt general bitmap 
> when
> more variable length mf_fields are introduced. The bitmaps are configured
> during the flow decoding process, and vswitchd use these bitmaps to increase 
> or
> decrease the ref counting when the flow is created or deleted.
> 
> VMWare-BZ: #1768370
> Fixes: 04f48a68c428 ("ofp-actions: Fix variable length meta-flow OXMs.")
> Suggested-by: Jarno Rajahalme 
> Suggested-by: Joe Stringer 
> Signed-off-by: Yi-Hung Wei 
> Signed-off-by: Joe Stringer 
> ---
> build-aux/extract-ofp-actions |   9 +-
> include/openvswitch/ofp-actions.h |   2 +
> include/openvswitch/ofp-errors.h  |   4 +
> include/openvswitch/ofp-util.h|   1 +
> lib/learn.c   |   5 +
> lib/meta-flow.c   | 228 --
> lib/ofp-actions.c | 208 +-
> lib/ofp-util.c|  21 ++--
> lib/vl-mff-map.h  |  17 ++-
> ofproto/ofproto-provider.h|   4 +
> ofproto/ofproto.c |  33 +-
> ovn/controller/pinctrl.c  |   6 +-
> tests/tunnel.at   |  76 -
> utilities/ovs-ofctl.c |   2 +-
> 14 files changed, 479 insertions(+), 137 deletions(-)
> 
> diff --git a/build-aux/extract-ofp-actions b/build-aux/extract-ofp-actions
> index 184447b99422..0062ab881dd5 100755
> --- a/build-aux/extract-ofp-actions
> +++ b/build-aux/extract-ofp-actions
> @@ -322,7 +322,8 @@ def extract_ofp_actions(fn, definitions):
> static enum ofperr
> ofpact_decode(const struct ofp_action_header *a, enum ofp_raw_action_type raw,
>   enum ofp_version version, uint64_t arg,
> -  const struct vl_mff_map *vl_mff_map, struct ofpbuf *out)
> +  const struct vl_mff_map *vl_mff_map,
> +  uint64_t *tlv_bitmap, struct ofpbuf *out)
> {
> switch (raw) {\
> """
> @@ -343,7 +344,7 @@ ofpact_decode(const struct ofp_action_header *a, enum 
> ofp_raw_action_type raw,
> else:
> arg = "arg"
> if arg_vl_mff_map:
> -print "return decode_%s(%s, version, vl_mff_map, 
> out);" % (enum, arg)
> +print "return decode_%s(%s, version, vl_mff_map, 
> tlv_bitmap, out);" % (enum, arg)
> else:
> print "return decode_%s(%s, version, out);" % 
> (enum, arg)
> print
> @@ -365,7 +366,7 @@ ofpact_decode(const struct ofp_action_header *a, enum 
> ofp_raw_action_type raw,
> else:
> prototype += "%s, enum ofp_version, " % base_argtype
> if arg_vl_mff_map:
> -prototype += 'const struct vl_mff_map *, '
> +prototype += 'const struct vl_mff_map *, uint64_t *, '
> prototype += "struct ofpbuf *);"
> print prototype
> 
> @@ -374,7 +375,7 @@ static enum ofperr ofpact_decode(const struct 
> ofp_action_header *,
>  enum ofp_raw_action_type raw,
>  enum ofp_version version,
>  uint64_t arg, const struct vl_mff_map 
> *vl_mff_map,
> - struct ofpbuf *out);
> + uint64_t *tlv_bitmap, struct ofpbuf *out);
> """
> 
> if __name__ == '__main__':
> diff --git a/include/openvswitch/ofp-actions.h 
> b/include/openvswitch/ofp-actions.h
> index 88f573dcd74e..214dfed3f3bd 100644
> --- a/include/openvswitch/ofp-actions.h
> +++ b/include/openvswitch/ofp-actions.h
> @@ -946,12 +946,14 @@ enum ofperr ofpacts_pull_openflow_actions(struct ofpbuf 
> 

[ovs-dev] [branch-2.7 3/4] ofproto: Add ref counting for variable length mf_fields.

[Joe Stringer]

From: Yi-Hung Wei 

Currently, a controller may potentially trigger a segmentation fault if it
accidentally removes a TLV mapping that is still used by an active flow.
To resolve this issue, in this patch, we maintain reference counting for each
dynamically allocated variable length mf_fields, so that vswitchd can use this
information to properly remove a TLV mapping, and to return an error if the
controller tries to remove a TLV mapping that is still used by any active flow.

To keep track of the usage of tun_metadata for each flow, two 'uint64_t'
bitmaps are introduce for the flow match and flow action respectively. We use
'uint64_t' as a bitmap since the 64 geneve TLV tunnel metadata are the only
available variable length mf_fields for now. We shall adopt general bitmap when
more variable length mf_fields are introduced. The bitmaps are configured
during the flow decoding process, and vswitchd use these bitmaps to increase or
decrease the ref counting when the flow is created or deleted.

VMWare-BZ: #1768370
Fixes: 04f48a68c428 ("ofp-actions: Fix variable length meta-flow OXMs.")
Suggested-by: Jarno Rajahalme 
Suggested-by: Joe Stringer 
Signed-off-by: Yi-Hung Wei 
Signed-off-by: Joe Stringer 
---
 build-aux/extract-ofp-actions |   9 +-
 include/openvswitch/ofp-actions.h |   2 +
 include/openvswitch/ofp-errors.h  |   4 +
 include/openvswitch/ofp-util.h|   1 +
 lib/learn.c   |   5 +
 lib/meta-flow.c   | 228 --
 lib/ofp-actions.c | 208 +-
 lib/ofp-util.c|  21 ++--
 lib/vl-mff-map.h  |  17 ++-
 ofproto/ofproto-provider.h|   4 +
 ofproto/ofproto.c |  33 +-
 ovn/controller/pinctrl.c  |   6 +-
 tests/tunnel.at   |  76 -
 utilities/ovs-ofctl.c |   2 +-
 14 files changed, 479 insertions(+), 137 deletions(-)

diff --git a/build-aux/extract-ofp-actions b/build-aux/extract-ofp-actions
index 184447b99422..0062ab881dd5 100755
--- a/build-aux/extract-ofp-actions
+++ b/build-aux/extract-ofp-actions
@@ -322,7 +322,8 @@ def extract_ofp_actions(fn, definitions):
 static enum ofperr
 ofpact_decode(const struct ofp_action_header *a, enum ofp_raw_action_type raw,
   enum ofp_version version, uint64_t arg,
-  const struct vl_mff_map *vl_mff_map, struct ofpbuf *out)
+  const struct vl_mff_map *vl_mff_map,
+  uint64_t *tlv_bitmap, struct ofpbuf *out)
 {
 switch (raw) {\
 """
@@ -343,7 +344,7 @@ ofpact_decode(const struct ofp_action_header *a, enum 
ofp_raw_action_type raw,
 else:
 arg = "arg"
 if arg_vl_mff_map:
-print "return decode_%s(%s, version, vl_mff_map, 
out);" % (enum, arg)
+print "return decode_%s(%s, version, vl_mff_map, 
tlv_bitmap, out);" % (enum, arg)
 else:
 print "return decode_%s(%s, version, out);" % 
(enum, arg)
 print
@@ -365,7 +366,7 @@ ofpact_decode(const struct ofp_action_header *a, enum 
ofp_raw_action_type raw,
 else:
 prototype += "%s, enum ofp_version, " % base_argtype
 if arg_vl_mff_map:
-prototype += 'const struct vl_mff_map *, '
+prototype += 'const struct vl_mff_map *, uint64_t *, '
 prototype += "struct ofpbuf *);"
 print prototype
 
@@ -374,7 +375,7 @@ static enum ofperr ofpact_decode(const struct 
ofp_action_header *,
  enum ofp_raw_action_type raw,
  enum ofp_version version,
  uint64_t arg, const struct vl_mff_map 
*vl_mff_map,
- struct ofpbuf *out);
+ uint64_t *tlv_bitmap, struct ofpbuf *out);
 """
 
 if __name__ == '__main__':
diff --git a/include/openvswitch/ofp-actions.h 
b/include/openvswitch/ofp-actions.h
index 88f573dcd74e..214dfed3f3bd 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -946,12 +946,14 @@ enum ofperr ofpacts_pull_openflow_actions(struct ofpbuf 
*openflow,
   unsigned int actions_len,
   enum ofp_version version,
   const struct vl_mff_map *,
+  uint64_t *ofpacts_tlv_bitmap,
   struct ofpbuf *ofpacts);
 enum ofperr
 ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
unsigned int instructions_len,
enum ofp_version version,