[ovs-dev] [PATCH] dpif-netdev: Fix crash when PACKET_OUT is metered

Ilya Maximets i.maximets at ovn.org
Thu Jun 17 17:47:32 UTC 2021


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)
+{
+    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