ok mvs@
> On 26 Nov 2021, at 01:37, Tobias Heider <[email protected]> wrote:
>
> 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 *);
