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

[Numan Siddique]

On Tue, Mar 5, 2019 at 6:30 PM parameswaran krishnamurthy <
parkrish at gmail.com> wrote:

> Problem Description:
> The ovs-vswitchd is crashing while invoking flow-mod with upsupported
> action(Tested with ovs2.10.1)
>
>
Ideally the patches are submitted against the master branch.  Once the
patch is merged in master, it is then back ported
to the branches.

My suggestion would be to test with the master branch and submit the patch
again.

Thanks
Numan




> 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>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>