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.
>> +
>> + 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
