On Fri, Nov 26, 2021 at 01:17:22AM +0300, Vitaliy Makkoveev wrote:
> On Thu, Nov 25, 2021 at 10:59:25PM +0100, Alexander Bluhm wrote:
> > On Thu, Nov 25, 2021 at 05:13:16PM +0100, Tobias Heider wrote:
> > > Now with the missing parts from pfkeyv2.c as noticed by Hrvoje.
> >
> > We have this code in pfkeyv2_send()
> >
> > if (headers[SADB_EXT_ADDRESS_SRC] ||
> > headers[SADB_EXT_ADDRESS_PROXY]) {
> > tdb_unlink(sa2);
> > import_address((struct sockaddr
> > *)&sa2->tdb_src,
> > headers[SADB_EXT_ADDRESS_SRC]);
> > import_address((struct sockaddr
> > *)&sa2->tdb_dst,
> > headers[SADB_EXT_ADDRESS_PROXY]);
> > puttdb(sa2);
> > }
> > }
> > NET_UNLOCK();
> >
> > Without the deleted flag, the pointers removed by tdb_unlink() and
> > set by puttdb() guarantee that tdb_delete() is not called twice.
> > In this piece of code they are missing for a short time.
> >
> > Net lock takes care of this. There should be a comment that describes
> > this.
> >
> > /*
> > * NET_LOCK prevents tdb_delete() between
> > * tdb_unlink() and puttdb().
> > */
> > tdb_unlink(sa2);
> > ...
> > puttdb(sa2);
> >
> > Or we don't want to rely on net lock. Then we need a common mutex
> > to pretect this.
> >
> > mtx_enter(&tdb_sadb_mtx);
> > tdb_unlink_locked(tdbp);
> > ...
> > puttdb_locked(sa2);
> > mtx_leave(&tdb_sadb_mtx);
> >
>
> Without regarding on this diff this could be right direction because
> `tdb_sadb_mtx' mutex(9) protects TDB hash consistency.
>
I agree that the mutex is the better solution. Updated diff below.
Index: net/pfkeyv2.c
===================================================================
RCS file: /cvs/src/sys/net/pfkeyv2.c,v
retrieving revision 1.222
diff -u -p -r1.222 pfkeyv2.c
--- net/pfkeyv2.c 25 Nov 2021 13:46:02 -0000 1.222
+++ net/pfkeyv2.c 25 Nov 2021 22:36:31 -0000
@@ -1046,11 +1046,8 @@ pfkeyv2_sa_flush(struct tdb *tdb, void *
/* keep in sync with tdb_delete() */
NET_ASSERT_LOCKED();
- if (tdb->tdb_flags & TDBF_DELETED)
+ if (tdb_unlink_locked(tdb) == 0)
return (0);
- tdb->tdb_flags |= TDBF_DELETED;
-
- tdb_unlink_locked(tdb);
tdb_unbundle(tdb);
tdb_deltimeouts(tdb);
tdb_unref(tdb);
@@ -1438,12 +1435,14 @@ pfkeyv2_send(struct socket *so, void *me
#endif
if (headers[SADB_EXT_ADDRESS_SRC] ||
headers[SADB_EXT_ADDRESS_PROXY]) {
- tdb_unlink(sa2);
+ mtx_enter(&tdb_sadb_mtx);
+ tdb_unlink_locked(sa2);
import_address((struct sockaddr *)&sa2->tdb_src,
headers[SADB_EXT_ADDRESS_SRC]);
import_address((struct sockaddr *)&sa2->tdb_dst,
headers[SADB_EXT_ADDRESS_PROXY]);
- puttdb(sa2);
+ puttdb_locked(sa2);
+ mtx_leave(&tdb_sadb_mtx);
}
}
NET_UNLOCK();
Index: netinet/ip_ipsp.c
===================================================================
RCS file: /cvs/src/sys/netinet/ip_ipsp.c,v
retrieving revision 1.254
diff -u -p -r1.254 ip_ipsp.c
--- netinet/ip_ipsp.c 25 Nov 2021 13:46:02 -0000 1.254
+++ netinet/ip_ipsp.c 25 Nov 2021 22:36:31 -0000
@@ -794,9 +794,16 @@ tdb_rehash(void)
void
puttdb(struct tdb *tdbp)
{
+ mtx_enter(&tdb_sadb_mtx);
+ puttdb_locked(tdbp);
+ mtx_leave(&tdb_sadb_mtx);
+}
+
+void
+puttdb_locked(struct tdb *tdbp)
+{
u_int32_t hashval;
- mtx_enter(&tdb_sadb_mtx);
hashval = tdb_hash(tdbp->tdb_spi, &tdbp->tdb_dst, tdbp->tdb_sproto);
/*
@@ -832,18 +839,20 @@ puttdb(struct tdb *tdbp)
#endif /* IPSEC */
ipsec_last_added = getuptime();
- mtx_leave(&tdb_sadb_mtx);
}
-void
+int
tdb_unlink(struct tdb *tdbp)
{
+ int r;
+
mtx_enter(&tdb_sadb_mtx);
- tdb_unlink_locked(tdbp);
+ r = tdb_unlink_locked(tdbp);
mtx_leave(&tdb_sadb_mtx);
+ return (r);
}
-void
+int
tdb_unlink_locked(struct tdb *tdbp)
{
struct tdb *tdbpp;
@@ -851,6 +860,9 @@ tdb_unlink_locked(struct tdb *tdbp)
MUTEX_ASSERT_LOCKED(&tdb_sadb_mtx);
+ if (tdbp->tdb_dnext == NULL && tdbp->tdb_snext == NULL)
+ return (0);
+
hashval = tdb_hash(tdbp->tdb_spi, &tdbp->tdb_dst, tdbp->tdb_sproto);
if (tdbh[hashval] == tdbp) {
@@ -907,6 +919,8 @@ tdb_unlink_locked(struct tdb *tdbp)
ipsecstat_inc(ipsec_prevtunnels);
}
#endif /* IPSEC */
+
+ return (1);
}
void
@@ -968,11 +982,8 @@ tdb_delete(struct tdb *tdbp)
/* keep in sync with pfkeyv2_sa_flush() */
NET_ASSERT_LOCKED();
- if (tdbp->tdb_flags & TDBF_DELETED)
+ if (tdb_unlink(tdbp) == 0)
return;
- tdbp->tdb_flags |= TDBF_DELETED;
-
- tdb_unlink(tdbp);
/* release tdb_onext/tdb_inext references */
tdb_unbundle(tdbp);
/* delete timeouts and release references */
Index: netinet/ip_ipsp.h
===================================================================
RCS file: /cvs/src/sys/netinet/ip_ipsp.h,v
retrieving revision 1.222
diff -u -p -r1.222 ip_ipsp.h
--- netinet/ip_ipsp.h 25 Nov 2021 13:46:02 -0000 1.222
+++ netinet/ip_ipsp.h 25 Nov 2021 22:36:31 -0000
@@ -337,7 +337,6 @@ struct tdb { /* tunnel
descriptor blo
#define TDBF_ALLOCATIONS 0x00008 /* Check the flows counters */
#define TDBF_INVALID 0x00010 /* This SPI is not valid
yet/anymore */
#define TDBF_FIRSTUSE 0x00020 /* Expire after first use */
-#define TDBF_DELETED 0x00040 /* This TDB has already been
deleted */
#define TDBF_SOFT_TIMER 0x00080 /* Soft expiration */
#define TDBF_SOFT_BYTES 0x00100 /* Soft expiration */
#define TDBF_SOFT_ALLOCATIONS 0x00200 /* Soft expiration */
@@ -352,7 +351,7 @@ struct tdb { /* tunnel
descriptor blo
#define TDBF_BITS ("\20" \
"\1UNIQUE\2TIMER\3BYTES\4ALLOCATIONS" \
- "\5INVALID\6FIRSTUSE\7DELETED\10SOFT_TIMER" \
+ "\5INVALID\6FIRSTUSE\10SOFT_TIMER" \
"\11SOFT_BYTES\12SOFT_ALLOCATIONS\13SOFT_FIRSTUSE\14PFS" \
"\15TUNNELING" \
"\21USEDTUNNEL\22UDPENCAP\23PFSYNC\24PFSYNC_RPL" \
@@ -537,6 +536,8 @@ extern char ipsec_def_comp[];
extern TAILQ_HEAD(ipsec_policy_head, ipsec_policy) ipsec_policy_head;
+extern struct mutex tdb_sadb_mtx;
+
struct cryptop;
/* Misc. */
@@ -565,14 +566,15 @@ struct tdb *gettdbbysrcdst_dir(u_int, u_
#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)
void puttdb(struct tdb *);
+void puttdb_locked(struct tdb *);
void tdb_delete(struct tdb *);
struct tdb *tdb_alloc(u_int);
struct tdb *tdb_ref(struct tdb *);
void tdb_unref(struct tdb *);
void tdb_free(struct tdb *);
int tdb_init(struct tdb *, u_int16_t, struct ipsecinit *);
-void tdb_unlink(struct tdb *);
-void tdb_unlink_locked(struct tdb *);
+int tdb_unlink(struct tdb *);
+int tdb_unlink_locked(struct tdb *);
void tdb_unbundle(struct tdb *);
void tdb_deltimeouts(struct tdb *);
int tdb_walk(u_int, int (*)(struct tdb *, void *, int), void *);
