| Message ID | AI6AWwDnI4upsxZaB49pYKpM.1.1680167605523.Hmail.mocan@ucloud.cn |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] learning-switch: Fix coredump of OpenFlow15 learning-switch | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Thu, Mar 30, 2023 at 05:13:25PM +0800, Faicker Mo wrote: > The OpenFlow15 Packet-Out message contains the match instead of the in_port. > The flow.tunnel.metadata.tab is not inited but used in the loop of tun_metadata_to_nx_match. > > The coredump gdb backtrace is: > #0 memcpy_from_metadata (dst=dst@entry=0x7ffcfac2f060, src=src@entry=0x7ffcfac30880, loc=loc@entry=0x10) at lib/tun-metadata.c:467 > #1 0x00000000004506e8 in metadata_loc_from_match_read (match=0x7ffcfac30598, is_masked=<synthetic pointer>, mask=0x7ffcfac30838, idx=0, map=0x0) at lib/tun-metadata.c:865 > #2 metadata_loc_from_match_read (is_masked=<synthetic pointer>, mask=0x7ffcfac30838, idx=0, match=0x7ffcfac30598, map=0x0) at lib/tun-metadata.c:854 > #3 tun_metadata_to_nx_match (b=b@entry=0x892260, oxm=oxm@entry=OFP15_VERSION, match=match@entry=0x7ffcfac30598) at lib/tun-metadata.c:888 > #4 0x000000000047c1f8 in nx_put_raw (b=b@entry=0x892260, oxm=oxm@entry=OFP15_VERSION, match=match@entry=0x7ffcfac30598, cookie=<optimized out>, cookie@entry=0, > cookie_mask=<optimized out>, cookie_mask@entry=0) at lib/nx-match.c:1186 > #5 0x000000000047d693 in oxm_put_match (b=b@entry=0x892260, match=match@entry=0x7ffcfac30598, version=version@entry=OFP15_VERSION) at lib/nx-match.c:1343 > #6 0x000000000043194e in ofputil_encode_packet_out (po=po@entry=0x7ffcfac30580, protocol=<optimized out>) at lib/ofp-packet.c:1226 > #7 0x000000000040a4fe in process_packet_in (sw=sw@entry=0x891d70, oh=<optimized out>) at lib/learning-switch.c:619 > #8 0x000000000040acdc in lswitch_process_packet (msg=0x892210, sw=0x891d70) at lib/learning-switch.c:374 > #9 lswitch_run (sw=0x891d70) at lib/learning-switch.c:324 > #10 0x0000000000406f26 in main (argc=<optimized out>, argv=<optimized out>) at utilities/ovs-testcontroller.c:180 > > Fix that by zeroing out the po variable. > > Fixes: 577bfa9f6879 ("ofp-util: Add OpenFlow 1.5 packet-out support") > Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On 3/30/23 11:13, Faicker Mo wrote: > The OpenFlow15 Packet-Out message contains the match instead of the in_port. > The flow.tunnel.metadata.tab is not inited but used in the loop of tun_metadata_to_nx_match. > > The coredump gdb backtrace is: > #0 memcpy_from_metadata (dst=dst@entry=0x7ffcfac2f060, src=src@entry=0x7ffcfac30880, loc=loc@entry=0x10) at lib/tun-metadata.c:467 > #1 0x00000000004506e8 in metadata_loc_from_match_read (match=0x7ffcfac30598, is_masked=<synthetic pointer>, mask=0x7ffcfac30838, idx=0, map=0x0) at lib/tun-metadata.c:865 > #2 metadata_loc_from_match_read (is_masked=<synthetic pointer>, mask=0x7ffcfac30838, idx=0, match=0x7ffcfac30598, map=0x0) at lib/tun-metadata.c:854 > #3 tun_metadata_to_nx_match (b=b@entry=0x892260, oxm=oxm@entry=OFP15_VERSION, match=match@entry=0x7ffcfac30598) at lib/tun-metadata.c:888 > #4 0x000000000047c1f8 in nx_put_raw (b=b@entry=0x892260, oxm=oxm@entry=OFP15_VERSION, match=match@entry=0x7ffcfac30598, cookie=<optimized out>, cookie@entry=0, > cookie_mask=<optimized out>, cookie_mask@entry=0) at lib/nx-match.c:1186 > #5 0x000000000047d693 in oxm_put_match (b=b@entry=0x892260, match=match@entry=0x7ffcfac30598, version=version@entry=OFP15_VERSION) at lib/nx-match.c:1343 > #6 0x000000000043194e in ofputil_encode_packet_out (po=po@entry=0x7ffcfac30580, protocol=<optimized out>) at lib/ofp-packet.c:1226 > #7 0x000000000040a4fe in process_packet_in (sw=sw@entry=0x891d70, oh=<optimized out>) at lib/learning-switch.c:619 > #8 0x000000000040acdc in lswitch_process_packet (msg=0x892210, sw=0x891d70) at lib/learning-switch.c:374 > #9 lswitch_run (sw=0x891d70) at lib/learning-switch.c:324 > #10 0x0000000000406f26 in main (argc=<optimized out>, argv=<optimized out>) at utilities/ovs-testcontroller.c:180 > > Fix that by zeroing out the po variable. > > Fixes: 577bfa9f6879 ("ofp-util: Add OpenFlow 1.5 packet-out support") I think, the following commit is more appropriate: Fixes: 35eb6326d5d0 ("ofp-util: Add flow metadata to ofputil_packet_out") > Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn> > --- > v2: > - changed the init and add test case > --- > lib/learning-switch.c | 2 +- > tests/system-traffic.at | 21 +++++++++++++++++++++ > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/lib/learning-switch.c b/lib/learning-switch.c > index 8102475ca..275c30e26 100644 > --- a/lib/learning-switch.c > +++ b/lib/learning-switch.c > @@ -524,7 +524,7 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh) > uint64_t ofpacts_stub[64 / 8]; > struct ofpbuf ofpacts; > > - struct ofputil_packet_out po; > + struct ofputil_packet_out po = {0}; It might be better to explicitly call match_init_catchall(&po.flow_metadata) instead. In case the 'match' structure will need a special initialization logic in the future. > enum ofperr error; > > struct dp_packet pkt; > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 2558f3b24..7de26840c 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -7845,3 +7845,24 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: *0000 *0000 *5002 *2000 *b85e *00 > > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > + > +AT_BANNER([learning-switch]) > +AT_SETUP([learning-switch - OpenFlow15]) > +dnl Start ovs-testcontroller > +AT_CHECK([ovs-testcontroller --no-chdir --detach punix:controller --pidfile -v ptcp:], [0], [ignore]) > +OVS_WAIT_UNTIL([test -e controller]) > +dnl Setup ovs > +OVS_TRAFFIC_VSWITCHD_START([-- set bridge br0 protocols=OpenFlow15 -- set-controller br0 tcp:127.0.0.1:6653]) > + > +ADD_NAMESPACES(at_ns0, at_ns1) > + > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") > + > +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 | FORMAT_PING], [0], [dnl > +10 packets transmitted, 10 received, 0% packet loss, time 0ms > +]) > +kill `cat ovs-testcontroller.pid` > +OVS_WAIT_UNTIL([! test -e controller]) > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP Thanks for adding a test. But we don't really need a system test in order to test this part of the code. The same can be expressed as a test in a common testsuite with dummy ports. 'netdev-dummy/receive' appctl commands can be used to inject packets. This way the test will be running in our CI automatically on every patch. I'm not sure though in which file the new test should live, so feel free to create a new one like tests/testcontroller.at. Best regards, Ilya Maximets.
diff --git a/lib/learning-switch.c b/lib/learning-switch.c index 8102475ca..275c30e26 100644 --- a/lib/learning-switch.c +++ b/lib/learning-switch.c @@ -524,7 +524,7 @@ process_packet_in(struct lswitch *sw, const struct ofp_header *oh) uint64_t ofpacts_stub[64 / 8]; struct ofpbuf ofpacts; - struct ofputil_packet_out po; + struct ofputil_packet_out po = {0}; enum ofperr error; struct dp_packet pkt; diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 2558f3b24..7de26840c 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -7845,3 +7845,24 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: *0000 *0000 *5002 *2000 *b85e *00 OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP + +AT_BANNER([learning-switch]) +AT_SETUP([learning-switch - OpenFlow15]) +dnl Start ovs-testcontroller +AT_CHECK([ovs-testcontroller --no-chdir --detach punix:controller --pidfile -v ptcp:], [0], [ignore]) +OVS_WAIT_UNTIL([test -e controller]) +dnl Setup ovs +OVS_TRAFFIC_VSWITCHD_START([-- set bridge br0 protocols=OpenFlow15 -- set-controller br0 tcp:127.0.0.1:6653]) + +ADD_NAMESPACES(at_ns0, at_ns1) + +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") + +NS_CHECK_EXEC([at_ns0], [ping -q -c 10 -i 0.1 -w 2 10.1.1.2 | FORMAT_PING], [0], [dnl +10 packets transmitted, 10 received, 0% packet loss, time 0ms +]) +kill `cat ovs-testcontroller.pid` +OVS_WAIT_UNTIL([! test -e controller]) +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP
The OpenFlow15 Packet-Out message contains the match instead of the in_port. The flow.tunnel.metadata.tab is not inited but used in the loop of tun_metadata_to_nx_match. The coredump gdb backtrace is: #0 memcpy_from_metadata (dst=dst@entry=0x7ffcfac2f060, src=src@entry=0x7ffcfac30880, loc=loc@entry=0x10) at lib/tun-metadata.c:467 #1 0x00000000004506e8 in metadata_loc_from_match_read (match=0x7ffcfac30598, is_masked=<synthetic pointer>, mask=0x7ffcfac30838, idx=0, map=0x0) at lib/tun-metadata.c:865 #2 metadata_loc_from_match_read (is_masked=<synthetic pointer>, mask=0x7ffcfac30838, idx=0, match=0x7ffcfac30598, map=0x0) at lib/tun-metadata.c:854 #3 tun_metadata_to_nx_match (b=b@entry=0x892260, oxm=oxm@entry=OFP15_VERSION, match=match@entry=0x7ffcfac30598) at lib/tun-metadata.c:888 #4 0x000000000047c1f8 in nx_put_raw (b=b@entry=0x892260, oxm=oxm@entry=OFP15_VERSION, match=match@entry=0x7ffcfac30598, cookie=<optimized out>, cookie@entry=0, cookie_mask=<optimized out>, cookie_mask@entry=0) at lib/nx-match.c:1186 #5 0x000000000047d693 in oxm_put_match (b=b@entry=0x892260, match=match@entry=0x7ffcfac30598, version=version@entry=OFP15_VERSION) at lib/nx-match.c:1343 #6 0x000000000043194e in ofputil_encode_packet_out (po=po@entry=0x7ffcfac30580, protocol=<optimized out>) at lib/ofp-packet.c:1226 #7 0x000000000040a4fe in process_packet_in (sw=sw@entry=0x891d70, oh=<optimized out>) at lib/learning-switch.c:619 #8 0x000000000040acdc in lswitch_process_packet (msg=0x892210, sw=0x891d70) at lib/learning-switch.c:374 #9 lswitch_run (sw=0x891d70) at lib/learning-switch.c:324 #10 0x0000000000406f26 in main (argc=<optimized out>, argv=<optimized out>) at utilities/ovs-testcontroller.c:180 Fix that by zeroing out the po variable. Fixes: 577bfa9f6879 ("ofp-util: Add OpenFlow 1.5 packet-out support") Signed-off-by: Faicker Mo <faicker.mo@ucloud.cn> --- v2: - changed the init and add test case --- lib/learning-switch.c | 2 +- tests/system-traffic.at | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-)