Tempesta TLS performance optimizations #1037

krizhanovsky · 2018-07-10T22:37:46Z

Fix #614:

Fix the memory leak in TLS;
Remvoed rx and tx copying queues in TLS;
remove copyings skb_{copy,store}_bits(), all crypto is done in-place now;
fix multiple allocations, calloc(), malloc(), free() wrappers are almost eliminated. At least from crucial workflos for now
fully rewritten mbedTLS state machine for TLS handshakes, so that now it works with chunked data in zero-copy fashion;
unification with linux/crypt - we use the crypt, but RSA and ECDSA which seems incomplete in the kernel
encrypt TLS records with dynamic size calculated in tcp_write_xmit() (see TLS linux-4.14.32-tfw#3): we encrypt several skbs, but not more than a minimum between TCP congestion window and the receiver's advertised window;
Address Redesign of TCP synchronous sending and data caching #391 point 12: now we can use skb->head as paged fragments;
Introduced unified logging as lib/log.h and unified return values
ss_skb_process(): process skb chains, replace parser->to_go by return value variable, so ss_skb_process() returns how much data was actually processed (seems buggy);
Fix Frang: rework limits for number of chunks #214
Address RFC7525 by removing weak algorithms.
Many cleanups and coding style adjustments.

…security

…H_* ciphersuite, but RFC 5246 defines them as mandatory. This reverts commit fda4945.

…security

…H_* ciphersuite, but RFC 5246 defines them as mandatory. This reverts commit fda4945.

aleksostapenko · 2018-12-29T07:27:11Z

tls/tls_srv.c

+	/*
+	 * This is, after ttls_choose_ciphersuite() call, the earliest time when
+	 * when we know which hash function to use and now we can start to
+	 * compute the handshake session checksu.


Double when word.
checksu -> checksum.

aleksostapenko · 2018-12-29T07:30:41Z

tls/tls_srv.c

+
+	/* Read extensins length. */
+	T_FSM_STATE(TTLS_CH_HS_EXTLEN) {
+		/* Use the first 2 bytes for toal number of extensions. */


Maybe total instead of toal should be.
In previous comment: extensins -> extensions.

aleksostapenko · 2018-12-29T07:40:33Z

tls/tls_srv.c

+	 * from a client and then selects a stream or Authenticated Encryption
+	 * with Associated Data (AEAD) ciphersuite, it MUST NOT send an
+	 * encrypt-then-MAC response extension back to the client."
+	 * We don't support other siphersuites, so we don't send EtM extension.


siphersuites -> ciphersuites

aleksostapenko · 2018-12-29T07:49:41Z

tls/tls_srv.c

+			T_FSM_EXIT();
+		CHECK_STATE(1024);
+		/*
+		 * RFC 5246 Certificate Request is optional, so don't requiest


requiest -> request

aleksostapenko · 2018-12-29T08:11:53Z

tls/tls_internal.h

+ * @curves	- supported elliptic curves;
+ * @randbytes	- random bytes;
+ * @finished	- temporal buffer for chunks of Finished message,
+ *		  @randbytes were used in prvious messages, so we can reuse it


prvious -> previous

aleksostapenko · 2018-12-29T08:40:14Z

tls/tls_internal.h

+};
+
+/*
+ * Extend the common FSM for hanshake parsing.


hanshake -> handshake

aleksostapenko · 2018-12-29T08:42:56Z

tls/tls_internal.h

+	*read += p - buf;						\
+	io->rlen += p - buf;
+
+/* Move to @st if we have at least @need bytes. */


Seems that comment is outdated: @need parameter is not present in the macro.

aleksostapenko · 2018-12-29T09:13:27Z

tls/ttls.c

+
+	/*
+	 * Free only the linked list wrapper, not the keys themselves
+	 * since the belong to the SNI callback.


Maybe the -> they.

aleksostapenko · 2018-12-29T09:18:15Z

tls/ttls.c

+	}
+
+	/*
+	 * Calculate final message checksum before goint to


goint -> going

aleksostapenko · 2018-12-29T09:51:53Z

tempesta_fw/tls.c

+	/*
+	 * Perform TLS handshake if necessary and decrypt the TLS message
+	 * in-place by chunks. Add skb to the list to build scatterlist if it
+	 * it contains end of current message.


Double it word.

aleksostapenko · 2018-12-29T09:53:28Z

tempesta_fw/tls.c

+	case T_OK:
+		/*
+		 * A complete TLS message decrypted and ready for upper
+		 * layer protocols processing - fall throught.


throught -> through

aleksostapenko · 2018-12-29T09:55:29Z

tempesta_fw/tls.c

+	 *
+	 * Many sibling skbs can be produced by TLS and HTTP layers
+	 * together - don't coalesce them: we process messages at once
+	 * and it hase sense to work with sparse skbs in HTTP


hase -> has

aleksostapenko · 2018-12-29T09:56:21Z

tempesta_fw/tls.c

+	 * Split @skb before calling HTTP layer to chop it and not let HTTP
+	 * to read after end of the message.
+	 */
+	if (parsed < skb->len) {


It seems that (off + parsed < skb->len) condition should be checked here.

aleksostapenko · 2018-12-29T10:10:52Z

tls/ttls.h

+ *
+ * \param ssl	SSL context
+ *
+ * \return	Protcol name, or NULL if no protocol was negotiated.


Protcol -> Protocol

aleksostapenko · 2018-12-29T10:18:05Z

tls/ttls.h

+ *		  data);
+ * @rlen	- read bytes of the message body so far;
+ * @skb_list	- list of skbs attachd to the current I/O context;
+ * @off		- data offset within @skb_list, can be afther the 1st skb;


attachd -> attached
afther -> after

aleksostapenko · 2018-12-29T10:30:27Z

tls/md.c

-		mbedtls_free(ctx->hmac_ctx);
+	md_ctx->tfm = crypto_alloc_shash(md_info->alg_name, 0, 0);
+	if (IS_ERR(md_ctx->tfm)) {
+		T_ERR("cannot initizlize hash driver %s."


initizlize -> initialize.
The same in __ttls_md_hmac_setup() below.

aleksostapenko · 2018-12-29T12:41:48Z

tempesta_fw/tls.c

-	spin_unlock(&tls->lock);
+next_msg:
+	ss_skb_queue_tail(&tls->io_in.skb_list, skb);
+	msg_skb = tls->io_in.skb_list;


I guess, skb_list processing can be made without msg_skb variable: here and in tfw_tls_chop_skb_rec() - we have access to tls->io_in.skb_list.

Yeah, also there was a bug that tfw_tls_msg_process() gets off from TCP layer as an overlap of sequence numbers, but we use the offset as TLS header for HTTP processing.

Actually ttls_recv() initializes IO context after each processed TLS record so tfw_tls_msg_process() must store skb_list for further chopping in tfw_tls_chop_skb_rec(). Since ttls_recv() zeroes almost full IO, tfw_tls_chop_skb_rec() doesn't need to zero it. Fixed.

aleksostapenko · 2018-12-29T12:52:11Z

tempesta_fw/tls.c

+
+eaten_skb:
+	if (likely(skb->len > prolog)) {
+		off += prolog;


It seems that case with (skb->len > prolog) and (skb->len == prolog + off) may appear here, i.e. skb's space is exhausted but we will continue to deal with it.

Now we chop the possible TCP seqnos overlap at the begin of tfw_tls_msg_process() because we can not link such overlapping skb at the middle of the skb chain for HTTP processing (there is no way to specify offsets for each skb in the list). So prolog went away and off is TLS header and IV.

vankoven · 2018-12-29T13:54:51Z

Makefile

+#
+#	$ DEBUG=3 DBG_SS=1 DBG_TLS=1 make clean all
+#
+# TODO can we generate the variables w/o the copy & paste?


We can use default values:

DEBUG ?= 0 DBG_CFG ?= 0 DBG_HTTP_PARSER ?= 0 TFW_CFLAGS += -DDEBUG=$(DEBUG) -DDBG_CFG=$(DBG_CFG) -DDBG_HTTP_PARSER=$(DBG_HTTP_PARSER) ...

And use in *.c files:

- #ifndef DBG_HTTP_PARSER + #if DBG_HTTP_PARSER == 0 #undef DEBUG #endif

But the more modules we have the more extra debug definitions will be used.

vankoven · 2018-12-29T14:08:43Z

lib/log.h

+/* Non-limited printing. */
+#define T_ERR_NL(...)	T_ERR(__VA_ARGS__)
+#define T_WARN_NL(...)	T_WARN(__VA_ARGS__)
+#define T_LOG_NL(...)	T_LOG(__VA_ARGS__)


Non limited printing is normally USER level warnings like errors or warnings while configuration parsing. There is no need to print call stacks here. TFW_(ERR|WARN|LOG) messages are also used for USER-level only messages and callstacks are not really required there and it sometimes annoying to search though system log and search between callstacks while some of them are not internal errors.

vankoven · 2018-12-29T14:16:12Z

tempesta_fw/http_parser.c

@@ -1131,8 +1214,7 @@ __parse_connection(TfwHttpMsg *hm, unsigned char *data, size_t len)
 	int r = CSTR_NEQ;
 	__FSM_DECLARE_VARS(hm);

-	BUILD_BUG_ON(sizeof(parser->hbh_parser.spec) * CHAR_BIT
-		     < TFW_HTTP_HDR_RAW);
+	BUILD_BUG_ON(sizeof(parser->hbh_parser.spec) * 8 < TFW_HTTP_HDR_RAW);


Why the change is required? Just to fit single line? Not very critical since we support only x86_64, but I prefer to have CHAR_BITS or BITS_PER_BYTE here.

Now it's undeclared and we need to add an include just for the constant

vankoven · 2018-12-29T14:17:08Z

tempesta_fw/sock_clnt.c

+		 * upcall beside GFSM and SS, but that's efficient and I didn't
+		 * find a simple and better solution.
+		 */
+		conn->sk->sk_write_xmit = tfw_tls_encrypt;


Not an issue, but better to use simply sk->sk_write_xmit as everywhere else.

Fix code review comments in this patch since there is significant overlaps in skb offsets. 1. chop skb before splitting in HTTP processing to chop all skbs. 2. offset on TLS layer comes from TCP seqnos overlap, so don't account it for TLS overhead; 3. chop skb TCP overhead before inserting the skb into the list of TLS record skbs.

so tfw_tls_msg_process() must store skb_list for further chopping in tfw_tls_chop_skb_rec(). Since ttls_recv() zeroes almost full IO, tfw_tls_chop_skb_rec() doesn't need to zero it.

vankoven · 2018-12-30T19:52:42Z

tempesta_fw/http.h

@@ -251,6 +250,8 @@ enum {
 	TFW_HTTP_B_WHITELIST,
 	/* Client was disconnected, drop the request. */
 	TFW_HTTP_B_REQ_DROP,
+	/* Request was received on TLS connection. */
+	TFW_HTTP_B_REQ_TLS,


The unused flag is still here.

Oops. Fixed.

aleksostapenko

Looks good to me.

vankoven · 2018-12-31T12:45:55Z

tls/ttls.c

+		n = *sgn + 1;
+	} else {
+		off = io->off;
+		n += *sgn + io->chunks;


Undefined behaviour if buff = null: n is uninitialised.

vankoven · 2018-12-31T12:47:52Z

tls/ttls.c

+			}
+			to_read = min(len, skb->len - off);
+			n = skb_to_sgvec(skb, sg_i, off, to_read);
+			if (n <= 0)


Condition is always false, n is unsigned.

vankoven · 2018-12-31T12:52:59Z

tls/tls_srv.c

+	unsigned char *pms = tls->hs->premaster;
+	unsigned char ver[2], fake_pms[48], peer_pms[48];
+
+	BUILD_BUG_ON(sizeof(tls->hs->premaster) < 0


sizeof(tls->hs->premaster) is unsigned value and never can be less than 0.

vankoven · 2018-12-31T13:00:32Z

tls/tls_srv.c

+
+	tls->hs->pmslen = 48;
+	/* mask = diff ? 0xff : 0x00 using bit operations to avoid branches */
+	mask = - ((diff | - diff) >> (sizeof(unsigned int) * 8 - 1));


diff | -diff seems to be invalid operation here. This operation always result as 0xffff_ffff .

Seems not:

$ cat t.c #include <stdio.h> int main(int argc, char *argv[]) { unsigned int diff = 0; unsigned char mask; mask = -((diff | -diff) >> (sizeof(unsigned int) * 8 - 1)); printf("mask = %x\n", mask); diff = 1; mask = -((diff | -diff) >> (sizeof(unsigned int) * 8 - 1)); printf("mask = %x\n", mask); return 0; } $ gcc t.c && ./a.out mask = 0 mask = ff

vankoven · 2018-12-31T13:00:59Z

tls/tls_srv.c

+
+	tls->hs->pmslen = 48;
+	/* mask = diff ? 0xff : 0x00 using bit operations to avoid branches */
+	mask = - ((diff | - diff) >> (sizeof(unsigned int) * 8 - 1));


diff | -diff seems to be invalid operation here. This operation always result as 0xffff_ffff .

krizhanovsky · 2019-01-01T15:50:28Z

Fixed in #1143 , @ikoveshnikov please review the new PR.

Fix review comments for #1037

krizhanovsky added 30 commits May 23, 2018 17:14

Remove renegotiation as it is dangerous, cleanups

67667d8

Remove dead ifdefs, platform.[ch]

56a7d5c

Replace mbedtls -> ttls

c2f1bd7

Remove couple of dummy headers

b9c1811

Remove deprecated logic

ba81dd0

RFC7525: TLSv1.2 is only supported, remove SSLv3, TLS1.0, TLS1.1

da9796d

Remove weak and null ciphers

afeb4e1

RFC7525: remove RC4 as weak cipher

fa3069c

RFC7525: remove TLS_RSA_WITH_* ciphersuite as not supporting forward …

fda4945

…security

Revert previous commit: RFC7525 suggests not to negotiate TLS_RSA_WIT…

8d0125f

…H_* ciphersuite, but RFC 5246 defines them as mandatory. This reverts commit fda4945.

RFC 7525: remove truncated HMAC as insecure

57815fb

Fix tls unit tests; fix time checking in x509; some cleanups

b3b1ea2

Gigantic messy commit which is no way to review

5441479

Remove more weak ciphers and hashes

d9f9f22

Concept of TLS I/O, many code cleanups, a lot of mess

9c91fbd

Remove renegotiation as it is dangerous, cleanups

f97ba51

Remove dead ifdefs, platform.[ch]

ab48140

Replace mbedtls -> ttls

cf0f409

Remove couple of dummy headers

d513609

Remove deprecated logic

eb485da

RFC7525: TLSv1.2 is only supported, remove SSLv3, TLS1.0, TLS1.1

40d9f0e

Remove weak and null ciphers

f003117

RFC7525: remove RC4 as weak cipher

675af34

RFC7525: remove TLS_RSA_WITH_* ciphersuite as not supporting forward …

e49fc5f

…security

Revert previous commit: RFC7525 suggests not to negotiate TLS_RSA_WIT…

acb5ed7

…H_* ciphersuite, but RFC 5246 defines them as mandatory. This reverts commit fda4945.

RFC 7525: remove truncated HMAC as insecure

0a33427

Fix tls unit tests; fix time checking in x509; some cleanups

3acb258

Gigantic messy commit which is no way to review

a93b94a

Remove more weak ciphers and hashes

cb0a86b

Concept of TLS I/O, many code cleanups, a lot of mess

ddfd806

aleksostapenko reviewed Dec 29, 2018

View reviewed changes

tls/tls_internal.h Outdated

};

/*

* Extend the common FSM for hanshake parsing.

Copy link

Contributor

aleksostapenko Dec 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hanshake -> handshake

aleksostapenko reviewed Dec 29, 2018

View reviewed changes

tls/ttls.c Outdated

}

/*

* Calculate final message checksum before goint to

Copy link

Contributor

aleksostapenko Dec 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goint -> going

aleksostapenko reviewed Dec 29, 2018

View reviewed changes

vankoven reviewed Dec 29, 2018

View reviewed changes

krizhanovsky added 2 commits December 30, 2018 17:49

ttls_recv() initializes IO context after each processed TLS record,

7c82140

so tfw_tls_msg_process() must store skb_list for further chopping in tfw_tls_chop_skb_rec(). Since ttls_recv() zeroes almost full IO, tfw_tls_chop_skb_rec() doesn't need to zero it.

vankoven reviewed Dec 30, 2018

View reviewed changes

Remove unused flag

2d8a951

aleksostapenko approved these changes Dec 31, 2018

View reviewed changes

krizhanovsky merged commit 5a9d217 into master Dec 31, 2018

vankoven reviewed Dec 31, 2018

View reviewed changes

krizhanovsky deleted the ak-tls-memcpq branch January 1, 2019 15:16

krizhanovsky added a commit that referenced this pull request Jan 10, 2019

Merge pull request #1143 from tempesta-tech/ak-1037-fixes

fe511ee

Fix review comments for #1037

krizhanovsky mentioned this pull request Jan 28, 2019

Fix #1157 #1159

Merged

Tempesta TLS performance optimizations #1037

Tempesta TLS performance optimizations #1037

Conversation

krizhanovsky commented Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Dec 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Dec 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky Dec 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko Dec 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky Dec 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleksostapenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krizhanovsky commented Jan 1, 2019 • edited Loading

krizhanovsky commented Jul 10, 2018 •

edited

Loading

aleksostapenko Dec 29, 2018 •

edited

Loading

aleksostapenko Dec 29, 2018 •

edited

Loading

krizhanovsky Dec 30, 2018 •

edited

Loading

aleksostapenko Dec 29, 2018 •

edited

Loading

krizhanovsky Dec 30, 2018 •

edited

Loading

krizhanovsky commented Jan 1, 2019 •

edited

Loading