I did the patch as suggested, and it looks something like this: --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4168,9 +4168,13 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute) flow_hash_5tuple(execute->flow, 0)); }
+ /* We are not losing the original packet, just relying on its previous + * owner to properly dispose of it. + */ + execute->packet = dp_packet_clone(execute->packet); dp_packet_batch_init_packet(&pp, execute->packet); - pp.do_not_steal = true; - dp_netdev_execute_actions(pmd, &pp, false, execute->flow, + pp.do_not_steal = false; + dp_netdev_execute_actions(pmd, &pp, true, execute->flow, execute->actions, execute->actions_len); dp_netdev_pmd_flush_output_packets(pmd, true); Unfortunately, I now get some errors in the "make check", including: 1012: PMD - monitor threads FAILED (pmd.at:657) 1018: ofproto-dpif - active-backup bonding (with primary) FAILED ( ofproto-dpif.at:70) 1020: ofproto-dpif - active-backup bonding (without primary) FAILED ( ofproto-dpif.at:288) 1184: ofproto-dpif megaflow - normal, balance-tcp bonding FAILED ( ofproto-dpif.at:8664) My earlier analysis of the code showed that a call tree starting from handle_upcalls can also result in dpif_netdev_execute getting called - I wonder if that's what's happening here? I'll keep analysing these new failures, but any pointers would be appreciated. Tony On Tue, Jun 15, 2021 at 12:28 PM Tony van der Peet < tony.vanderp...@gmail.com> wrote: > Yes, I can create that patch. > > Tony > > On Sat, Jun 12, 2021 at 5:24 AM Ilya Maximets <i.maxim...@ovn.org> wrote: > >> On 6/9/21 3:12 PM, Aaron Conole wrote: >> > Ilya Maximets <i.maxim...@ovn.org> writes: >> > >> >> On 6/7/21 3:59 PM, Ilya Maximets wrote: >> >>> On 6/7/21 3:09 PM, Aaron Conole wrote: >> >>>> Ilya Maximets <i.maxim...@ovn.org> writes: >> >>>> >> >>>>>> Here is a patch with both a test and a fix. >> >>>> >> >>>> Thanks so much! It's nice to get fixes, but I think it's really >> great >> >>>> when test cases come along with them. >> >>>> >> >>>>> Hi. Thanks for working n this! >> >>>>> >> >>>>> CC: ovs-dev >> >>>>> >> >>>>>> Not submitting as a formal >> >>>>>> patch because I would like some feedback on whether 1) maintainers >> feel >> >>>>>> this is worth fixing and >> >>>>> >> >>>>> I can reproduce the crash with your test. Basically, actions in >> userspace >> >>>>> datapath may drop packets if something goes wrong. 'meter' action >> just >> >>>>> seems to be the most explicit variant. So, I think, this is >> definitely >> >>>>> worth fixing as some other condition might trigger this crash on >> packet-out >> >>>>> as well. >> >>>>> >> >>>>> ==2568112==ERROR: AddressSanitizer: heap-use-after-free >> >>>>> on address 0x61600000699c at pc 0x000000573860 bp 0x7ffebc6cc880 sp >> 0x7ffebc6cc878 >> >>>>> READ of size 1 at 0x61600000699c thread T0 >> >>>>> #0 0x57385f in dp_packet_delete lib/dp-packet.h:242:16 >> >>>>> #1 0x57372c in ofproto_packet_out_uninit >> ofproto/ofproto.c:3562:5 >> >>>>> #2 0x585e77 in handle_packet_out ofproto/ofproto.c:3722:5 >> >>>>> #3 0x583801 in handle_single_part_openflow >> ofproto/ofproto.c:8499:16 >> >>>>> #4 0x570c9c in handle_openflow ofproto/ofproto.c:8686:21 >> >>>>> #5 0x611781 in ofconn_run ofproto/connmgr.c:1329:13 >> >>>>> #6 0x6112ed in connmgr_run ofproto/connmgr.c:356:9 >> >>>>> #7 0x56fdf4 in ofproto_run ofproto/ofproto.c:1891:5 >> >>>>> #8 0x545ec0 in bridge_run__ vswitchd/bridge.c:3251:9 >> >>>>> #9 0x5456a5 in bridge_run vswitchd/bridge.c:3310:5 >> >>>>> #10 0x55f5b1 in main vswitchd/ovs-vswitchd.c:127:9 >> >>>>> #11 0x7f85bfe09081 in __libc_start_main >> (/lib64/libc.so.6+0x27081) >> >>>>> #12 0x46d00d in _start (vswitchd/ovs-vswitchd+0x46d00d) >> >>>>> >> >>>>>> 2) whether this is the way to fix it. >> >>>>> >> >>>>> See inline. >> >>>>> >> >>>>>> >> >>>>>> I have tried to make the most minimal change possible, but this >> means that >> >>>>>> there might be paths through the code that give unexpected >> behaviour (which >> >>>>>> in the worst case would be a memory leak I suppose). >> >>>>>> >> >>>>>> Tony >> >>>>>> >> >>>>>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h >> >>>>>> index 246be14d0..5e0dabe67 100644 >> >>>>>> --- a/lib/dp-packet.h >> >>>>>> +++ b/lib/dp-packet.h >> >>>>>> @@ -739,6 +739,7 @@ struct dp_packet_batch { >> >>>>>> size_t count; >> >>>>>> bool trunc; /* true if the batch needs truncate. */ >> >>>>>> bool do_not_steal; /* Indicate that the packets should not be >> stolen. >> >>>>>> */ >> >>>>>> + bool packet_out; /* Indicate single packet is PACKET_OUT */ >> >>>>>> struct dp_packet *packets[NETDEV_MAX_BURST]; >> >>>>>> }; >> >>>>>> >> >>>>>> @@ -748,6 +749,7 @@ dp_packet_batch_init(struct dp_packet_batch >> *batch) >> >>>>>> batch->count = 0; >> >>>>>> batch->trunc = false; >> >>>>>> batch->do_not_steal = false; >> >>>>>> + batch->packet_out = false; >> >>>>>> } >> >>>>>> >> >>>>>> static inline void >> >>>>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> >>>>>> index 650e67ab3..deba4a94a 100644 >> >>>>>> --- a/lib/dpif-netdev.c >> >>>>>> +++ b/lib/dpif-netdev.c >> >>>>>> @@ -4170,6 +4170,7 @@ dpif_netdev_execute(struct dpif *dpif, struct >> >>>>>> dpif_execute *execute) >> >>>>>> >> >>>>>> dp_packet_batch_init_packet(&pp, execute->packet); >> >>>>>> pp.do_not_steal = true; >> >>>>>> + pp.packet_out = execute->packet_out; >> >>>>>> dp_netdev_execute_actions(pmd, &pp, false, execute->flow, >> >>>>>> execute->actions, >> execute->actions_len); >> >>>>> >> >>>>> There is already a dirty hack named "do_not_steal" that was >> introduced, >> >>>>> I guess, exactly to avoid crash in the conntrack code that could >> drop/steal >> >>>>> the packet just like meter action. And it seems that here in >> >>>>> dpif_netdev_execute() is the only problematic entry point as all >> other >> >>>>> normal paths expects that packet might be destroyed. >> >>>>> >> >>>>> The problem was, I suppose, introduced when we tried to unify >> semantics >> >>>>> of "may_steal" flag by turning it into "should_steal". But it >> seems that >> >>>>> in this function we really need to prohibit stealing of the packet >> since >> >>>>> ofproto layer still owns it regardless of the result of execution. >> >>>>> >> >>>>> I don't think that we need one more flag here, but we have several >> options >> >>>>> how to fix the crash: >> >>>>> >> >>>>> 1. Start honoring batch->do_not_steal flag in all actions that may >> result >> >>>>> in packet drops. As the original idea of having 'do_not_steal' >> flag for >> >>>>> a batch is very hacky, I'd like to not do that. >> >>>> >> >>>> +1 >> >>>> >> >>>>> 2. Try to propagate information that packet was deleted up to >> ofproto layer, >> >>>>> i.e. make handle_packet_out() aware of that. Will, probably, be >> not that >> >>>>> easy to do. >> >>>> >> >>>> I had a look at doing this, but as you note it is quite intrusive, >> and >> >>>> we need to make changes all over. >> >>>> >> >>>>> 3. This function (dpif_netdev_execute) is not on a hot path in >> userspace >> >>>>> datapath, IIUC. It might be that it's just easier to remove the >> >>>>> 'do_not_steal' flag entirely, clone the packet here and call the >> >>>>> dp_netdev_execute_actions() with should_steal=true. >> >>>>> This sounds like the best solution for me, unless I overlooked >> some >> >>>>> scenario, where this code is on a hot path. >> >>>> >> >>>> I like this approach. >> >>>> >> >>>>> Thoughts? >> >>>>> >> >>>>> Aaron, you have a patch[1] to remove 'do_not_steal' flag, so fix >> for this issue >> >>>>> will, likely, touch the same parts of the code. What do you think >> about this >> >>>>> issue and possible solutions? >> >>>> >> >>>> I guess we should do the same thing we do in other places, ie: >> default >> >>>> assume that the packet cannot be 'stolen' and we should clone our own >> >>>> copies. >> >>> >> >>> I'm not sure that I understood this correctly, but the idea was that >> default >> >>> assumption is that packet can be stolen at any point of datapath >> processing >> >>> and higher layers should deal with this. >> > >> > Re-reading what I wrote, I don't either. :-/ I agree with what you've >> > written. >> > >> >>> For the IP fragmentation handling this will mean that ipf will just >> take a >> >>> packet directly from the original batch without copying, so the >> original >> >>> batch will not have this packet anymore and ipf is allowed to free it >> at >> >>> any point in time, because now it owns this packet. >> >>> This aligns with the "should_steal" semantics, as any function called >> with >> >>> "should_steal=true" must take the ownership of the packet. If the >> function >> >>> called with "should_steal=false" it still allowed to take ownership >> of some >> >>> packets from the batch and caller must be prepared for that. >> >>> If some function in datapath has no "should_steal" argument, it >> should be >> >>> treated as a function with "should_steal=false". This applies to >> both >> >>> conntrack_execute() and dp_netdev_run_meter(), also to >> netdev_pop_header() >> >>> and so on. >> >> >> >> Hmm, OVS_ACTION_ATTR_TUNNEL_POP implies recirculation, so >> netdev_pop_header() >> >> is not a fully valid example here. Actions that implies >> recirculation or >> >> recirculates packets in any other way (e.g. OVS_ACTION_ATTR_USERSPACE) >> should >> >> clone packets before doing that if not asked to take ownership. >> > >> > The ownership of the dp_packet object needs to be well established. Use >> > of should_steal could be okay, but as an example, we have something in >> > the packet batch but it doesn't actually get honored (which is why I >> > proposed removing it). >> >> Yeah. It looks like we have more issues than I thought initially. >> I thought that ip fragmentation module actually steals packets from >> the original batch, but it doesn't. Instead it tries to hack some >> poor form of reference counting with 'do_not_steal' flag. This also >> creates yet another logical difference with kernel conntrack >> implementation >> that actually steals fragments and therefore stops their processing >> until reassembled. >> >> In the end, I think we can't create a consolidated solution here for >> all problems at once. At least, we can't do that easily. So, what >> I will do is that I'll review and apply Aaron's fix for ipf: >> >> https://patchwork.ozlabs.org/project/openvswitch/patch/20210521175905.165779-1-acon...@redhat.com/ >> >> >> And this bug with crash on packet_out with meters should be fixed by >> cloning the packet inside dpif_netdev_execute() and calling the >> dp_netdev_execute_actions() with should_steal=true. >> >> Tony, could you prepare a patch for this? >> >> Best regards, Ilya Maximets. >> >> > >> >>>> If we are worried about the time it takes to copy the dp_packet >> >>>> structure and buffer, we can always introduce a reference counting >> >>>> mechanism later as an optimization. >> >>>> >> >>>> I would just prefer to do clone, and then the functional area which >> >>>> needs to hold a reference to a valid packet buffer can delete when it >> >>>> makes sense. >> >>>> >> >>>> Hope it helps. >> >>>> >> >>>>> Best regards, Ilya Maximets. >> >>>>> >> >>>>> >> >>>>> [1] >> https://patchwork.ozlabs.org/project/openvswitch/patch/20210521175905.165779-1-acon...@redhat.com/ >> >>>>> >> >>>>> Non-line-wrapped version of the test for convenience: >> >>>>> >> >>>>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> >>>>> index 31064ed95..d01f438b8 100644 >> >>>>> --- a/tests/ofproto-dpif.at >> >>>>> +++ b/tests/ofproto-dpif.at >> >>>>> @@ -2159,6 +2159,27 @@ meter:controller flow_count:0 >> packet_in_count:8 byte_in_count:112 duration:0.0s >> >>>>> OVS_VSWITCHD_STOP >> >>>>> AT_CLEANUP >> >>>>> >> >>>>> +AT_SETUP([ofproto-dpif packet-out table meter drop qwe]) >> >>>>> +OVS_VSWITCHD_START >> >>>>> +add_of_ports br0 1 2 >> >>>>> + >> >>>>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-meter br0 'meter=1 pktps >> bands=type=drop rate=1']) >> >>>>> +AT_CHECK([ovs-ofctl -O OpenFlow13 add-flow br0 'in_port=1 >> action=meter:1,output:2']) >> >>>>> + >> >>>>> +ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 >> packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000400080000 >> actions=resubmit(,0)" >> >>>>> +ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 >> packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000400080000 >> actions=resubmit(,0)" >> >>>>> + >> >>>>> +# Check that vswitchd hasn't crashed by dumping the meter added >> above >> >>>>> +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br0 | ofctl_strip], >> [0], [dnl >> >>>>> +OFPST_METER_CONFIG reply (OF1.3): >> >>>>> +meter=1 pktps bands= >> >>>>> +type=drop rate=1 >> >>>>> +]) >> >>>>> + >> >>>>> +OVS_VSWITCHD_STOP >> >>>>> +AT_CLEANUP >> >>>>> + >> >>>>> + >> >>>>> AT_SETUP([ofproto-dpif - MPLS handling]) >> >>>>> OVS_VSWITCHD_START([dnl >> >>>>> add-port br0 p1 -- set Interface p1 type=dummy >> >>>>> --- >> >>>> >> >>> >> > >> >>
_______________________________________________ discuss mailing list disc...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-discuss