[ovs-dev] [ovs-discuss] Patch for PACKET_OUT getting deleted twice crash

Ilya Maximets i.maximets at ovn.org
Tue Jun 15 08:34:05 UTC 2021


On 6/15/21 4:31 AM, Tony van der Peet wrote:
> 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);

You're leaking the packet here and destroying the copy later, because
dp_netdev_execute_actions() steals it, so no packet left at all.
'execute->packet' should stay as is, cloned packet should be added to
the 'pp' batch.  Something like:

    dp_packet_batch_init_packet(&pp, 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 <http://pmd.at:657>)
> 1018: ofproto-dpif - active-backup bonding (with primary) FAILED (ofproto-dpif.at:70 <http://ofproto-dpif.at:70>)
> 1020: ofproto-dpif - active-backup bonding (without primary) FAILED (ofproto-dpif.at:288 <http://ofproto-dpif.at:288>)
> 1184: ofproto-dpif megaflow - normal, balance-tcp bonding FAILED (ofproto-dpif.at:8664 <http://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.vanderpeet at gmail.com <mailto:tony.vanderpeet at gmail.com>> wrote:
> 
>     Yes, I can create that patch.
> 
>     Tony
> 
>     On Sat, Jun 12, 2021 at 5:24 AM Ilya Maximets <i.maximets at ovn.org <mailto:i.maximets at ovn.org>> wrote:
> 
>         On 6/9/21 3:12 PM, Aaron Conole wrote:
>         > Ilya Maximets <i.maximets at ovn.org <mailto:i.maximets at 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.maximets at ovn.org <mailto:i.maximets at 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-aconole@redhat.com/ <https://patchwork.ozlabs.org/project/openvswitch/patch/20210521175905.165779-1-aconole@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-aconole@redhat.com/ <https://patchwork.ozlabs.org/project/openvswitch/patch/20210521175905.165779-1-aconole@redhat.com/>
>         >>>>>
>         >>>>> Non-line-wrapped version of the test for convenience:
>         >>>>>
>         >>>>> diff --git a/tests/ofproto-dpif.at <http://ofproto-dpif.at> b/tests/ofproto-dpif.at <http://ofproto-dpif.at>
>         >>>>> index 31064ed95..d01f438b8 100644
>         >>>>> --- a/tests/ofproto-dpif.at <http://ofproto-dpif.at>
>         >>>>> +++ b/tests/ofproto-dpif.at <http://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
>         >>>>> ---
>         >>>>
>         >>>
>         >
> 



More information about the dev mailing list