| Message ID | 1481133844-120196-2-git-send-email-nic@opencloud.tech |
|---|---|
| State | Accepted |
| Headers | show |
2016-12-07 10:04 GMT-08:00 nickcooper-zhangtonghao <nic@opencloud.tech>: > When the datapath, whose type is "netdev", processes packets > in userspce action, it may cause a segmentation fault. In the > dp_execute_userspace_action(), we pass the "wc" argument to > dp_netdev_upcall() using NULL. In the dp_netdev_upcall() call tree, > the "wc" will be used. For example, dp_netdev_upcall() uses the > &wc->masks for debugging, and flow_wildcards_init_for_packet() > uses the "wc" if we disable megaflow, which is described in > more detail below. > > Segmentation fault in flow_wildcards_init_for_packet: > > #0 0x0000000000468fe8 flow_wildcards_init_for_packet lib/flow.c:1275 > #1 0x0000000000436c0b upcall_cb ofproto/ofproto-dpif-upcall.c:1231 > #2 0x000000000045bd96 dp_netdev_upcall lib/dpif-netdev.c:3857 > #3 0x0000000000461bf3 dp_execute_userspace_action lib/dpif-netdev.c:4388 > #4 dp_execute_cb lib/dpif-netdev.c:4521 > #5 0x0000000000486ae2 odp_execute_actions lib/odp-execute.c:538 > #6 0x00000000004607f9 dp_netdev_execute_actions lib/dpif-netdev.c:4627 > #7 packet_batch_per_flow_execute lib/dpif-netdev.c:3927 > #8 dp_netdev_input__ lib/dpif-netdev.c:4229 > #9 0x0000000000460ba8 dp_netdev_input lib/dpif-netdev.c:4238 > #10 dp_netdev_process_rxq_port lib/dpif-netdev.c:2873 > #11 0x000000000046126e dpif_netdev_run lib/dpif-netdev.c:3000 > #12 0x000000000042baf5 type_run ofproto/ofproto-dpif.c:504 > #13 0x00000000004192bf ofproto_type_run ofproto/ofproto.c:1687 > #14 0x0000000000409965 bridge_run__ vswitchd/bridge.c:2875 > #15 0x000000000040f145 bridge_run vswitchd/bridge.c:2938 > #16 0x00000000004062e5 main vswitchd/ovs-vswitchd.c:111 > > Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> > Signed-off-by: Daniele Di Proietto <diproiettod@ovn.org> Thanks! I added another check for 'wc' in ukey_create_from_upcall(). I pushed this to master, branch-2.6 and branch-2.5 > --- > lib/dpif-netdev.c | 2 +- > ofproto/ofproto-dpif-upcall.c | 4 ++-- > tests/ofproto-dpif.at | 34 ++++++++++++++++++++++++++++++++++ > 3 files changed, 37 insertions(+), 3 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 1400511..0b73056 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -3834,7 +3834,7 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet *packet_, > struct ofpbuf key; > struct odp_flow_key_parms odp_parms = { > .flow = flow, > - .mask = &wc->masks, > + .mask = wc ? &wc->masks : NULL, > .support = dp_netdev_support, > }; > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index 6cb9c2e..aa7eabf 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -1227,7 +1227,7 @@ upcall_cb(const struct dp_packet *packet, const struct flow *flow, ovs_u128 *ufi > upcall.put_actions.size); > } > > - if (OVS_UNLIKELY(!megaflow)) { > + if (OVS_UNLIKELY(!megaflow && wc)) { > flow_wildcards_init_for_packet(wc, flow); > } > > @@ -1504,7 +1504,7 @@ ukey_create_from_upcall(struct upcall *upcall, struct flow_wildcards *wc) > bool megaflow; > struct odp_flow_key_parms odp_parms = { > .flow = upcall->flow, > - .mask = &wc->masks, > + .mask = wc ? &wc->masks : NULL, > }; > > odp_parms.support = ofproto_dpif_get_support(upcall->ofproto)->odp; > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index cd90424..6d57445 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -1627,6 +1627,40 @@ NXST_FLOW reply: > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([ofproto-dpif - controller action without megaflows]) > +OVS_VSWITCHD_START > +add_of_ports br0 1 > + > +AT_CHECK([ovs-ofctl add-flow br0 in_port=1,action=controller]) > +AT_CHECK([ovs-appctl upcall/disable-megaflows], [0], [dnl > +megaflows disabled > +]) > + > +AT_CAPTURE_FILE([ofctl_monitor.log]) > +AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach --no-chdir --pidfile 2> ofctl_monitor.log]) > + > +for i in 1 2; do > + AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)']) > +done > + > +OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 4]) > +OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit]) > + > +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl > +flow-dump from non-dpdk interfaces: > +packets:1, bytes:14, used:0.001s, actions:userspace(pid=0,slow_path(controller)) > +]) > + > +AT_CHECK([cat ofctl_monitor.log], [0], [dnl > +NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via action) data_len=14 (unbuffered) > +vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234 > +NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via action) data_len=14 (unbuffered) > +vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([ofproto-dpif - MPLS handling]) > OVS_VSWITCHD_START([dnl > add-port br0 p1 -- set Interface p1 type=dummy > -- > 1.8.3.1 > > >
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 1400511..0b73056 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -3834,7 +3834,7 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet *packet_, struct ofpbuf key; struct odp_flow_key_parms odp_parms = { .flow = flow, - .mask = &wc->masks, + .mask = wc ? &wc->masks : NULL, .support = dp_netdev_support, }; diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 6cb9c2e..aa7eabf 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1227,7 +1227,7 @@ upcall_cb(const struct dp_packet *packet, const struct flow *flow, ovs_u128 *ufi upcall.put_actions.size); } - if (OVS_UNLIKELY(!megaflow)) { + if (OVS_UNLIKELY(!megaflow && wc)) { flow_wildcards_init_for_packet(wc, flow); } @@ -1504,7 +1504,7 @@ ukey_create_from_upcall(struct upcall *upcall, struct flow_wildcards *wc) bool megaflow; struct odp_flow_key_parms odp_parms = { .flow = upcall->flow, - .mask = &wc->masks, + .mask = wc ? &wc->masks : NULL, }; odp_parms.support = ofproto_dpif_get_support(upcall->ofproto)->odp; diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index cd90424..6d57445 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -1627,6 +1627,40 @@ NXST_FLOW reply: OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([ofproto-dpif - controller action without megaflows]) +OVS_VSWITCHD_START +add_of_ports br0 1 + +AT_CHECK([ovs-ofctl add-flow br0 in_port=1,action=controller]) +AT_CHECK([ovs-appctl upcall/disable-megaflows], [0], [dnl +megaflows disabled +]) + +AT_CAPTURE_FILE([ofctl_monitor.log]) +AT_CHECK([ovs-ofctl monitor br0 65534 invalid_ttl -P nxt_packet_in --detach --no-chdir --pidfile 2> ofctl_monitor.log]) + +for i in 1 2; do + AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x1234)']) +done + +OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 4]) +OVS_WAIT_UNTIL([ovs-appctl -t ovs-ofctl exit]) + +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*\(packets:\)/\1/' | sed 's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl +flow-dump from non-dpdk interfaces: +packets:1, bytes:14, used:0.001s, actions:userspace(pid=0,slow_path(controller)) +]) + +AT_CHECK([cat ofctl_monitor.log], [0], [dnl +NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via action) data_len=14 (unbuffered) +vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234 +NXT_PACKET_IN (xid=0x0): cookie=0x0 total_len=14 in_port=1 (via action) data_len=14 (unbuffered) +vlan_tci=0x0000,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,dl_type=0x1234 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([ofproto-dpif - MPLS handling]) OVS_VSWITCHD_START([dnl add-port br0 p1 -- set Interface p1 type=dummy
When the datapath, whose type is "netdev", processes packets in userspce action, it may cause a segmentation fault. In the dp_execute_userspace_action(), we pass the "wc" argument to dp_netdev_upcall() using NULL. In the dp_netdev_upcall() call tree, the "wc" will be used. For example, dp_netdev_upcall() uses the &wc->masks for debugging, and flow_wildcards_init_for_packet() uses the "wc" if we disable megaflow, which is described in more detail below. Segmentation fault in flow_wildcards_init_for_packet: #0 0x0000000000468fe8 flow_wildcards_init_for_packet lib/flow.c:1275 #1 0x0000000000436c0b upcall_cb ofproto/ofproto-dpif-upcall.c:1231 #2 0x000000000045bd96 dp_netdev_upcall lib/dpif-netdev.c:3857 #3 0x0000000000461bf3 dp_execute_userspace_action lib/dpif-netdev.c:4388 #4 dp_execute_cb lib/dpif-netdev.c:4521 #5 0x0000000000486ae2 odp_execute_actions lib/odp-execute.c:538 #6 0x00000000004607f9 dp_netdev_execute_actions lib/dpif-netdev.c:4627 #7 packet_batch_per_flow_execute lib/dpif-netdev.c:3927 #8 dp_netdev_input__ lib/dpif-netdev.c:4229 #9 0x0000000000460ba8 dp_netdev_input lib/dpif-netdev.c:4238 #10 dp_netdev_process_rxq_port lib/dpif-netdev.c:2873 #11 0x000000000046126e dpif_netdev_run lib/dpif-netdev.c:3000 #12 0x000000000042baf5 type_run ofproto/ofproto-dpif.c:504 #13 0x00000000004192bf ofproto_type_run ofproto/ofproto.c:1687 #14 0x0000000000409965 bridge_run__ vswitchd/bridge.c:2875 #15 0x000000000040f145 bridge_run vswitchd/bridge.c:2938 #16 0x00000000004062e5 main vswitchd/ovs-vswitchd.c:111 Signed-off-by: nickcooper-zhangtonghao <nic@opencloud.tech> Signed-off-by: Daniele Di Proietto <diproiettod@ovn.org> --- lib/dpif-netdev.c | 2 +- ofproto/ofproto-dpif-upcall.c | 4 ++-- tests/ofproto-dpif.at | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-)