On 14 Sep 2026 at 11:48:29 AM, Aaron Conole <[email protected]> wrote:
> Paolo Valerio <[email protected]> writes: > >> 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. > > Fair enough. > >>> + >>> + /* 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? > > You mean checkpatch.py ? Hrrm, looks like a bug in checkpatch; okay. > Oops, yes, checkpatch.py :) >>> + >>> + 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
