On Thu, Nov 25, 2021 at 07:17:21PM +0100, Alexander Bluhm wrote:
> To find the ref counting bugs in IPsec tdb I use this trace code.
> Per default there is no change due to #ifdef.
>
> tdb_trace[72]: 3944909: refs 5 +1 cpu1 gettdb_dir:360
> tdb_trace[73]: 3944910: refs 6 +0 cpu1 pfkeyv2_send:1598
> tdb_trace[74]: 3944911: refs 6 +0 cpu1 pfkeyv2_send:1605
> tdb_trace[75]: 3944912: refs 6 -1 cpu1 tdb_deltimeouts:960
> tdb_trace[76]: 3944913: refs 5 -1 cpu1 tdb_deltimeouts:964
> tdb_trace[77]: 3944914: refs 4 -1 cpu1 tdb_delete0:1004
> tdb_trace[78]: 3944915: refs 3 -1 cpu1 pfkeyv2_send:2149
>
> Do we want to have that in tree?
Updated diff below. Should I commit this? Alternatively I can
keep a local patch that I apply when something breaks.
bluhm
Index: netinet/ip_ipsp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.258
diff -u -p -r1.258 ip_ipsp.c
--- netinet/ip_ipsp.c 29 Nov 2021 19:19:00 -0000 1.258
+++ netinet/ip_ipsp.c 1 Dec 2021 18:27:03 -0000
@@ -209,6 +209,11 @@ ipsp_init(void)
M_WAITOK | M_ZERO);
}
+#ifdef TDB_TRACE_MAX
+int tdb_trace_level;
+unsigned int tdb_trace_stamp;
+#endif /* TDB_TRACE_MAX */
+
/*
* Our hashing function needs to stir things with a non-zero random multiplier
* so we cannot be DoS-attacked via choosing of the data to hash.
@@ -452,7 +457,7 @@ ipsp_aux_match(struct tdb *tdb,
* the desired IDs.
*/
struct tdb *
-gettdbbydst(u_int rdomain, union sockaddr_union *dst, u_int8_t sproto,
+gettdbbydst0(u_int rdomain, union sockaddr_union *dst, u_int8_t sproto,
struct ipsec_ids *ids,
struct sockaddr_encap *filter, struct sockaddr_encap *filtermask)
{
@@ -483,7 +488,7 @@ gettdbbydst(u_int rdomain, union sockadd
* the desired IDs.
*/
struct tdb *
-gettdbbysrc(u_int rdomain, union sockaddr_union *src, u_int8_t sproto,
+gettdbbysrc0(u_int rdomain, union sockaddr_union *src, u_int8_t sproto,
struct ipsec_ids *ids,
struct sockaddr_encap *filter, struct sockaddr_encap *filtermask)
{
@@ -608,6 +613,24 @@ tdb_printit(void *addr, int full, int (*
/* tdb_filtermask */
/* tdb_policy_head */
/* tdb_sync_entry */
+#ifdef TDB_TRACE_MAX
+ {
+ struct tdb_trace *tt;
+ unsigned int idx, i;
+
+ DUMP(trace_idx, "%d");
+ for (i = 0; i < TDB_TRACE_MAX; i++) {
+ idx = (tdb->tdb_trace_idx + i) % TDB_TRACE_MAX;
+ tt = &tdb->tdb_trace[idx];
+ if (tt->tt_stamp == 0)
+ continue;
+ pr(" tdb_trace[%2u]: "
+ "%u: refs %u %+d cpu%d %s:%d\n",
+ idx, tt->tt_stamp, tt->tt_refs, tt->tt_op,
+ tt->tt_cpu, tt->tt_func, tt->tt_line);
+ }
+ }
+#endif /* TDB_TRACE_MAX */
} else {
pr("%p:", tdb);
pr(" %08x", ntohl(tdb->tdb_spi));
@@ -960,7 +983,7 @@ tdb_deltimeouts(struct tdb *tdbp)
}
struct tdb *
-tdb_ref(struct tdb *tdb)
+tdb_ref0(struct tdb *tdb)
{
if (tdb == NULL)
return NULL;
@@ -969,7 +992,7 @@ tdb_ref(struct tdb *tdb)
}
void
-tdb_unref(struct tdb *tdb)
+tdb_unref0(struct tdb *tdb)
{
if (tdb == NULL)
return;
@@ -979,7 +1002,7 @@ tdb_unref(struct tdb *tdb)
}
void
-tdb_delete(struct tdb *tdbp)
+tdb_delete0(struct tdb *tdbp)
{
/* keep in sync with pfkeyv2_sa_flush() */
NET_ASSERT_LOCKED();
Index: netinet/ip_ipsp.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipsp.h,v
retrieving revision 1.225
diff -u -p -r1.225 ip_ipsp.h
--- netinet/ip_ipsp.h 1 Dec 2021 12:51:09 -0000 1.225
+++ netinet/ip_ipsp.h 1 Dec 2021 18:27:03 -0000
@@ -309,6 +309,18 @@ struct ipsec_policy {
#define IPSP_IDENTITY_USERFQDN 3
#define IPSP_IDENTITY_ASN1_DN 4
+/* #define TDB_TRACE_MAX 100 */
+#ifdef TDB_TRACE_MAX
+struct tdb_trace {
+ const char * tt_func;
+ int tt_line;
+ unsigned int tt_stamp;
+ unsigned int tt_refs;
+ int tt_op;
+ int tt_cpu;
+};
+#endif /* TDB_TRACE_MAX */
+
struct tdb { /* tunnel descriptor block */
/*
* Each TDB is on three hash tables: one keyed on dst/spi/sproto,
@@ -431,6 +443,10 @@ struct tdb { /* tunnel
descriptor blo
TAILQ_HEAD(tdb_policy_head, ipsec_policy) tdb_policy_head;
TAILQ_ENTRY(tdb) tdb_sync_entry;
+#ifdef TDB_TRACE_MAX
+ unsigned int tdb_trace_idx;
+ struct tdb_trace tdb_trace[TDB_TRACE_MAX];
+#endif /* TDB_TRACE_MAX */
};
#define tdb_ipackets tdb_data.tdd_ipackets
#define tdb_opackets tdb_data.tdd_opackets
@@ -551,26 +567,108 @@ int spd_table_walk(unsigned int,
/* TDB management routines */
uint32_t reserve_spi(u_int, u_int32_t, u_int32_t, union sockaddr_union *,
- union sockaddr_union *, u_int8_t, int *);
-struct tdb *gettdb_dir(u_int, u_int32_t, union sockaddr_union *, u_int8_t,
int);
-#define gettdb(a,b,c,d) gettdb_dir((a),(b),(c),(d),0)
-#define gettdb_rev(a,b,c,d) gettdb_dir((a),(b),(c),(d),1)
-struct tdb *gettdbbydst(u_int, union sockaddr_union *, u_int8_t,
- struct ipsec_ids *,
- struct sockaddr_encap *, struct sockaddr_encap *);
-struct tdb *gettdbbysrc(u_int, union sockaddr_union *, u_int8_t,
- struct ipsec_ids *,
- struct sockaddr_encap *, struct sockaddr_encap *);
+ union sockaddr_union *, u_int8_t, int *);
+struct tdb *gettdb_dir(u_int, u_int32_t, union sockaddr_union *, u_int8_t,
+ int);
struct tdb *gettdbbysrcdst_dir(u_int, u_int32_t, union sockaddr_union *,
- union sockaddr_union *, u_int8_t, int);
-#define gettdbbysrcdst(a,b,c,d,e) gettdbbysrcdst_dir((a),(b),(c),(d),(e),0)
-#define gettdbbysrcdst_rev(a,b,c,d,e) gettdbbysrcdst_dir((a),(b),(c),(d),(e),1)
+ union sockaddr_union *, u_int8_t, int);
+struct tdb *gettdbbydst0(u_int, union sockaddr_union *, u_int8_t,
+ struct ipsec_ids *, struct sockaddr_encap *, struct sockaddr_encap *);
+struct tdb *gettdbbysrc0(u_int, union sockaddr_union *, u_int8_t,
+ struct ipsec_ids *, struct sockaddr_encap *, struct sockaddr_encap *);
void puttdb(struct tdb *);
void puttdb_locked(struct tdb *);
-void tdb_delete(struct tdb *);
+void tdb_delete0(struct tdb *);
struct tdb *tdb_alloc(u_int);
-struct tdb *tdb_ref(struct tdb *);
-void tdb_unref(struct tdb *);
+struct tdb *tdb_ref0(struct tdb *);
+void tdb_unref0(struct tdb *);
+
+#ifdef TDB_TRACE_MAX
+extern int tdb_trace_level;
+extern unsigned int tdb_trace_stamp;
+
+#define TDB_TRACE_RECORD(y, f, l, op) do { \
+ struct tdb *x = y; \
+ if (x) { \
+ struct tdb_trace *tt; \
+ unsigned int idx; \
+ \
+ idx = atomic_inc_int_nv(&x->tdb_trace_idx); \
+ tt = &x->tdb_trace[(idx - 1) % TDB_TRACE_MAX]; \
+ tt->tt_stamp = atomic_inc_int_nv(&tdb_trace_stamp); \
+ tt->tt_func = f; \
+ tt->tt_line = l; \
+ tt->tt_op = op; \
+ tt->tt_cpu = cpu_number(); \
+ tt->tt_refs = x->tdb_refcnt.refs; /* RACE */ \
+ if (tdb_trace_level > 0) \
+ printf("tdb %p: %u: refs %u %+d cpu%d %s:%d\n", \
+ x, tt->tt_stamp, tt->tt_refs, tt->tt_op, \
+ tt->tt_cpu, tt->tt_func, tt->tt_line); \
+ } \
+ } while(0)
+#define tdb_ref(x) ({ \
+ struct tdb *y; \
+ TDB_TRACE_RECORD(x, __func__, __LINE__, 1); \
+ y = tdb_ref0(x); \
+ y; \
+ })
+#define tdb_unref(x) do { \
+ TDB_TRACE_RECORD(x, __func__, __LINE__, -1); \
+ tdb_unref0(x); \
+ } while(0)
+#define tdb_delete(x) do { \
+ TDB_TRACE_RECORD(x, __func__, __LINE__, 0); \
+ tdb_delete0(x); \
+ } while(0)
+#define gettdb(a,b,c,d) ({ \
+ struct tdb *y; \
+ y = gettdb_dir((a),(b),(c),(d),0); \
+ TDB_TRACE_RECORD(y, __func__, __LINE__, 0); \
+ y; \
+ })
+#define gettdb_rev(a,b,c,d) ({ \
+ struct tdb *y; \
+ y = gettdb_dir((a),(b),(c),(d),1); \
+ TDB_TRACE_RECORD(y, __func__, __LINE__, 0); \
+ y; \
+ })
+#define gettdbbysrcdst(a,b,c,d,e) ({ \
+ struct tdb *y; \
+ y = gettdbbysrcdst_dir((a),(b),(c),(d),(e),0); \
+ TDB_TRACE_RECORD(y, __func__, __LINE__, 0); \
+ y; \
+ })
+#define gettdbbysrcdst_rev(a,b,c,d,e) ({ \
+ struct tdb *y; \
+ y = gettdbbysrcdst_dir((a),(b),(c),(d),(e),1); \
+ TDB_TRACE_RECORD(y, __func__, __LINE__, 0); \
+ y; \
+ })
+#define gettdbbydst(a,b,c,d,e,f) ({ \
+ struct tdb *y; \
+ y = gettdbbydst0((a),(b),(c),(d),(e),(f)); \
+ TDB_TRACE_RECORD(y, __func__, __LINE__, 0); \
+ y; \
+ })
+#define gettdbbysrc(a,b,c,d,e,f) ({ \
+ struct tdb *y; \
+ y = gettdbbysrc0((a),(b),(c),(d),(e),(f)); \
+ TDB_TRACE_RECORD(y, __func__, __LINE__, 0); \
+ y; \
+ })
+#else /* TDB_TRACE_MAX */
+#define tdb_ref(x) tdb_ref0(x)
+#define tdb_unref(x) tdb_unref0(x)
+#define tdb_delete(x) tdb_delete0(x)
+#define gettdb(a,b,c,d) gettdb_dir((a),(b),(c),(d),0)
+#define gettdb_rev(a,b,c,d) gettdb_dir((a),(b),(c),(d),1)
+#define gettdbbysrcdst(a,b,c,d,e) gettdbbysrcdst_dir((a),(b),(c),(d),(e),0)
+#define gettdbbysrcdst_rev(a,b,c,d,e) gettdbbysrcdst_dir((a),(b),(c),(d),(e),1)
+#define gettdbbydst(a,b,c,d,e,f) gettdbbydst0((a),(b),(c),(d),(e),(f))
+#define gettdbbysrc(a,b,c,d,e,f) gettdbbysrc0((a),(b),(c),(d),(e),(f))
+#endif /* TDB_TRACE_MAX */
+
void tdb_free(struct tdb *);
int tdb_init(struct tdb *, u_int16_t, struct ipsecinit *);
int tdb_unlink(struct tdb *);
