[ovs-dev] [PATCH] dpif-netdev: Fix crash when PACKET_OUT is metered
Ilya Maximets
i.maximets at ovn.org
Thu Jun 17 18:26:39 UTC 2021
On 6/17/21 7:47 PM, Ilya Maximets wrote:
> On 6/16/21 11:54 PM, Tony van der Peet wrote:
>> Thanks Ilya. For what it's worth, besides running the OVS unit tests, I put this new code through our (enhanced) version of oftest (500 test cases) including a couple I wrote just for this situation.
>>
>> Tony
>>
>> On Thu, Jun 17, 2021 at 8:05 AM Ilya Maximets <i.maximets at ovn.org <mailto:i.maximets at ovn.org>> wrote:
>>
>> On 6/16/21 2:04 AM, Tony van der Peet wrote:
>> > From: Tony van der Peet <tony.vanderpeet at gmail.com <mailto:tony.vanderpeet at gmail.com>>
>> >
>> > When a PACKET_OUT has output port of OFPP_TABLE, and the rule
>> > table includes a meter and this causes the packet to be deleted,
>> > stop the packet from being deleted twice by cloning it and setting
>> > it up to be stolen in execution.
>> >
>> > Add a test to verify this condition.
>> >
>> > Signed-off-by: Tony van der Peet <tony.vanderpeet at gmail.com <mailto:tony.vanderpeet at gmail.com>>
>>
>> Thanks for the patch! OVS seems to work fine with this change,
>> but for some reason several OVN unit tests are failing if it
>> uses OVS with this change applied.
>>
>> Trying to figure out why...
>>
>> Best regards, Ilya Maximets.
>>
>
> OK, I've spent most of a day trying to figure out what is going wrong there
> (mostly because OVN tests are insanely complex and very hard to debug).
>
> In short: dpif_execute() expected to modify the packet by higher layers and
> by cloning the packet inside dp_netdev_execute_actions() all the
> modifications applied to the copy and not propagated to the original packet.
>
> The call stack looks something like this:
>
> 1. handle_packet_out()
> 2. --> ofproto_packet_out_finish()
> 3. --> packet_execute()
> 4. --> dpif_execute()
> 5. --> dpif_operate()
> 6. --> dpif_execute_with_help()
> 7. --> odp_execute_actions()
> 8. ** For each action on a list **:
> 9. --> dpif_execute_helper_cb()
> 10. --> dpif_execute()
> 11. --> dpif_operate()
> 12. --> dp_netdev_execute()
>
> The problem is on a line 8. odp_execute_actions() executes actions one by
> one expecting the datapath to modify the packet. In case of OVN unit tests
> PACKET_OUT resulted in a flow with 2 actions: tunnel push + output.
> So, the first dp_netdev_execute() is called to push the tunnel header to the
> packet and the second time it's called to execute OUTPUT action and send the
> packet to the destination. And since we're cloning the packet, tunnel header
> was lost and bare packet was sent out. This caused failure of OVN unit tests.
>
> It's understandable why odp_execute_actions() executes actions one by one.
> Some actions requires datapath assistance and others could be executed in
> userspace, some actions could be executed in userspace only. So, we have
> to use it this way. But that means that we need to propagate information
> about stolen packet at least to the level of odp_execute_actions(). I tried
> to implement that but, it doesn't look good...
>
> So, technically, what I'm suggesting is to copy the content of the packet
> back after the execution. Still kind of ugly, but, at least localized.
> We also need to report an error condition if the copy was stolen, so upper
> layers will be aware that actions was not successful.
> Something like this:
>
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 08d93c277..860a6c3e7 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -199,6 +199,7 @@ struct dp_packet *dp_packet_clone_data_with_headroom(const void *, size_t,
> void dp_packet_resize(struct dp_packet *b, size_t new_headroom,
> size_t new_tailroom);
> static inline void dp_packet_delete(struct dp_packet *);
> +static inline void dp_packet_swap(struct dp_packet *, struct dp_packet *);
>
> static inline void *dp_packet_at(const struct dp_packet *, size_t offset,
> size_t size);
> @@ -256,6 +257,16 @@ dp_packet_delete(struct dp_packet *b)
> }
> }
>
> +/* Swaps content of two packets. */
> +static inline void
> +dp_packet_swap(struct dp_packet *a, struct dp_packet *b)
> +{
Ugh. This function can not be used for packets originated from
afxdp or dpdk. Luckily, I think, that dpif_netdev_execute() should
never work with such packets, but a bunch of assertions here is
needed. DPBUF_STACK also doesn't sound like a good idea, because
it's intended to be immutable. So:
ovs_assert(a->source == DPBUF_MALLOC || a->source == DPBUF_STUB);
ovs_assert(b->source == DPBUF_MALLOC || b->source == DPBUF_STUB);
> + struct dp_packet c = *a;
> +
> + *a = *b;
> + *b = c;
> +}
> +
> /* If 'b' contains at least 'offset + size' bytes of data, returns a pointer to
> * byte 'offset'. Otherwise, returns a null pointer. */
> static inline void *
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 8fa7eb6d4..a660a2fd6 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -4168,7 +4168,11 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute)
> flow_hash_5tuple(execute->flow, 0));
> }
>
> - dp_packet_batch_init_packet(&pp, execute->packet);
> + /* Making a copy because the packet might be stolen during the execution
> + * and caller might still need it. */
> + struct dp_packet *packet_clone = dp_packet_clone(execute->packet);
> + dp_packet_batch_init_packet(&pp, packet_clone);
> +
> dp_netdev_execute_actions(pmd, &pp, false, execute->flow,
> execute->actions, execute->actions_len);
> dp_netdev_pmd_flush_output_packets(pmd, true);
> @@ -4178,6 +4182,20 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute)
> dp_netdev_pmd_unref(pmd);
> }
>
> + if (dp_packet_batch_size(&pp)) {
> + /* Packet wasn't dropped during the execution. Swapping content with
> + * the original packet, because the caller might expect actions to
> + * modify it. */
> + dp_packet_swap(execute->packet, packet_clone);
> + dp_packet_delete_batch(&pp, true);
> + } else {
> + /* Packet was stolen during the execution due to some error. We need
> + * to flag that for the caller, so it will not proceed with other
> + * actions on this packet. Returning EAGAIN because we just don't
> + * know what execlty happened. */
> + return EAGAIN;
> + }
> +
> return 0;
> }
>
> ---
>
> For the test, following addition will be needed, because the error now will
> be printed:
>
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 522cc1318..1b55aa0c5 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -9537,7 +9537,9 @@ meter=1 pktps bands=
> type=drop rate=1
> ])
>
> -OVS_VSWITCHD_STOP
> +OVS_WAIT_UNTIL([grep 'execute meter(0),2 failed' ovs-vswitchd.log])
> +
> +OVS_VSWITCHD_STOP(["/execute meter(0),2 failed/d"])
> AT_CLEANUP
>
> AT_SETUP([ofproto-dpif - ICMPv6])
> ---
>
> With above changes both OVS and OVN testsuites works fine.
>
> Any thoughts?
>
> Best regards, Ilya Maximets.
>
More information about the dev
mailing list