On Wed, Dec 15, 2021 at 07:21:46PM +0300, Vitaliy Makkoveev wrote:
> The previous version of this diff exposed UAF issue we had after
> tdb_delete(). bluhm@ fixed this and proposes to put per-cpu counters
> diff again to tree. This is the updated diff to be against the resent
> sources.
OK bluhm@ after tdb walker sleep has been fixed
> Index: sys/net/pfkeyv2_convert.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pfkeyv2_convert.c,v
> retrieving revision 1.77
> diff -u -p -r1.77 pfkeyv2_convert.c
> --- sys/net/pfkeyv2_convert.c 11 Dec 2021 16:33:46 -0000 1.77
> +++ sys/net/pfkeyv2_convert.c 15 Dec 2021 15:45:26 -0000
> @@ -963,18 +963,21 @@ export_satype(void **p, struct tdb *tdb)
> void
> export_counter(void **p, struct tdb *tdb)
> {
> + uint64_t counters[tdb_ncounters];
> struct sadb_x_counter *scnt = (struct sadb_x_counter *)*p;
>
> + counters_read(tdb->tdb_counters, counters, tdb_ncounters);
> +
> scnt->sadb_x_counter_len = sizeof(struct sadb_x_counter) /
> sizeof(uint64_t);
> scnt->sadb_x_counter_pad = 0;
> - scnt->sadb_x_counter_ipackets = tdb->tdb_ipackets;
> - scnt->sadb_x_counter_opackets = tdb->tdb_opackets;
> - scnt->sadb_x_counter_ibytes = tdb->tdb_ibytes;
> - scnt->sadb_x_counter_obytes = tdb->tdb_obytes;
> - scnt->sadb_x_counter_idrops = tdb->tdb_idrops;
> - scnt->sadb_x_counter_odrops = tdb->tdb_odrops;
> - scnt->sadb_x_counter_idecompbytes = tdb->tdb_idecompbytes;
> - scnt->sadb_x_counter_ouncompbytes = tdb->tdb_ouncompbytes;
> + scnt->sadb_x_counter_ipackets = counters[tdb_ipackets];
> + scnt->sadb_x_counter_opackets = counters[tdb_opackets];
> + scnt->sadb_x_counter_ibytes = counters[tdb_ibytes];
> + scnt->sadb_x_counter_obytes = counters[tdb_obytes];
> + scnt->sadb_x_counter_idrops = counters[tdb_idrops];
> + scnt->sadb_x_counter_odrops = counters[tdb_odrops];
> + scnt->sadb_x_counter_idecompbytes = counters[tdb_idecompbytes];
> + scnt->sadb_x_counter_ouncompbytes = counters[tdb_ouncompbytes];
> *p += sizeof(struct sadb_x_counter);
> }
> Index: sys/netinet/ip_ah.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_ah.c,v
> retrieving revision 1.169
> diff -u -p -r1.169 ip_ah.c
> --- sys/netinet/ip_ah.c 11 Dec 2021 16:33:46 -0000 1.169
> +++ sys/netinet/ip_ah.c 15 Dec 2021 15:45:26 -0000
> @@ -608,7 +608,7 @@ ah_input(struct mbuf **mp, struct tdb *t
> /* Update the counters. */
> ibytes = (m->m_pkthdr.len - skip - hl * sizeof(u_int32_t));
> tdb->tdb_cur_bytes += ibytes;
> - tdb->tdb_ibytes += ibytes;
> + tdbstat_add(tdb, tdb_ibytes, ibytes);
> ahstat_add(ahs_ibytes, ibytes);
>
> /* Hard expiration. */
> Index: sys/netinet/ip_esp.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_esp.c,v
> retrieving revision 1.189
> diff -u -p -r1.189 ip_esp.c
> --- sys/netinet/ip_esp.c 11 Dec 2021 16:33:47 -0000 1.189
> +++ sys/netinet/ip_esp.c 15 Dec 2021 15:45:26 -0000
> @@ -420,7 +420,7 @@ esp_input(struct mbuf **mp, struct tdb *
>
> /* Update the counters */
> tdb->tdb_cur_bytes += plen;
> - tdb->tdb_ibytes += plen;
> + tdbstat_add(tdb, tdb_ibytes, plen);
> espstat_add(esps_ibytes, plen);
>
> /* Hard expiration */
> Index: sys/netinet/ip_ipcomp.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_ipcomp.c,v
> retrieving revision 1.89
> diff -u -p -r1.89 ip_ipcomp.c
> --- sys/netinet/ip_ipcomp.c 11 Dec 2021 16:33:47 -0000 1.89
> +++ sys/netinet/ip_ipcomp.c 15 Dec 2021 15:45:26 -0000
> @@ -193,7 +193,7 @@ ipcomp_input(struct mbuf **mp, struct td
> /* update the counters */
> ibytes = m->m_pkthdr.len - (skip + hlen);
> tdb->tdb_cur_bytes += ibytes;
> - tdb->tdb_ibytes += ibytes;
> + tdbstat_add(tdb, tdb_ibytes, ibytes);
> ipcompstat_add(ipcomps_ibytes, ibytes);
>
> /* Hard expiration */
> Index: sys/netinet/ip_ipsp.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_ipsp.c,v
> retrieving revision 1.265
> diff -u -p -r1.265 ip_ipsp.c
> --- sys/netinet/ip_ipsp.c 14 Dec 2021 17:50:37 -0000 1.265
> +++ sys/netinet/ip_ipsp.c 15 Dec 2021 15:45:26 -0000
> @@ -1062,6 +1062,9 @@ tdb_alloc(u_int rdomain)
> tdbp->tdb_rdomain = rdomain;
> tdbp->tdb_rdomain_post = rdomain;
>
> + /* Initialize counters. */
> + tdbp->tdb_counters = counters_alloc(tdb_ncounters);
> +
> /* Initialize timeouts. */
> timeout_set_proc(&tdbp->tdb_timer_tmo, tdb_timeout, tdbp);
> timeout_set_proc(&tdbp->tdb_first_tmo, tdb_firstuse, tdbp);
> @@ -1099,6 +1102,8 @@ tdb_free(struct tdb *tdbp)
> tdbp->tdb_tag = 0;
> }
> #endif
> +
> + counters_free(tdbp->tdb_counters, tdb_ncounters);
>
> KASSERT(tdbp->tdb_onext == NULL);
> KASSERT(tdbp->tdb_inext == NULL);
> Index: sys/netinet/ip_ipsp.h
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_ipsp.h,v
> retrieving revision 1.231
> diff -u -p -r1.231 ip_ipsp.h
> --- sys/netinet/ip_ipsp.h 14 Dec 2021 17:50:37 -0000 1.231
> +++ sys/netinet/ip_ipsp.h 15 Dec 2021 15:45:26 -0000
> @@ -136,17 +136,6 @@ struct ipsecstat {
> uint64_t ipsec_exctdb; /* TDBs with hardlimit excess */
> };
>
> -struct tdb_data {
> - uint64_t tdd_ipackets; /* Input IPsec packets */
> - uint64_t tdd_opackets; /* Output IPsec packets */
> - uint64_t tdd_ibytes; /* Input bytes */
> - uint64_t tdd_obytes; /* Output bytes */
> - uint64_t tdd_idrops; /* Dropped on input */
> - uint64_t tdd_odrops; /* Dropped on output */
> - uint64_t tdd_idecompbytes; /* Input bytes, decompressed */
> - uint64_t tdd_ouncompbytes; /* Output bytes, uncompressed */
> -};
> -
> #ifdef _KERNEL
>
> #include <sys/timeout.h>
> @@ -400,7 +389,8 @@ struct tdb { /* tunnel
> descriptor blo
> u_int64_t tdb_last_used; /* When was this SA last used */
> u_int64_t tdb_last_marked;/* Last SKIPCRYPTO status change */
>
> - struct tdb_data tdb_data; /* stats about this TDB */
> + struct cpumem *tdb_counters; /* stats about this TDB */
> +
> u_int64_t tdb_cryptoid; /* Crypto session ID */
>
> u_int32_t tdb_spi; /* [I] SPI */
> @@ -446,15 +436,37 @@ struct tdb { /* tunnel
> descriptor blo
> TAILQ_HEAD(tdb_policy_head, ipsec_policy) tdb_policy_head; /* [p] */
> TAILQ_ENTRY(tdb) tdb_sync_entry;
> };
> -#define tdb_ipackets tdb_data.tdd_ipackets
> -#define tdb_opackets tdb_data.tdd_opackets
> -#define tdb_ibytes tdb_data.tdd_ibytes
> -#define tdb_obytes tdb_data.tdd_obytes
> -#define tdb_idrops tdb_data.tdd_idrops
> -#define tdb_odrops tdb_data.tdd_odrops
> -#define tdb_idecompbytes tdb_data.tdd_idecompbytes
> -#define tdb_ouncompbytes tdb_data.tdd_ouncompbytes
>
> +enum tdb_counters {
> + tdb_ipackets, /* Input IPsec packets */
> + tdb_opackets, /* Output IPsec packets */
> + tdb_ibytes, /* Input bytes */
> + tdb_obytes, /* Output bytes */
> + tdb_idrops, /* Dropped on input */
> + tdb_odrops, /* Dropped on output */
> + tdb_idecompbytes, /* Input bytes, decompressed */
> + tdb_ouncompbytes, /* Output bytes, uncompressed */
> + tdb_ncounters
> +};
> +
> +static inline void
> +tdbstat_inc(struct tdb *tdb, enum tdb_counters c)
> +{
> + counters_inc(tdb->tdb_counters, c);
> +}
> +
> +static inline void
> +tdbstat_add(struct tdb *tdb, enum tdb_counters c, uint64_t v)
> +{
> + counters_add(tdb->tdb_counters, c, v);
> +}
> +
> +static inline void
> +tdbstat_pkt(struct tdb *tdb, enum tdb_counters pc, enum tdb_counters bc,
> + uint64_t bytes)
> +{
> + counters_pkt(tdb->tdb_counters, pc, bc, bytes);
> +}
>
> struct tdb_ident {
> u_int32_t spi;
> Index: sys/netinet/ip_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> retrieving revision 1.377
> diff -u -p -r1.377 ip_output.c
> --- sys/netinet/ip_output.c 3 Dec 2021 17:18:34 -0000 1.377
> +++ sys/netinet/ip_output.c 15 Dec 2021 15:45:26 -0000
> @@ -662,7 +662,7 @@ ip_output_ipsec_send(struct tdb *tdb, st
> error = ipsp_process_packet(m, tdb, AF_INET, 0);
> if (error) {
> ipsecstat_inc(ipsec_odrops);
> - tdb->tdb_odrops++;
> + tdbstat_inc(tdb, tdb_odrops);
> }
> if (ip_mtudisc && error == EMSGSIZE)
> ip_output_ipsec_pmtu_update(tdb, ro, dst, rtableid, 0);
> Index: sys/netinet/ipsec_input.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ipsec_input.c,v
> retrieving revision 1.197
> diff -u -p -r1.197 ipsec_input.c
> --- sys/netinet/ipsec_input.c 8 Dec 2021 14:24:18 -0000 1.197
> +++ sys/netinet/ipsec_input.c 15 Dec 2021 15:45:26 -0000
> @@ -340,8 +340,7 @@ ipsec_common_input(struct mbuf **mp, int
> }
> }
>
> - tdbp->tdb_ipackets++;
> - tdbp->tdb_ibytes += m->m_pkthdr.len;
> + tdbstat_pkt(tdbp, tdb_ipackets, tdb_ibytes, m->m_pkthdr.len);
>
> /*
> * Call appropriate transform and return -- callback takes care of
> @@ -350,7 +349,7 @@ ipsec_common_input(struct mbuf **mp, int
> prot = (*(tdbp->tdb_xform->xf_input))(mp, tdbp, skip, protoff);
> if (prot == IPPROTO_DONE) {
> ipsecstat_inc(ipsec_idrops);
> - tdbp->tdb_idrops++;
> + tdbstat_inc(tdbp, tdb_idrops);
> }
> tdb_unref(tdbp);
> return prot;
> @@ -359,7 +358,7 @@ ipsec_common_input(struct mbuf **mp, int
> m_freemp(mp);
> ipsecstat_inc(ipsec_idrops);
> if (tdbp != NULL)
> - tdbp->tdb_idrops++;
> + tdbstat_inc(tdbp, tdb_idrops);
> tdb_unref(tdbp);
> return IPPROTO_DONE;
> }
> @@ -537,7 +536,7 @@ ipsec_common_input_cb(struct mbuf **mp,
> m->m_flags |= M_TUNNEL;
>
> ipsecstat_add(ipsec_idecompbytes, m->m_pkthdr.len);
> - tdbp->tdb_idecompbytes += m->m_pkthdr.len;
> + tdbstat_add(tdbp, tdb_idecompbytes, m->m_pkthdr.len);
>
> #if NBPFILTER > 0
> encif = enc_getif(tdbp->tdb_rdomain_post, tdbp->tdb_tap);
> Index: sys/netinet/ipsec_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ipsec_output.c,v
> retrieving revision 1.94
> diff -u -p -r1.94 ipsec_output.c
> --- sys/netinet/ipsec_output.c 11 Dec 2021 16:33:47 -0000 1.94
> +++ sys/netinet/ipsec_output.c 15 Dec 2021 15:45:26 -0000
> @@ -367,7 +367,7 @@ ipsp_process_packet(struct mbuf *m, stru
> }
>
> ipsecstat_add(ipsec_ouncompbytes, m->m_pkthdr.len);
> - tdb->tdb_ouncompbytes += m->m_pkthdr.len;
> + tdbstat_add(tdb, tdb_ouncompbytes, m->m_pkthdr.len);
>
> /* Non expansion policy for IPCOMP */
> if (tdb->tdb_sproto == IPPROTO_IPCOMP) {
> @@ -507,8 +507,7 @@ ipsp_process_done(struct mbuf *m, struct
> m_tag_prepend(m, mtag);
>
> ipsecstat_pkt(ipsec_opackets, ipsec_obytes, m->m_pkthdr.len);
> - tdb->tdb_opackets++;
> - tdb->tdb_obytes += m->m_pkthdr.len;
> + tdbstat_pkt(tdb, tdb_opackets, tdb_obytes, m->m_pkthdr.len);
>
> /* If there's another (bundled) TDB to apply, do so. */
> tdbo = tdb_ref(tdb->tdb_onext);
> Index: sys/netinet6/ip6_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_output.c,v
> retrieving revision 1.263
> diff -u -p -r1.263 ip6_output.c
> --- sys/netinet6/ip6_output.c 3 Dec 2021 17:18:34 -0000 1.263
> +++ sys/netinet6/ip6_output.c 15 Dec 2021 15:45:26 -0000
> @@ -2875,7 +2875,7 @@ ip6_output_ipsec_send(struct tdb *tdb, s
> rtableid, transportmode);
> if (error) {
> ipsecstat_inc(ipsec_odrops);
> - tdb->tdb_odrops++;
> + tdbstat_inc(tdb, tdb_odrops);
> m_freem(m);
> return error;
> }
> @@ -2897,7 +2897,7 @@ ip6_output_ipsec_send(struct tdb *tdb, s
> error = ipsp_process_packet(m, tdb, AF_INET6, tunalready);
> if (error) {
> ipsecstat_inc(ipsec_odrops);
> - tdb->tdb_odrops++;
> + tdbstat_inc(tdb, tdb_odrops);
> }
> if (ip_mtudisc && error == EMSGSIZE)
> ip6_output_ipsec_pmtu_update(tdb, ro, &dst, ifidx, rtableid, 0);
