[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