diff mbox series

[ovs-dev,1/3] netdev-vport: Fix unsafe handling of GRE sequence number.

Message ID 20230520003120.1070717-2-i.maximets@ovn.org
State Accepted
Commit be6f096fbe5e75b1969d10c1b3813499fbc32d9a
Headers show
Series netdev-vport: Fix tunnel config thread safety. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Ilya Maximets May 20, 2023, 12:31 a.m. UTC 
GRE sequence number is maintained as part of the tunnel config.
This triggers tunnel reconfiguration every time set_tunnel_config()
is called, because memset over tunnel config will never be equal to
the new config constructed from database options.

And sequence number incremented non-atomically without holding a
mutex on tunnel push, that may lead to corruption if multiple
threads are sending packets to the same tunnel.

Fix that by moving sequence number to the netdev_vport structure
instead and using an atomic counter.

Fixes: 0ffff4975308 ("userspace: add gre sequence number support.")
Fixes: 7dc18ae96d33 ("userspace: add erspan tunnel support.")
Fixes: 3c6d05a02e0f ("userspace: Add GTP-U support.")
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 lib/netdev-native-tnl.c    | 14 ++++----------
 lib/netdev-vport-private.h |  4 ++++
 lib/netdev-vport.c         |  2 ++
 lib/netdev.h               |  1 -
 4 files changed, 10 insertions(+), 11 deletions(-)

Comments

Simon Horman May 25, 2023, 11:50 a.m. UTC | #1 
On Sat, May 20, 2023 at 02:31:18AM +0200, Ilya Maximets wrote:
> GRE sequence number is maintained as part of the tunnel config.
> This triggers tunnel reconfiguration every time set_tunnel_config()
> is called, because memset over tunnel config will never be equal to
> the new config constructed from database options.
> 
> And sequence number incremented non-atomically without holding a
> mutex on tunnel push, that may lead to corruption if multiple
> threads are sending packets to the same tunnel.
> 
> Fix that by moving sequence number to the netdev_vport structure
> instead and using an atomic counter.
> 
> Fixes: 0ffff4975308 ("userspace: add gre sequence number support.")
> Fixes: 7dc18ae96d33 ("userspace: add erspan tunnel support.")
> Fixes: 3c6d05a02e0f ("userspace: Add GTP-U support.")
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Reviewed-by: Simon Horman <simon.horman@corigine.com>
diff mbox series

Patch

diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
index 9abdf5107..e31d61dd5 100644
--- a/lib/netdev-native-tnl.c
+++ b/lib/netdev-native-tnl.c
@@ -452,7 +452,6 @@  netdev_gre_push_header(const struct netdev *netdev,
                        const struct ovs_action_push_tnl *data)
 {
     struct netdev_vport *dev = netdev_vport_cast(netdev);
-    struct netdev_tunnel_config *tnl_cfg;
     struct gre_base_hdr *greh;
     int ip_tot_size;
 
@@ -468,8 +467,7 @@  netdev_gre_push_header(const struct netdev *netdev,
         int seq_ofs = gre_header_len(greh->flags) - 4;
         ovs_16aligned_be32 *seq_opt =
             ALIGNED_CAST(ovs_16aligned_be32 *, (char *)greh + seq_ofs);
-        tnl_cfg = &dev->tnl_cfg;
-        put_16aligned_be32(seq_opt, htonl(tnl_cfg->seqno++));
+        put_16aligned_be32(seq_opt, htonl(atomic_count_inc(&dev->gre_seqno)));
     }
 }
 
@@ -605,7 +603,6 @@  netdev_erspan_push_header(const struct netdev *netdev,
                           const struct ovs_action_push_tnl *data)
 {
     struct netdev_vport *dev = netdev_vport_cast(netdev);
-    struct netdev_tunnel_config *tnl_cfg;
     struct erspan_base_hdr *ersh;
     struct gre_base_hdr *greh;
     struct erspan_md2 *md2;
@@ -615,9 +612,8 @@  netdev_erspan_push_header(const struct netdev *netdev,
                                      data->header_len, &ip_tot_size);
 
     /* update GRE seqno */
-    tnl_cfg = &dev->tnl_cfg;
     ovs_16aligned_be32 *seqno = (ovs_16aligned_be32 *) (greh + 1);
-    put_16aligned_be32(seqno, htonl(tnl_cfg->seqno++));
+    put_16aligned_be32(seqno, htonl(atomic_count_inc(&dev->gre_seqno)));
 
     /* update v2 timestamp */
     if (greh->protocol == htons(ETH_TYPE_ERSPAN2)) {
@@ -786,7 +782,6 @@  netdev_gtpu_push_header(const struct netdev *netdev,
                         const struct ovs_action_push_tnl *data)
 {
     struct netdev_vport *dev = netdev_vport_cast(netdev);
-    struct netdev_tunnel_config *tnl_cfg;
     struct udp_header *udp;
     struct gtpuhdr *gtpuh;
     int ip_tot_size;
@@ -801,10 +796,9 @@  netdev_gtpu_push_header(const struct netdev *netdev,
 
     gtpuh = ALIGNED_CAST(struct gtpuhdr *, udp + 1);
 
-    tnl_cfg = &dev->tnl_cfg;
-    if (tnl_cfg->set_seq) {
+    if (gtpuh->md.flags & GTPU_S_MASK) {
         ovs_be16 *seqno = ALIGNED_CAST(ovs_be16 *, gtpuh + 1);
-        *seqno = htons(tnl_cfg->seqno++);
+        *seqno = htons(atomic_count_inc(&dev->gre_seqno));
         payload_len += sizeof(struct gtpuhdr_opt);
     }
     gtpuh->len = htons(payload_len);
diff --git a/lib/netdev-vport-private.h b/lib/netdev-vport-private.h
index d89a28c66..e3c3bdb43 100644
--- a/lib/netdev-vport-private.h
+++ b/lib/netdev-vport-private.h
@@ -22,11 +22,15 @@ 
 #include "compiler.h"
 #include "netdev.h"
 #include "netdev-provider.h"
+#include "ovs-atomic.h"
 #include "ovs-thread.h"
 
 struct netdev_vport {
     struct netdev up;
 
+    /* Sequence number for outgoing GRE packets. */
+    atomic_count gre_seqno;
+
     /* Protects all members below. */
     struct ovs_mutex mutex;
 
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 663ee8606..6bbaa2feb 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -37,6 +37,7 @@ 
 #include "netdev-provider.h"
 #include "netdev-vport-private.h"
 #include "openvswitch/dynamic-string.h"
+#include "ovs-atomic.h"
 #include "ovs-router.h"
 #include "packets.h"
 #include "openvswitch/poll-loop.h"
@@ -198,6 +199,7 @@  netdev_vport_construct(struct netdev *netdev_)
     uint16_t port = 0;
 
     ovs_mutex_init(&dev->mutex);
+    atomic_count_init(&dev->gre_seqno, 0);
     eth_addr_random(&dev->etheraddr);
 
     if (name && dpif_port && (strlen(name) > strlen(dpif_port) + 1) &&
diff --git a/lib/netdev.h b/lib/netdev.h
index ff207f56c..1fab91273 100644
--- a/lib/netdev.h
+++ b/lib/netdev.h
@@ -130,7 +130,6 @@  struct netdev_tunnel_config {
     enum netdev_pt_mode pt_mode;
 
     bool set_seq;
-    uint32_t seqno;
     uint32_t erspan_idx;
     uint8_t erspan_ver;
     uint8_t erspan_dir;