[ovs-dev] [PATCH 1/5] ofproto: Fix double-unref of temporary rule when learning.

[Ben Pfaff]

When ofproto_flow_mod_init() accepts a rule, it takes ownership of it and
either unrefs it on error or transfers ownership to the struct it
initializes on success, but ofproto_flow_mod_init_for_learn() was unref-ing
it a second time if it reported an error.

Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 ofproto/ofproto-provider.h |  5 ++++-
 ofproto/ofproto.c          | 16 ++++++----------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 9dc73c482119..ae4af4525705 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -1874,7 +1874,10 @@ struct rule_criteria {
 /* flow_mod with execution context. */
 struct ofproto_flow_mod {
     /* Allocated by 'init' phase, may be freed after 'start' phase, as these
-     * are not needed for 'revert' nor 'finish'. */
+     * are not needed for 'revert' nor 'finish'.
+     *
+     * This structure owns a reference to 'temp_rule' (if it is nonnull) that
+     * must be eventually be released with ofproto_rule_unref().  */
     struct rule *temp_rule;
     struct rule_criteria criteria;
     struct cls_conjunction *conjs;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index d42acd747acc..76d96a6f93f3 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -4948,15 +4948,14 @@ ofproto_rule_create(struct ofproto *ofproto, struct cls_rule *cr,
 
 /* Initialize 'ofm' for a learn action.  If the rule already existed, reference
  * to that rule is taken, otherwise a new rule is created.  'ofm' keeps the
- * rule reference in both.  This does not take the global 'ofproto_mutex'. */
+ * rule reference in both.  This does not take the global 'ofproto_mutex'.
+ */
 enum ofperr
 ofproto_flow_mod_init_for_learn(struct ofproto *ofproto,
                                 const struct ofputil_flow_mod *fm,
                                 struct ofproto_flow_mod *ofm)
     OVS_EXCLUDED(ofproto_mutex)
 {
-    enum ofperr error;
-
     /* Reject flow mods that do not look like they were generated by a learn
      * action. */
     if (fm->command != OFPFC_MODIFY_STRICT || fm->table_id == OFPTT_ALL
@@ -4997,13 +4996,7 @@ ofproto_flow_mod_init_for_learn(struct ofproto *ofproto,
         }
     }
 
-    /* Initialize ofproto_flow_mod for future use. */
-    error = ofproto_flow_mod_init(ofproto, ofm, fm, rule);
-    if (error) {
-        ofproto_rule_unref(rule);
-        return error;
-    }
-    return 0;
+    return ofproto_flow_mod_init(ofproto, ofm, fm, rule);
 }
 
 enum ofperr
@@ -7558,6 +7551,9 @@ ofproto_flow_mod_uninit(struct ofproto_flow_mod *ofm)
     }
 }
 
+/* Initializes 'ofm' with 'ofproto', 'fm', and 'rule'.  'rule' may be null, but
+ * if it is nonnull then the caller must own a reference to it, which on
+ * success is transferred to 'ofm' and on failure is unreffed. */
 static enum ofperr
 ofproto_flow_mod_init(struct ofproto *ofproto, struct ofproto_flow_mod *ofm,
                       const struct ofputil_flow_mod *fm, struct rule *rule)
-- 
2.10.2