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 <[email protected]
> <mailto:[email protected]>> wrote:
>
> Yes, I can create that patch.
>
> Tony
>
> On Sat, Jun 12, 2021 at 5:24 AM Ilya Maximets <[email protected]
> <mailto:[email protected]>> wrote:
>
> On 6/9/21 3:12 PM, Aaron Conole wrote:
> > Ilya Maximets <[email protected] <mailto:[email protected]>>
> writes:
> >
> >> On 6/7/21 3:59 PM, Ilya Maximets wrote:
> >>> On 6/7/21 3:09 PM, Aaron Conole wrote:
> >>>> Ilya Maximets <[email protected] <mailto:[email protected]>>
> 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/[email protected]/
>
> <https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/>
>
>
> 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/[email protected]/
>
> <https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/>
> >>>>>
> >>>>> 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
> >>>>> ---
> >>>>
> >>>
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
