Currently, if a conntrack submodule wants to add per-connection
private details, the pattern looks like:
struct private_conn {
struct conn conn_;
... private data ...
}
...
new_conn = xalloc(sizeof struct private_conn);
...
return &new_conn->conn_;
...
struct private_conn *module_conn = (struct private_conn *)conn_;
This is a common pattern where the underlying allocations are
delegated to the submodule areas, and the main processing module
always assumes that each module allocates a conn_ storage area at the
head of the connection struct anyway.
However, this means that some storage details can't be shared in a
convenient way between modules without leaking details about the
underlying implementations of the module. For example, TCP based
connections may want to share some TCP block details, but not want to
expose the full private TCP connection module internals.
To facilitate this, introduce a private storage section into
connection objects. This will allow storing pre-defined details that
each module can fill and guarantee some kind of compatibility without
needing to completely expose the internals. Because this affects how
the conn objects get initialized, add a new init routine and call it
at all the init sites.
Assisted-by: Claude Sonnet 4.6 <[email protected]>
Signed-off-by: Aaron Conole <[email protected]>
---
lib/conntrack-icmp.c | 2 +-
lib/conntrack-other.c | 2 +-
lib/conntrack-private.h | 46 +++++++++
lib/conntrack-tcp.c | 1 +
lib/conntrack.c | 46 ++++++++-
lib/conntrack.h | 42 ++++++++
tests/library.at | 24 +++++
tests/test-conntrack.c | 177 +++++++++++++++++++++++++++++++++-
utilities/checkpatch_dict.txt | 1 +
9 files changed, 336 insertions(+), 5 deletions(-)
diff --git a/lib/conntrack-icmp.c b/lib/conntrack-icmp.c
index b402970398..12b480bc00 100644
--- a/lib/conntrack-icmp.c
+++ b/lib/conntrack-icmp.c
@@ -88,7 +88,7 @@ icmp_new_conn(struct conntrack *ct, struct dp_packet *pkt
OVS_UNUSED,
struct conn_icmp *conn = xzalloc(sizeof *conn);
conn->state = ICMPS_FIRST;
conn->up.tp_id = tp_id;
-
+ conn_init(&conn->up);
conn_init_expiration(ct, &conn->up, icmp_timeouts[conn->state], now);
return &conn->up;
}
diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c
index 7f3e63c384..788cbbe0e5 100644
--- a/lib/conntrack-other.c
+++ b/lib/conntrack-other.c
@@ -78,7 +78,7 @@ other_new_conn(struct conntrack *ct, struct dp_packet *pkt
OVS_UNUSED,
conn = xzalloc(sizeof *conn);
conn->state = OTHERS_FIRST;
conn->up.tp_id = tp_id;
-
+ conn_init(&conn->up);
conn_init_expiration(ct, &conn->up, other_timeouts[conn->state], now);
return &conn->up;
diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index f1132e8aa8..91a9ba569f 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -156,6 +156,10 @@ struct conn {
bool alg_related; /* True if alg data connection. */
uint32_t tp_id; /* Timeout policy ID. */
+
+ /* Private per-module storage. Indexed by ct_private_id_t values obtained
+ * via conn_private_id_alloc(). Access is protected by conn->lock. */
+ void *private[CT_CONN_PRIVATE_MAX];
};
enum ct_update_res {
@@ -264,4 +268,46 @@ struct ct_l4_proto {
struct ct_dpif_protoinfo *);
};
+/* Initialize the mutex and reclaimed flag of a freshly allocated conn.
+ * Must be called before conn->lock is acquired or before the conn is made
+ * visible to other threads. Each L4 new_conn callback is responsible for
+ * calling this on the conn it allocates. */
+static inline void
+conn_init(struct conn *conn)
+{
+ ovs_mutex_init_adaptive(&conn->lock);
+ atomic_flag_clear(&conn->reclaimed);
+}
+
+/* conn_private_get() / conn_private_set()
+ *
+ * Fast-path accessors for per-connection private storage slots.
+ * The caller must hold conn->lock when accessing the pointer.
+ *
+ * Both functions silently no-op when 'id' is CT_PRIVATE_ID_INVALID, so
+ * callers need not guard against an unallocated slot: get() returns NULL
+ * and set() discards the value.
+ */
+static inline void *
+conn_private_get(const struct conn *conn, ct_private_id_t id)
+ OVS_REQUIRES(conn->lock)
+{
+ if (id == CT_PRIVATE_ID_INVALID) {
+ return NULL;
+ }
+ ovs_assert(id < CT_CONN_PRIVATE_MAX);
+ return conn->private[id];
+}
+
+static inline void
+conn_private_set(struct conn *conn, ct_private_id_t id, void *data)
+ OVS_REQUIRES(conn->lock)
+{
+ if (id == CT_PRIVATE_ID_INVALID) {
+ return;
+ }
+ ovs_assert(id < CT_CONN_PRIVATE_MAX);
+ conn->private[id] = data;
+}
+
#endif /* conntrack-private.h */
diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c
index 8a7c98cc45..cd857cd334 100644
--- a/lib/conntrack-tcp.c
+++ b/lib/conntrack-tcp.c
@@ -474,6 +474,7 @@ tcp_new_conn(struct conntrack *ct, struct dp_packet *pkt,
long long now,
dst->state = CT_DPIF_TCPS_CLOSED;
newconn->up.tp_id = tp_id;
+ conn_init(&newconn->up);
conn_init_expiration(ct, &newconn->up, CT_TM_TCP_FIRST_PACKET, now);
return &newconn->up;
diff --git a/lib/conntrack.c b/lib/conntrack.c
index f84cdd216a..52942041ab 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -157,6 +157,19 @@ expectation_clean(struct conntrack *ct, const struct
conn_key *parent_key);
static struct ct_l4_proto *l4_protos[UINT8_MAX + 1];
+/* Private per-connection storage slot registry.
+ *
+ * ct_private_slots[] is written once per slot at module initialization (via
+ * conn_private_id_alloc()) and then read-only for the lifetime of the process,
+ * so no additional locking is required to read the destructor pointer.
+ */
+struct ct_private_slot {
+ void (*destructor)(void *); /* NULL means no cleanup required. */
+};
+
+static struct ct_private_slot ct_private_slots[CT_CONN_PRIVATE_MAX];
+static atomic_uint32_t ct_private_next_id = 0;
+
static void
handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
struct dp_packet *pkt, struct conn *ec, long long now,
@@ -607,6 +620,27 @@ conn_force_expire(struct conn *conn)
atomic_store_relaxed(&conn->expiration, 0);
}
+ct_private_id_t
+conn_private_id_alloc(void (*destructor)(void *))
+{
+ uint32_t id;
+
+ atomic_add(&ct_private_next_id, 1u, &id);
+ if (id >= CT_CONN_PRIVATE_MAX) {
+ /* Undo the increment so the counter doesn't overflow.
+ * Because we are not supposed to call this after ct initialization,
+ * there shouldn't be an access race here. */
+ atomic_sub(&ct_private_next_id, 1u, &id);
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+ VLOG_ERR_RL(&rl, "conntrack: all %d private storage slots are in use; "
+ "cannot allocate a new one", CT_CONN_PRIVATE_MAX);
+ return CT_PRIVATE_ID_INVALID;
+ }
+
+ ct_private_slots[id].destructor = destructor;
+ return id;
+}
+
/* Destroys the connection tracker 'ct' and frees all the allocated memory.
* The caller of this function must already have shut down packet input
* and PMD threads (which would have been quiesced). */
@@ -1082,8 +1116,6 @@ conn_not_found(struct conntrack *ct, struct dp_packet
*pkt,
nc->parent_key = alg_exp->parent_key;
}
- ovs_mutex_init_adaptive(&nc->lock);
- atomic_flag_clear(&nc->reclaimed);
fwd_key_node->dir = CT_DIR_FWD;
rev_key_node->dir = CT_DIR_REV;
@@ -2716,6 +2748,16 @@ new_conn(struct conntrack *ct, struct dp_packet *pkt,
struct conn_key *key,
static void
delete_conn__(struct conn *conn)
{
+ uint32_t n;
+
+ /* Invoke registered destructors for any non-NULL private slots. */
+ atomic_read_relaxed(&ct_private_next_id, &n);
+ for (uint32_t i = 0; i < n; i++) {
+ if (ct_private_slots[i].destructor && conn->private[i]) {
+ ct_private_slots[i].destructor(conn->private[i]);
+ }
+ }
+
free(conn->alg);
free(conn);
}
diff --git a/lib/conntrack.h b/lib/conntrack.h
index c3136e9554..0f791e75b4 100644
--- a/lib/conntrack.h
+++ b/lib/conntrack.h
@@ -91,6 +91,48 @@ struct nat_action_info_t {
uint16_t nat_flags;
};
+/* Private per-connection storage slots.
+ *
+ * Modules (protocol handlers, offload interfaces, etc.) can reserve a slot
+ * at initialization time and use it to attach private data to every tracked
+ * connection. Slot IDs are small integers that index directly into a fixed-
+ * size array inside struct conn, so get/set operations are O(1) and branch-
+ * free, safe to call on the datapath fast path.
+ *
+ * Usage
+ * -----
+ * // At module initialization, allocate and store the returned id.
+ * static ct_private_id_t my_id = CT_PRIVATE_ID_INVALID;
+ * my_id = conn_private_id_alloc(my_conn_data_free);
+ * if (my_id == CT_PRIVATE_ID_INVALID) {
+ * VLOG_ERR("failed to allocate private storage slot");
+ * }
+ *
+ * // On the fast path, caller must hold conn->lock.
+ * conn_private_set(conn, my_id, my_data);
+ * my_data = conn_private_get(conn, my_id);
+ *
+ * Thread-safety
+ * -------------
+ * The pointer slot itself is protected by conn->lock. The pointed-to data
+ * is the responsibility of the registering module.
+ */
+
+/* Maximum number of private storage slots available per connection. */
+#define CT_CONN_PRIVATE_MAX 8
+
+typedef uint32_t ct_private_id_t;
+
+/* Returned by conn_private_id_alloc() when no slots remain. */
+#define CT_PRIVATE_ID_INVALID UINT32_MAX
+
+/* Allocate a private storage slot. 'destructor' (may be NULL) is called with
+ * the stored pointer when a connection is freed; the destructor is only
+ * invoked for non-NULL slot values. Returns CT_PRIVATE_ID_INVALID on failure
+ * (all slots taken). Must be called before any connection is created that
+ * should carry this slot (i.e. at module initialization time). */
+ct_private_id_t conn_private_id_alloc(void (*destructor)(void *));
+
struct conntrack *conntrack_init(void);
void conntrack_destroy(struct conntrack *);
diff --git a/tests/library.at b/tests/library.at
index 80ebe6ed8a..df4b8d2886 100644
--- a/tests/library.at
+++ b/tests/library.at
@@ -296,3 +296,27 @@ AT_CLEANUP
AT_SETUP([Conntrack Library - FTP ALG parsing])
AT_CHECK([ovstest test-conntrack ftp-alg-large-payload])
AT_CLEANUP
+
+AT_SETUP([conntrack private storage - id alloc])
+AT_KEYWORDS([conntrack])
+AT_CHECK([ovstest test-conntrack private-id-alloc], [0], [.
+])
+AT_CLEANUP
+
+AT_SETUP([conntrack private storage - slot exhaustion])
+AT_KEYWORDS([conntrack])
+AT_CHECK([ovstest test-conntrack private-id-exhaustion], [0], [.........
+], [ignore])
+AT_CLEANUP
+
+AT_SETUP([conntrack private storage - destructor])
+AT_KEYWORDS([conntrack])
+AT_CHECK([ovstest test-conntrack private-destructor], [0], [.
+])
+AT_CLEANUP
+
+AT_SETUP([conntrack private storage - multi slot])
+AT_KEYWORDS([conntrack])
+AT_CHECK([ovstest test-conntrack private-multi-slot], [0], [.
+])
+AT_CLEANUP
diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c
index 2babe989c4..6d29b1e681 100644
--- a/tests/test-conntrack.c
+++ b/tests/test-conntrack.c
@@ -16,6 +16,7 @@
#include <config.h>
#include "conntrack.h"
+#include "conntrack-private.h"
#include "dp-packet.h"
#include "fatal-signal.h"
@@ -496,7 +497,7 @@ test_pcap(struct ovs_cmdl_context *ctx)
ovs_pcap_close(pcap);
}
-/* ALG related testing. */
+/* Conntrack functional testing. */
/* FTP IPv4 PORT payload for testing. */
#define FTP_PORT_CMD_STR "PORT 192,168,123,2,113,42\r\n"
@@ -576,6 +577,169 @@ test_ftp_alg_large_payload(struct ovs_cmdl_context *ctx
OVS_UNUSED)
conntrack_destroy(ct);
}
+/* Verify that conn_private_id_alloc() returns a valid slot ID and that the
+ * idiomatic "store the ID in a static variable at module init" pattern works.
+ */
+static void
+test_private_id_alloc(struct ovs_cmdl_context *ctx OVS_UNUSED)
+{
+ /* Mirrors the real-world pattern: a module stores its slot ID in a static
+ * so it is initialised once and available everywhere in the translation
+ * unit. */
+ static ct_private_id_t my_id = CT_PRIVATE_ID_INVALID;
+
+ my_id = conn_private_id_alloc(NULL);
+
+ ovs_assert(my_id != CT_PRIVATE_ID_INVALID);
+
+ ovs_assert(my_id < CT_CONN_PRIVATE_MAX);
+
+ /* The first allocation must yield slot 0. */
+ ovs_assert(my_id == 0);
+ printf(".\n");
+}
+
+/* Allocate every available slot and confirm that the next request returns
+ * CT_PRIVATE_ID_INVALID. Each successful allocation prints one dot so the
+ * .at test can verify both the count and the error behaviour.
+ */
+static void
+test_private_id_exhaustion(struct ovs_cmdl_context *ctx OVS_UNUSED)
+{
+ ct_private_id_t ids[CT_CONN_PRIVATE_MAX];
+
+ /* Fill all CT_CONN_PRIVATE_MAX slots. */
+ for (unsigned int i = 0; i < CT_CONN_PRIVATE_MAX; i++) {
+ ids[i] = conn_private_id_alloc(NULL);
+ ovs_assert(ids[i] != CT_PRIVATE_ID_INVALID);
+
+ ovs_assert(ids[i] == i);
+ printf(".");
+ }
+
+ /* The very next allocation must fail. */
+ ct_private_id_t extra = conn_private_id_alloc(NULL);
+ ovs_assert(extra == CT_PRIVATE_ID_INVALID);
+ printf(".\n");
+}
+
+/* Globals written by the destructor callback used in test 3. */
+static int dtor_call_count = 0;
+static void *dtor_last_ptr = NULL;
+
+static void
+record_destructor(void *data)
+{
+ dtor_call_count++;
+ dtor_last_ptr = data;
+}
+
+/* Register a destructor, commit a real connection, attach a sentinel pointer
+ * as private data, then destroy the conntrack instance. After draining the
+ * RCU queue (ovsrcu_exit) the destructor must have been called exactly
+ * once with the sentinel value.
+ */
+static uintptr_t ERRPTR;
+
+static void
+test_private_destructor(struct ovs_cmdl_context *ctx OVS_UNUSED)
+{
+ /* Sentinel: a non-NULL pointer value we can identify unambiguously.
+ * ERRPTR is defined above in case we want to use it in the future as
+ * a platform-agnostic and portable sentinel value rather than some
+ * hardcoded hex. */
+ void *sentinel = (void *)(uintptr_t)&ERRPTR;
+
+ static ct_private_id_t dtor_id = CT_PRIVATE_ID_INVALID;
+ dtor_id = conn_private_id_alloc(record_destructor);
+ ovs_assert(dtor_id != CT_PRIVATE_ID_INVALID);
+
+ /* Create a conntrack instance and commit one UDP connection. */
+ struct conntrack *lct = conntrack_init();
+ ovs_be16 dl_type;
+ struct dp_packet *pkt = build_packet(1, 2, &dl_type);
+ struct dp_packet_batch batch;
+ dp_packet_batch_init(&batch);
+ dp_packet_batch_add(&batch, pkt);
+
+ long long now = time_msec();
+ conntrack_execute(lct, &batch, dl_type, false, true, 0,
+ NULL, NULL, NULL, NULL, now, 0);
+
+ /* After a committed execute the packet carries a cached conn pointer. */
+ struct conn *conn = pkt->md.conn;
+ ovs_assert(conn != NULL);
+
+ /* Attach the sentinel as private data for our slot. */
+ ovs_mutex_lock(&conn->lock);
+ conn_private_set(conn, dtor_id, sentinel);
+ ovs_mutex_unlock(&conn->lock);
+
+ /* Destroying the tracker flushes all connections, queuing delete_conn()
+ * callbacks via ovsrcu_postpone(). The destructor fires once those
+ * callbacks are processed. */
+ conntrack_destroy(lct);
+
+ /* ovsrcu_exit() stops the urcu background thread and synchronously drains
+ * all pending postponed callbacks (including delete_conn__ / destructor
+ * chain) before returning. ovsrcu_synchronize() is insufficient here: it
+ * only waits for threads to quiesce, not for the urcu thread to have
+ * actually executed the queued callbacks. */
+ ovsrcu_exit();
+
+ ovs_assert(dtor_call_count == 1);
+
+ ovs_assert(dtor_last_ptr == sentinel);
+
+ dp_packet_delete_batch(&batch, true);
+ printf(".\n");
+}
+
+
+/* Verify that multiple slots on the same connection are independent: writing
+ * to slot A does not clobber slot B and vice versa. */
+static void
+test_private_multi_slot(struct ovs_cmdl_context *ctx OVS_UNUSED)
+{
+ ct_private_id_t id_a = conn_private_id_alloc(NULL);
+ ct_private_id_t id_b = conn_private_id_alloc(NULL);
+ ovs_assert(id_a != CT_PRIVATE_ID_INVALID);
+ ovs_assert(id_b != CT_PRIVATE_ID_INVALID);
+ ovs_assert(id_a != id_b);
+
+ struct conntrack *lct = conntrack_init();
+ ovs_be16 dl_type;
+ struct dp_packet *pkt = build_packet(1, 2, &dl_type);
+ struct dp_packet_batch batch;
+ dp_packet_batch_init(&batch);
+ dp_packet_batch_add(&batch, pkt);
+
+ long long now = time_msec();
+ conntrack_execute(lct, &batch, dl_type, false, true, 0,
+ NULL, NULL, NULL, NULL, now, 0);
+
+ struct conn *conn = pkt->md.conn;
+ ovs_assert(conn != NULL);
+
+ void *sentinel_a = (void *) (uintptr_t) 0xAAAAULL;
+ void *sentinel_b = (void *) (uintptr_t) 0xBBBBULL;
+
+ ovs_mutex_lock(&conn->lock);
+ conn_private_set(conn, id_a, sentinel_a);
+ conn_private_set(conn, id_b, sentinel_b);
+ ovs_assert(conn_private_get(conn, id_a) == sentinel_a);
+ ovs_assert(conn_private_get(conn, id_b) == sentinel_b);
+ /* Overwrite A and confirm B is unchanged. */
+ conn_private_set(conn, id_a, NULL);
+ ovs_assert(conn_private_get(conn, id_a) == NULL);
+ ovs_assert(conn_private_get(conn, id_b) == sentinel_b);
+ ovs_mutex_unlock(&conn->lock);
+
+ conntrack_destroy(lct);
+ ovsrcu_exit();
+ dp_packet_delete_batch(&batch, true);
+ printf(".\n");
+}
static const struct ovs_cmdl_command commands[] = {
/* Connection tracker tests. */
@@ -601,6 +765,17 @@ static const struct ovs_cmdl_command commands[] = {
* is rewritten to the SNAT target rather than causing a crash. */
{"ftp-alg-large-payload", "", 0, 0,
test_ftp_alg_large_payload, OVS_RO},
+ /* Private per-connection storage registry tests.
+ * Each MUST be run as a separate ovstest invocation so the process-global
+ * slot counter is fresh (starts at 0). */
+ {"private-id-alloc", "", 0, 0,
+ test_private_id_alloc, OVS_RO},
+ {"private-id-exhaustion", "", 0, 0,
+ test_private_id_exhaustion, OVS_RO},
+ {"private-destructor", "", 0, 0,
+ test_private_destructor, OVS_RO},
+ {"private-multi-slot", "", 0, 0,
+ test_private_multi_slot, OVS_RO},
{NULL, NULL, 0, 0, NULL, OVS_RO},
};
diff --git a/utilities/checkpatch_dict.txt b/utilities/checkpatch_dict.txt
index cb25a86ead..ce01743bd9 100644
--- a/utilities/checkpatch_dict.txt
+++ b/utilities/checkpatch_dict.txt
@@ -302,6 +302,7 @@ unixctl
untrusted
upcall
upcalls
+urcu
us
usec
userspace
--
2.51.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
