On 30 Jul 2026 at 01:49:16 PM, Aaron Conole <[email protected]> wrote:
--text follows this line--
> 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);
optional nit: I guess these two assertions look a bit redundant as we
check the exact value in the next one. At least CT_CONN_PRIVATE_MAX
should not be possible.
> +
> + /* 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;
nit: looks checkpatch.pl does not flag this. Is it a false negative?
> +
> + 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
