ktls: Ensure FIFO encryption order for TLS 1.0.

[jhb]

Summary

TLS 1.0 records are encrypted as one continuous CBC chain where the
last block of the previous record is used as the IV for the next
record. As a result, TLS 1.0 records cannot be encrypted out of order
but must be encrypted as a FIFO.

If the later pages of a sendfile(2) request complete before the first
pages, then TLS records can be encrypted out of order. For TLS 1.1
and later this is fine, but this can break for TLS 1.0.

To cope, add a queue in each TLS session to hold TLS records that
contain valid unencrypted data but are waiting for an earlier TLS
record to be encrypted first.

• In ktls_enqueue(), check if a TLS record being queued is the next record expected for a TLS 1.0 session. If not, it is placed in sorted order in the pending_records queue in the TLS session.
  
  If it is the next expected record, queue it for SW encryption like normal. In addition, check if this new record (really a potential batch of records) was holding up any previously queued records in the pending_records queue. Any of those records that are now in order are also placed on the queue for SW encryption.

• In ktls_destroy(), free any TLS records on the pending_records queue. These mbufs are marked M_NOTREADY so were not freed when the socket buffer was purged in sbdestroy(). Instead, they must be freed explicitly.

Sponsored by: Netflix

[jhb]

Testing this was a bit challenging. I have a test client that uses msync(MS_INVALIDATE) to flush out every over MB of a file before it sendfile()'s it. However, even with that I ended up adding a hack into sendfile_iodone() to use a callout to defer ktls_enqueue with a random delay. This did allow me to provoke some misordering and I have some KTR traces that show it working, but most of the time I only managed to get two batches of records misordered relative to each other. I did get 2 batches deferred once while waiting for the missing record:

  5223 ktls_enqueue: unpending TLS seq 67856
  5222 ktls_enqueue: unpending TLS seq 67827
  5219 ktls_enqueue: queueing TLS seq 67856 at the end
  5218 ktls_enqueue: queueing TLS seq 67827 at the end
  1148 ktls_enqueue: unpending TLS seq 6095
  1145 ktls_enqueue: queueing TLS seq 6095 at the end
  1099 ktls_enqueue: unpending TLS seq 5380
  1096 ktls_enqueue: queueing TLS seq 5380 at the end
  1080 ktls_enqueue: unpending TLS seq 5117
  1077 ktls_enqueue: queueing TLS seq 5117 at the end
  1001 ktls_enqueue: unpending TLS seq 4053
   998 ktls_enqueue: queueing TLS seq 4053 at the end
   884 ktls_enqueue: unpending TLS seq 2692
   881 ktls_enqueue: queueing TLS seq 2692 at the end
...

[jhb]

sys/kern/uipc_ktls.c

1501

I have not tested that this cleans up ok in case of an encryption error or I/O error.

2117

I have considered changing m_epg_enc_cnt to be a count of TLS records instead of a count of pages. We could compute the needed page count in the loop in ktls_encrypt. However, optimizing for TLS 1.0 probably isn't very worthwhile.

[gallatin]

sys/kern/uipc_ktls.c

2182

Do you really want to return here? I think there can be cases where what you've inserted puts the pending records in order and gives you something to process.. Also, if you return here, is there a backstop to ensure processing of the pending records if this was the batch to be enqueued?

Eg, what's wrong with putting the tls->next_seqno += and STAILQ_INSERT_TAIL() into an else, and then attempting processing of the pending records?

[jhb]

sys/kern/uipc_ktls.c

2182

No, tls->next_seqno is the next record I need to process. At any given time, all of the TLS records in tls->pending_records must have sequence numbers > tls->next_seqno. To be in this case, m->m_epg_seqno must also be > than tls->next_seqno. The loop below is what releases pending records once the missing record shows up.

If the record is the missing record, then we don't go into this if and instead queue the missing record and then look to see if that frees up more records. However, we can only make forward progress when a record that is tls->next_seqno shows up.

Note that this design effectively has tls->pending_records as a queue in front of the encryption work queue. It doesn't wait for the missing record to be decrypted, it just ensures that the records are placed into the encryption work queue in FIFO order.

[gallatin]

sys/kern/uipc_ktls.c

2182

Makes sense, thanks!

[markj]

sys/kern/uipc_ktls.c

1513

These three lines can be replaced with m = m_free(m);.

2182

I think a brief comment along these lines would be worthwhile to have here.