tls: encrypting in new sk_buff pages, if `sendfile()` is used or response from cache #1264

avbelov23 · 2019-05-29T15:55:12Z

Transfer the tx_flags to the new skb_buff and encrypt in the new sk_buff pages if SKBTX_SHARED_FRAG is set or response from cache when send response via https.

i-rinat

Current version of PR introduces redundant data copying, which could be avoided.

tempesta_fw/tls.c

krizhanovsky

There are several issues with the code. Also @i-rinat proposed much better design - this is the right way to go.

krizhanovsky · 2019-06-03T13:42:03Z

tempesta_fw/ss_skb.c

 		__copy_ip_header(*skb_head, skb);
+		skb_shinfo(*skb_head)->tx_flags = skb_shinfo(skb)->tx_flags;


Socket buffer unrolling is supposed to receive list of skbs on GRO and unroll it to separate skbs, which will be processed separately in Tempesta and the TCP/IP stack aftewards. The sense of GROed lists of skbs is that the only head skb is processed in control flow and the rest of skbs are processed only when some data is needed, so all the control information is kept in head skb only.

It seems this is a wrong place for the flags copying - you set the flags only for the first skb, bu all the skbs must have the flag. You should do very similar things as we propagate skb->mark: the first newly allocated skb gets mark from the original skb at the first ss_skb_queue_coalesce_tail() call in ss_skb_unroll_slow(). Next, we copy the mark to all frags f_skb and they pass the mark to any newly allocated skb if necessary.

krizhanovsky · 2019-06-03T13:52:19Z

tempesta_fw/tls.c

 	int r;
 	TlsCtx *tls = tfw_tls_context(c);
 	TlsIOCtx *io = &tls->io_out;

+	while ((skb = ss_skb_dequeue(&msg->skb_head))) {
+		if (skb_shinfo(skb)->tx_flags & SKBTX_SHARED_FRAG) {


Ok, lovely to see that splice() sets the flag. Please also investigate MSG_ZEROCOPY usage in modern kernels (see comment #1217 (comment)) . Please explore the latest kernel for the flag and at least write a comment what should we do if a application server uses it for sending us the data. The comment will be very helpful for further migration to newer kernel and we won't forget about the problem.

BTW @ikoveshnikov is right in #1217 (comment) : we should copy only response body for cached responses, headers are copied anyway.

As far as I can tell, SKBTX_DEV_ZEROCOPY is set when MSG_ZEROCOPY is used:

static inline struct ubuf_info *skb_zcopy(struct sk_buff *skb) { bool is_zcopy = skb && skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY; return is_zcopy ? skb_uarg(skb) : NULL; }

#define SKBTX_ZEROCOPY_FRAG (SKBTX_DEV_ZEROCOPY | SKBTX_SHARED_FRAG)

So, maybe we should check for SKBTX_ZEROCOPY_FRAG rather than only for SKBTX_SHARED_FRAG?

krizhanovsky · 2019-06-03T18:32:53Z

tempesta_fw/tls.c

+{
+	int i;
+	struct sk_buff *frag_iter;
+


Need a comment why don't we need to copy skb head.

krizhanovsky · 2019-06-03T18:36:26Z

tempesta_fw/tls.c

+
+		p = skb_frag_page(f);
+		order = get_order(frag_size);
+		p2 = alloc_pages(__GFP_MEMALLOC, order);


We're in softirq, so GFP_ATOMIC must be used instead

krizhanovsky · 2019-06-03T18:37:19Z

tempesta_fw/tls.c

+			goto err;
+		vaddr = kmap_atomic(p);
+		vaddr2 = kmap_atomic(p2);
+		memcpy(vaddr2, vaddr + f->page_offset, frag_size);


memcpy() sucks, we can and should use memcpy_fast()

krizhanovsky · 2019-06-03T18:42:48Z

tempesta_fw/tls.c

+		vaddr2 = kmap_atomic(p2);
+		memcpy(vaddr2, vaddr + f->page_offset, frag_size);
+		kunmap_atomic(vaddr2);
+		kunmap_atomic(vaddr);


We're in softirq, so there is no preemtion or page faults, so it's safe just to use page_address().

tempesta_fw/tls.c

i-rinat

I have some comments about the code. See below.

tempesta_fw/tls.c

tls/ttls.c

vankoven · 2019-06-19T11:54:50Z

tempesta_fw/tls.c

+				GFP_ATOMIC);
+		if (!pages) {
+			T_WARN("cannot alloc memory.\n");
+			return -ENOMEM;


Memory leak happens here. In case of the error, the out_sgt.sgl wouldn't be freed.

tempesta_fw/tls.c

avbelov23 · 2019-06-20T14:55:50Z

DONE

vankoven · 2019-06-21T06:15:45Z

tempesta_fw/cache.c

@@ -1579,6 +1579,9 @@ tfw_cache_build_resp(TfwHttpReq *req, TfwCacheEntry *ce)
 	if (tfw_cache_build_resp_body(db, resp, trec, &it, p))
 		goto err;

+	if (TFW_CONN_TLS(req->conn))


This should happen right after the response object is allocated in tfw_cache_build_resp() function, i.e. after tfw_http_msg_setup(). Please add a comment here, why the flag is added and which problem it solves.

And we have problem here, only very first skb is marked with the flag, all the following skbs in the message won't have the flag. Bit in the same time huge responses may require more than one skb. Seems like we should care of this case in tfw_msg_iter_append_skb() function.

vankoven

Looks good to me

i-rinat

I think we need to revert to SKBTX_SHARED_FRAG, since we don't handle working with user pages yet.

tempesta_fw/cache.c

tempesta_fw/tls.c

i-rinat

Looks good to me.

All comments from the review were addressed.

krizhanovsky

Good job! I tested #1217 and #1275 cases against the code and can confirm that the issues are gone, so close them on the merge. However, there are couple notes, and one of them relatively crucial, which must be fixed before merge.

krizhanovsky · 2019-07-01T12:41:51Z

tempesta_fw/tls.c

+		if (!pages) {
+			T_WARN("cannot alloc memory.\n");
+			r = -ENOMEM;
+			goto err_pages_alloc;


Dynamic allocation is relatively heavy operation and I see no reason to call kmalloc() 3 times - you free all the allocations at the same time.

krizhanovsky · 2019-07-01T12:47:58Z

tempesta_fw/tls.c

+			if (r <= 0)
+				goto out;
+			out_frags += r;
+		}


tfw_tls_encrypt() is already overloaded function and the code just prepares memory for encrypted data, so it could make sense to move it to separate function in ss_skb. However, I'm not sure how many local variables are used - if the function must accept more than standard 6 arguments, them let's leave with the current code.

krizhanovsky · 2019-07-01T12:48:19Z

tempesta_fw/tls.c

 		ttls_aad2hdriv(xfrm, skb->data);

 	spin_unlock(&tls->lock);

+	for (i = 0; i < elt; ++i) {
+		put_page(pages[i]);
+	}


Do not need braces for single line code blocks

…ache

krizhanovsky

Looks good

backport tls encrypting in new sk_buff pages, if `sendfile()` is used or response from cache, from #1264 to release 0.6 branch

avbelov23 changed the title ~~copy sk_buff pages, if sendfile () is used~~ tls: copy sk_buff pages, if sendfile () is used May 29, 2019

avbelov23 changed the title ~~tls: copy sk_buff pages, if sendfile () is used~~ tls: copy sk_buff pages, if sendfile() is used May 29, 2019

i-rinat reviewed May 29, 2019

View reviewed changes

tempesta_fw/tls.c Outdated Show resolved Hide resolved

tempesta_fw/tls.c Outdated Show resolved Hide resolved

tempesta_fw/tls.c Outdated Show resolved Hide resolved

krizhanovsky previously requested changes Jun 3, 2019

View reviewed changes

avbelov23 force-pushed the avb-sendfile2 branch 7 times, most recently from 491f99c to 7b8806e Compare June 17, 2019 12:00

avbelov23 changed the title ~~tls: copy sk_buff pages, if sendfile() is used~~ tls: encrypt in new sk_buff pages, if sendfile() is used or response from cache Jun 17, 2019

avbelov23 changed the title ~~tls: encrypt in new sk_buff pages, if sendfile() is used or response from cache~~ tls: encrypting in new sk_buff pages, if sendfile() is used or response from cache Jun 17, 2019

i-rinat reviewed Jun 18, 2019

View reviewed changes

vankoven reviewed Jun 19, 2019

View reviewed changes

avbelov23 force-pushed the avb-sendfile2 branch 3 times, most recently from 33089bf to 3de5b27 Compare June 20, 2019 14:53

avbelov23 changed the title ~~tls: encrypting in new sk_buff pages, if sendfile() is used or response from cache~~ tls: encrypting in new sk_buff pages, if sendfile() or MSG_ZEROCOPY is used or response from cache Jun 20, 2019

vankoven reviewed Jun 21, 2019

View reviewed changes

avbelov23 force-pushed the avb-sendfile2 branch 2 times, most recently from 7fa1ce8 to 284a8d0 Compare June 21, 2019 11:17

vankoven approved these changes Jun 21, 2019

View reviewed changes

i-rinat suggested changes Jun 21, 2019

View reviewed changes

tempesta_fw/cache.c Outdated Show resolved Hide resolved

tempesta_fw/cache.c Show resolved Hide resolved

tempesta_fw/tls.c Outdated Show resolved Hide resolved

tempesta_fw/tls.c Outdated Show resolved Hide resolved

i-rinat requested a review from krizhanovsky June 21, 2019 19:21

avbelov23 force-pushed the avb-sendfile2 branch from 284a8d0 to 061bddb Compare June 24, 2019 13:14

i-rinat approved these changes Jun 24, 2019

View reviewed changes

avbelov23 changed the title ~~tls: encrypting in new sk_buff pages, if sendfile() or MSG_ZEROCOPY is used or response from cache~~ tls: encrypting in new sk_buff pages, if sendfile() is used or response from cache Jun 25, 2019

i-rinat mentioned this pull request Jun 25, 2019

tls.test_tls_stress read errors #1275

Closed

avbelov23 force-pushed the avb-sendfile2 branch 2 times, most recently from d55621e to 2ce2672 Compare June 28, 2019 13:44

krizhanovsky requested changes Jul 1, 2019

View reviewed changes

avbelov23 force-pushed the avb-sendfile2 branch from 2ce2672 to 6d31f79 Compare July 1, 2019 15:16

encrypt in new sk_buff pages, if sendfile () is used or resp from c…

fe18fc4

…ache

avbelov23 force-pushed the avb-sendfile2 branch from 6d31f79 to fe18fc4 Compare July 1, 2019 15:37

krizhanovsky approved these changes Jul 1, 2019

View reviewed changes

avbelov23 merged commit 1d7f538 into master Jul 1, 2019

avbelov23 deleted the avb-sendfile2 branch July 1, 2019 16:02

avbelov23 mentioned this pull request Jul 2, 2019

backport tls encrypting in new sk_buff pages, if sendfile() is used or response from cache, from #1264 to release 0.6 branch #1291

Merged

avbelov23 added a commit that referenced this pull request Jul 2, 2019

Merge pull request #1291 from tempesta-tech/avb-06-sendfile

8aa60a3

backport tls encrypting in new sk_buff pages, if `sendfile()` is used or response from cache, from #1264 to release 0.6 branch

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: encrypting in new sk_buff pages, if `sendfile()` is used or response from cache #1264

tls: encrypting in new sk_buff pages, if `sendfile()` is used or response from cache #1264

avbelov23 commented May 29, 2019 •

edited

Loading

i-rinat left a comment

krizhanovsky left a comment

krizhanovsky Jun 3, 2019

krizhanovsky Jun 3, 2019

i-rinat Jun 19, 2019

krizhanovsky Jun 3, 2019

krizhanovsky Jun 3, 2019

krizhanovsky Jun 3, 2019

krizhanovsky Jun 3, 2019

i-rinat left a comment •

edited

Loading

vankoven Jun 19, 2019

avbelov23 commented Jun 20, 2019

vankoven Jun 21, 2019

vankoven left a comment

i-rinat left a comment

i-rinat left a comment

krizhanovsky left a comment

krizhanovsky Jul 1, 2019

krizhanovsky Jul 1, 2019

krizhanovsky Jul 1, 2019

krizhanovsky left a comment

		__copy_ip_header(*skb_head, skb);
		skb_shinfo(*skb_head)->tx_flags = skb_shinfo(skb)->tx_flags;

tls: encrypting in new sk_buff pages, if sendfile() is used or response from cache #1264

tls: encrypting in new sk_buff pages, if sendfile() is used or response from cache #1264

Conversation

avbelov23 commented May 29, 2019 • edited Loading

i-rinat left a comment

Choose a reason for hiding this comment

krizhanovsky 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

Choose a reason for hiding this comment

i-rinat left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avbelov23 commented Jun 20, 2019

Choose a reason for hiding this comment

vankoven left a comment

Choose a reason for hiding this comment

i-rinat left a comment

Choose a reason for hiding this comment

i-rinat left a comment

Choose a reason for hiding this comment

krizhanovsky 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

krizhanovsky left a comment

Choose a reason for hiding this comment

tls: encrypting in new sk_buff pages, if `sendfile()` is used or response from cache #1264

tls: encrypting in new sk_buff pages, if `sendfile()` is used or response from cache #1264

avbelov23 commented May 29, 2019 •

edited

Loading

i-rinat left a comment •

edited

Loading