[ovs-discuss] Patch for PACKET_OUT getting deleted twice crash
Tony van der Peet
tony.vanderpeet at gmail.com
Tue Jun 15 02:31:38 UTC 2021
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.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> wrote:
>
>> On 6/9/21 3:12 PM, Aaron Conole wrote:
>> > Ilya Maximets <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> 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/
>>
>>
>> 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/
>> >>>>>
>> >>>>> 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
>> >>>>> ---
>> >>>>
>> >>>
>> >
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-discuss/attachments/20210615/c6baf2dd/attachment-0001.html>
More information about the discuss
mailing list