From 0ae952208f54c8830b58d4facb9561e2e34af873 Mon Sep 17 00:00:00 2001 From: bluhm Date: Mon, 26 Jul 2021 21:27:56 +0000 Subject: [PATCH] Do not queue crypto operations for IPsec. The packet entries in task queues were unlimited and could overflow during havy traffic. Even if we still use hardware drivers that sleep, softnet task instead of soft interrupt can handle this now. Without queues net lock is inherited and kernel lock is only needed once per packet. This results in less lock contention and faster IPsec. Also protect tdb drop counters with net lock and avoid a leak in crypto dispatch error handling. intense testing Hrvoje Popovski; OK mpi@ --- sys/crypto/crypto.c | 32 ++++++++++++++++++++++++-------- sys/crypto/cryptodev.h | 3 ++- sys/netinet/ip_ah.c | 10 +++------- sys/netinet/ip_esp.c | 10 +++------- sys/netinet/ip_ipcomp.c | 10 +++------- sys/netinet/ipsec_input.c | 21 +++++++-------------- sys/netinet/ipsec_output.c | 22 +++++++++------------- 7 files changed, 51 insertions(+), 57 deletions(-) diff --git a/sys/crypto/crypto.c b/sys/crypto/crypto.c index 4f3f1f85509..bff6dd4a97c 100644 --- a/sys/crypto/crypto.c +++ b/sys/crypto/crypto.c @@ -1,4 +1,4 @@ -/* $OpenBSD: crypto.c,v 1.84 2021/07/21 11:11:41 bluhm Exp $ */ +/* $OpenBSD: crypto.c,v 1.85 2021/07/26 21:27:56 bluhm Exp $ */ /* * The author of this code is Angelos D. Keromytis (angelos@cis.upenn.edu) * @@ -387,23 +387,32 @@ crypto_unregister(u_int32_t driverid, int alg) int crypto_dispatch(struct cryptop *crp) { - struct taskq *tq = crypto_taskq; - int error = 0, s; + int error = 0, lock = 1, s; u_int32_t hid; s = splvm(); hid = (crp->crp_sid >> 32) & 0xffffffff; if (hid < crypto_drivers_num) { if (crypto_drivers[hid].cc_flags & CRYPTOCAP_F_MPSAFE) - tq = crypto_taskq_mpsafe; + lock = 0; } splx(s); - if ((crp->crp_flags & CRYPTO_F_NOQUEUE) == 0) { + /* XXXSMP crypto_invoke() is not MP safe */ + lock = 1; + + if (crp->crp_flags & CRYPTO_F_NOQUEUE) { + if (lock) + KERNEL_LOCK(); + error = crypto_invoke(crp); + if (lock) + KERNEL_UNLOCK(); + } else { + struct taskq *tq; + + tq = lock ? crypto_taskq : crypto_taskq_mpsafe; task_set(&crp->crp_task, (void (*))crypto_invoke, crp); task_add(tq, &crp->crp_task); - } else { - error = crypto_invoke(crp); } return error; @@ -424,6 +433,8 @@ crypto_invoke(struct cryptop *crp) if (crp == NULL || crp->crp_callback == NULL) return EINVAL; + KERNEL_ASSERT_LOCKED(); + s = splvm(); if (crp->crp_ndesc < 1 || crypto_drivers == NULL) { crp->crp_etype = EINVAL; @@ -535,11 +546,16 @@ void crypto_done(struct cryptop *crp) { crp->crp_flags |= CRYPTO_F_DONE; + if (crp->crp_flags & CRYPTO_F_NOQUEUE) { /* not from the crypto queue, wakeup the userland process */ crp->crp_callback(crp); } else { + struct taskq *tq; + + tq = (crp->crp_flags & CRYPTO_F_MPSAFE) ? + crypto_taskq_mpsafe : crypto_taskq; task_set(&crp->crp_task, (void (*))crp->crp_callback, crp); - task_add(crypto_taskq, &crp->crp_task); + task_add(tq, &crp->crp_task); } } diff --git a/sys/crypto/cryptodev.h b/sys/crypto/cryptodev.h index 42ef0c58e8c..10bb1a6f625 100644 --- a/sys/crypto/cryptodev.h +++ b/sys/crypto/cryptodev.h @@ -1,4 +1,4 @@ -/* $OpenBSD: cryptodev.h,v 1.73 2021/07/09 20:43:28 mvs Exp $ */ +/* $OpenBSD: cryptodev.h,v 1.74 2021/07/26 21:27:56 bluhm Exp $ */ /* * The author of this code is Angelos D. Keromytis (angelos@cis.upenn.edu) @@ -171,6 +171,7 @@ struct cryptop { #define CRYPTO_F_IMBUF 0x0001 /* Input/output are mbuf chains, otherwise contig */ #define CRYPTO_F_IOV 0x0002 /* Input/output are uio */ +#define CRYPTO_F_MPSAFE 0x0004 /* Do not use kernel lock for callback */ #define CRYPTO_F_NOQUEUE 0x0008 /* Don't use crypto queue/thread */ #define CRYPTO_F_DONE 0x0010 /* request completed */ diff --git a/sys/netinet/ip_ah.c b/sys/netinet/ip_ah.c index b9c4ffab16c..70a0c173a69 100644 --- a/sys/netinet/ip_ah.c +++ b/sys/netinet/ip_ah.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ip_ah.c,v 1.151 2021/07/18 14:38:20 bluhm Exp $ */ +/* $OpenBSD: ip_ah.c,v 1.152 2021/07/26 21:27:56 bluhm Exp $ */ /* * The authors of this code are John Ioannidis (ji@tla.org), * Angelos D. Keromytis (kermit@csd.uch.gr) and @@ -684,7 +684,7 @@ ah_input(struct mbuf *m, struct tdb *tdb, int skip, int protoff) /* Crypto operation descriptor. */ crp->crp_ilen = m->m_pkthdr.len; /* Total input length. */ - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE | CRYPTO_F_NOQUEUE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_input_cb; crp->crp_sid = tdb->tdb_cryptoid; @@ -699,9 +699,7 @@ ah_input(struct mbuf *m, struct tdb *tdb, int skip, int protoff) memcpy(&tc->tc_dst, &tdb->tdb_dst, sizeof(union sockaddr_union)); tc->tc_rpl = tdb->tdb_rpl; - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; drop: @@ -1134,7 +1132,7 @@ ah_output(struct mbuf *m, struct tdb *tdb, struct mbuf **mp, int skip, /* Crypto operation descriptor. */ crp->crp_ilen = m->m_pkthdr.len; /* Total input length. */ - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE | CRYPTO_F_NOQUEUE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_output_cb; crp->crp_sid = tdb->tdb_cryptoid; @@ -1148,9 +1146,7 @@ ah_output(struct mbuf *m, struct tdb *tdb, struct mbuf **mp, int skip, tc->tc_rdomain = tdb->tdb_rdomain; memcpy(&tc->tc_dst, &tdb->tdb_dst, sizeof(union sockaddr_union)); - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; drop: diff --git a/sys/netinet/ip_esp.c b/sys/netinet/ip_esp.c index 11a323f34ef..7865c9a41ee 100644 --- a/sys/netinet/ip_esp.c +++ b/sys/netinet/ip_esp.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ip_esp.c,v 1.169 2021/07/18 14:38:20 bluhm Exp $ */ +/* $OpenBSD: ip_esp.c,v 1.170 2021/07/26 21:27:57 bluhm Exp $ */ /* * The authors of this code are John Ioannidis (ji@tla.org), * Angelos D. Keromytis (kermit@csd.uch.gr) and @@ -498,7 +498,7 @@ esp_input(struct mbuf *m, struct tdb *tdb, int skip, int protoff) /* Crypto operation descriptor */ crp->crp_ilen = m->m_pkthdr.len; /* Total input length */ - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE | CRYPTO_F_NOQUEUE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_input_cb; crp->crp_sid = tdb->tdb_cryptoid; @@ -528,9 +528,7 @@ esp_input(struct mbuf *m, struct tdb *tdb, int skip, int protoff) crde->crd_len = m->m_pkthdr.len - (skip + hlen + alen); } - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; drop: @@ -983,7 +981,7 @@ esp_output(struct mbuf *m, struct tdb *tdb, struct mbuf **mp, int skip, /* Crypto operation descriptor. */ crp->crp_ilen = m->m_pkthdr.len; /* Total input length. */ - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE | CRYPTO_F_NOQUEUE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_output_cb; crp->crp_opaque = (caddr_t)tc; @@ -1015,9 +1013,7 @@ esp_output(struct mbuf *m, struct tdb *tdb, struct mbuf **mp, int skip, crda->crd_len = m->m_pkthdr.len - (skip + alen); } - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; drop: diff --git a/sys/netinet/ip_ipcomp.c b/sys/netinet/ip_ipcomp.c index 1b29d702593..d7f53d153f7 100644 --- a/sys/netinet/ip_ipcomp.c +++ b/sys/netinet/ip_ipcomp.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ip_ipcomp.c,v 1.71 2021/07/08 21:07:19 bluhm Exp $ */ +/* $OpenBSD: ip_ipcomp.c,v 1.72 2021/07/26 21:27:57 bluhm Exp $ */ /* * Copyright (c) 2001 Jean-Jacques Bernard-Gundol (jj@wabbitt.org) @@ -174,7 +174,7 @@ ipcomp_input(struct mbuf *m, struct tdb *tdb, int skip, int protoff) /* Crypto operation descriptor */ crp->crp_ilen = m->m_pkthdr.len - (skip + hlen); - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE | CRYPTO_F_NOQUEUE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_input_cb; crp->crp_sid = tdb->tdb_cryptoid; @@ -188,9 +188,7 @@ ipcomp_input(struct mbuf *m, struct tdb *tdb, int skip, int protoff) tc->tc_rdomain = tdb->tdb_rdomain; tc->tc_dst = tdb->tdb_dst; - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; } @@ -479,15 +477,13 @@ ipcomp_output(struct mbuf *m, struct tdb *tdb, struct mbuf **mp, int skip, /* Crypto operation descriptor */ crp->crp_ilen = m->m_pkthdr.len; /* Total input length */ - crp->crp_flags = CRYPTO_F_IMBUF; + crp->crp_flags = CRYPTO_F_IMBUF | CRYPTO_F_MPSAFE | CRYPTO_F_NOQUEUE; crp->crp_buf = (caddr_t)m; crp->crp_callback = ipsec_output_cb; crp->crp_opaque = (caddr_t)tc; crp->crp_sid = tdb->tdb_cryptoid; - KERNEL_LOCK(); error = crypto_dispatch(crp); - KERNEL_UNLOCK(); return error; drop: diff --git a/sys/netinet/ipsec_input.c b/sys/netinet/ipsec_input.c index ba4b92a572f..1271038d7b8 100644 --- a/sys/netinet/ipsec_input.c +++ b/sys/netinet/ipsec_input.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ipsec_input.c,v 1.176 2021/07/21 12:23:32 bluhm Exp $ */ +/* $OpenBSD: ipsec_input.c,v 1.177 2021/07/26 21:27:57 bluhm Exp $ */ /* * The authors of this code are John Ioannidis (ji@tla.org), * Angelos D. Keromytis (kermit@csd.uch.gr) and @@ -380,21 +380,19 @@ ipsec_input_cb(struct cryptop *crp) struct tdb *tdb = NULL; int clen, error; - KERNEL_ASSERT_LOCKED(); + NET_ASSERT_LOCKED(); if (m == NULL) { DPRINTF("bogus returned buffer from crypto"); ipsecstat_inc(ipsec_crypto); - goto droponly; + goto drop; } - - NET_LOCK(); tdb = gettdb(tc->tc_rdomain, tc->tc_spi, &tc->tc_dst, tc->tc_proto); if (tdb == NULL) { DPRINTF("TDB is expired while in crypto"); ipsecstat_inc(ipsec_notdb); - goto baddone; + goto drop; } /* Check for crypto errors */ @@ -403,18 +401,16 @@ ipsec_input_cb(struct cryptop *crp) /* Reset the session ID */ if (tdb->tdb_cryptoid != 0) tdb->tdb_cryptoid = crp->crp_sid; - NET_UNLOCK(); error = crypto_dispatch(crp); if (error) { DPRINTF("crypto dispatch error %d", error); - ipsecstat_inc(ipsec_idrops); - tdb->tdb_idrops++; + goto drop; } return; } DPRINTF("crypto error %d", crp->crp_etype); ipsecstat_inc(ipsec_noxform); - goto baddone; + goto drop; } /* Length of data after processing */ @@ -438,16 +434,13 @@ ipsec_input_cb(struct cryptop *crp) __func__, tdb->tdb_sproto); } - NET_UNLOCK(); if (error) { ipsecstat_inc(ipsec_idrops); tdb->tdb_idrops++; } return; - baddone: - NET_UNLOCK(); - droponly: + drop: ipsecstat_inc(ipsec_idrops); if (tdb != NULL) tdb->tdb_idrops++; diff --git a/sys/netinet/ipsec_output.c b/sys/netinet/ipsec_output.c index 9f39a532930..59997701e9d 100644 --- a/sys/netinet/ipsec_output.c +++ b/sys/netinet/ipsec_output.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ipsec_output.c,v 1.83 2021/07/21 11:11:41 bluhm Exp $ */ +/* $OpenBSD: ipsec_output.c,v 1.84 2021/07/26 21:27:57 bluhm Exp $ */ /* * The author of this code is Angelos D. Keromytis (angelos@cis.upenn.edu) * @@ -395,20 +395,19 @@ ipsec_output_cb(struct cryptop *crp) struct tdb *tdb = NULL; int error, ilen, olen; - KERNEL_ASSERT_LOCKED(); + NET_ASSERT_LOCKED(); if (m == NULL) { DPRINTF("bogus returned buffer from crypto"); ipsecstat_inc(ipsec_crypto); - goto droponly; + goto drop; } - NET_LOCK(); tdb = gettdb(tc->tc_rdomain, tc->tc_spi, &tc->tc_dst, tc->tc_proto); if (tdb == NULL) { DPRINTF("TDB is expired while in crypto"); ipsecstat_inc(ipsec_notdb); - goto baddone; + goto drop; } /* Check for crypto errors. */ @@ -417,18 +416,16 @@ ipsec_output_cb(struct cryptop *crp) /* Reset the session ID */ if (tdb->tdb_cryptoid != 0) tdb->tdb_cryptoid = crp->crp_sid; - NET_UNLOCK(); error = crypto_dispatch(crp); if (error) { DPRINTF("crypto dispatch error %d", error); - ipsecstat_inc(ipsec_odrops); - tdb->tdb_odrops++; + goto drop; } return; } DPRINTF("crypto error %d", crp->crp_etype); ipsecstat_inc(ipsec_noxform); - goto baddone; + goto drop; } olen = crp->crp_olen; @@ -452,16 +449,13 @@ ipsec_output_cb(struct cryptop *crp) __func__, tdb->tdb_sproto); } - NET_UNLOCK(); if (error) { ipsecstat_inc(ipsec_odrops); tdb->tdb_odrops++; } return; - baddone: - NET_UNLOCK(); - droponly: + drop: if (tdb != NULL) tdb->tdb_odrops++; m_freem(m); @@ -485,6 +479,8 @@ ipsp_process_done(struct mbuf *m, struct tdb *tdb) struct m_tag *mtag; int roff, error; + NET_ASSERT_LOCKED(); + tdb->tdb_last_used = gettime(); if ((tdb->tdb_flags & TDBF_UDPENCAP) != 0) { -- 2.20.1