[ovs-discuss] [PATCH ] ofproto: Fix for ovs-vswitchd crash on flow-mod with unsupported action

[parameswaran krishnamurthy]

Problem Description:
The ovs-vswitchd is crashing while invoking flow-mod with upsupported
action(Tested with ovs2.10.1)

Steps to recreate:
Step 1) Create a flow
ovs-ofctl add-flow switch1
priority=228,dl_type=0x0800,dl_vlan="600",in_port=25,actions=output:ALL
This step is successful.

Step 2) Invoke flow-mod with incorrect contents.
 ovs-ofctl mod-flows switch1
 priority=228,dl_type=0x0800,dl_vlan="600",in_port=25,actions=output:ALL,mod_vlan_vid:50,mod_vlan_pcp=6,mod_nw_tos=16

 In the above example, the ofproto provider I have, will return error for
 rule_construct as set_fields come after Output.

 However the OVS is ignoring the error (The return value of add_flow_init
 is ignored in modify_flow_init_strict) and eventually the ovs-vswitched
 crashes.

Crash backtrace:
-----------------------

 Thread 1 "ovs-vswitchd" received signal SIGSEGV, Segmentation fault.

 0x00007f6a06e785fb in modify_flows_start__ (
   ofproto=ofproto at entry=0x55b289cecc28, ofm=ofm at entry=0x7ffdf7d57b70)
     at ofproto/ofproto.c:5402
 5402    in ofproto/ofproto.c
 (gdb) bt
 #0  0x00007f6a06e785fb in modify_flows_start__ (
   ofproto=ofproto at entry=0x55b289cecc28, ofm=ofm at entry=0x7ffdf7d57b70)
    at ofproto/ofproto.c:5402
 #1  0x00007f6a06e790db in modify_flows_start_loose (ofm=0x7ffdf7d57b70,
    ofproto=0x55b289cecc28) at ofproto/ofproto.c:5443
 #2  ofproto_flow_mod_start (ofproto=ofproto at entry=0x55b289cecc28,
     ofm=ofm at entry=0x7ffdf7d57b70) at ofproto/ofproto.c:7672
 #3  0x00007f6a06e79164 in handle_flow_mod__ (
    ofproto=ofproto at entry=0x55b289cecc28, fm=fm at entry=0x7ffdf7d57d20,
    req=req at entry=0x7ffdf7d57cd0) at ofproto/ofproto.c:5858
 #4  0x00007f6a06e792c2 in handle_flow_mod (ofconn=ofconn at entry
 =0x55b289d528c0,
oh=oh at entry=0x55b289d5a410) at ofproto/ofproto.c:5835
 #5  0x00007f6a06e7a173 in handle_openflow__ (msg=0x55b289d351d0,
    ofconn=0x55b289d528c0) at ofproto/ofproto.c:8127
 #6  handle_openflow (ofconn=0x55b289d528c0, ofp_msg=0x55b289d351d0)
  at ofproto/ofproto.c:8296
 #7  0x00007f6a06e6a013 in ofconn_run (
  handle_openflow=0x7f6a06e796f0 <handle_openflow>,
 ofconn=0x55b289d528c0)
 at ofproto/connmgr.c:1446
 #8  connmgr_run (mgr=0x55b289d14fe0,
 handle_openflow=handle_openflow at entry=0x7f6a06e796f0
handle_openflow>)
at ofproto/connmgr.c:365
#9  0x00007f6a06e73056 in ofproto_run (p=0x55b289cecc28)

Fix:
------
In modify_flow_init_strict and modify_flow_init_loose instead of
blindly returning 0, the fix returns the value returned by
add_flow_init.

Testing:
----------
The flow-mod that was causing the crash get properly rejected with the
fix and no crash occurred.

Patch
----------

diff -uNr '--exclude=*.o' openvswitch-2.10.1/ofproto/ofproto.c
openvswitch-2.10.1_modified/ofproto/ofproto.c
--- openvswitch-2.10.1/ofproto/ofproto.c    2018-10-19 15:57:46.650077822 -0700
+++ openvswitch-2.10.1_modified/ofproto/ofproto.c        2019-03-05
00:16:27.436754000 -0800
@@ -5410,6 +5410,7 @@
                         const struct ofputil_flow_mod *fm)
     OVS_EXCLUDED(ofproto_mutex)
 {
+    enum ofperr error;
     rule_criteria_init(&ofm->criteria, fm->table_id, &fm->match, 0,
                        OVS_VERSION_MAX, fm->cookie, fm->cookie_mask, OFPP_ANY,
                        OFPG_ANY);
@@ -5417,9 +5418,9 @@
                              (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
     /* Must create a new flow in advance for the case that no matches are
      * found.  Also used for template for multiple modified flows. */
-    add_flow_init(ofproto, ofm, fm);
+    error = add_flow_init(ofproto, ofm, fm);

-    return 0;
+    return error;
 }

 /* Implements OFPFC_MODIFY.  Returns 0 on success or an OpenFlow error code on
@@ -5495,6 +5496,7 @@
                         const struct ofputil_flow_mod *fm)
     OVS_EXCLUDED(ofproto_mutex)
 {
+    enum ofperr error;
     rule_criteria_init(&ofm->criteria, fm->table_id, &fm->match, fm->priority,
                        OVS_VERSION_MAX, fm->cookie, fm->cookie_mask, OFPP_ANY,
                        OFPG_ANY);
@@ -5502,9 +5504,9 @@
                              (fm->flags & OFPUTIL_FF_NO_READONLY) != 0);
     /* Must create a new flow in advance for the case that no matches are
      * found.  Also used for template for multiple modified flows. */
-    add_flow_init(ofproto, ofm, fm);
+    error = add_flow_init(ofproto, ofm, fm);

-    return 0;
+    return error;
 }

 /* Implements OFPFC_MODIFY_STRICT.  Returns 0 on success or an OpenFlow error

Signed-off-by: Parameswaran Krishnamurthy <parkrish at gmail.com>