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);
Technically the diff is correct, but I would to like to handle this
potential race a bit more obviously.
bluhm
> 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 16:10:51 -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);
> 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 16:10:51 -0000
> @@ -835,15 +835,18 @@ puttdb(struct tdb *tdbp)
> 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 +854,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 +913,8 @@ tdb_unlink_locked(struct tdb *tdbp)
> ipsecstat_inc(ipsec_prevtunnels);
> }
> #endif /* IPSEC */
> +
> + return (1);
> }
>
> void
> @@ -968,11 +976,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 16:10:51 -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" \
> @@ -571,8 +570,8 @@ 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 *);
