Page MenuHomeFreeBSD

Fix a sw ktls bug where we would never encrypt anonymous data in place
ClosedPublic

Authored by gallatin on Sep 25 2019, 10:36 PM.
Tags
None
Referenced Files
Unknown Object (File)
Oct 9 2024, 11:49 PM
Unknown Object (File)
Sep 20 2024, 11:53 PM
Unknown Object (File)
Sep 18 2024, 12:18 AM
Unknown Object (File)
Sep 7 2024, 8:49 PM
Unknown Object (File)
Sep 7 2024, 6:48 AM
Unknown Object (File)
Sep 1 2024, 11:13 PM
Unknown Object (File)
Jul 14 2024, 6:25 AM
Unknown Object (File)
Jul 14 2024, 6:25 AM
Subscribers

Details

Summary

Software Kernel TLS needs to allocate a new destination crypto buffer when encrypting data from the page cache, so as to avoid overwriting shared clear-text file data with encrypted data specific to a single socket. When the data is anonymous, eg, not tied to a file, then we can encrypt in place and avoid allocating a new page. This fixes a bug where the existing code always assumes the data is private, and never encrypts in place. This results in unneeded page allocations and potentially more memory bandwidth consumption when doing socket writes.

When the code was written at Netflix, ktls_encrypt() looked at private sendfile flags to determine if the pages being encrypted where part of the page cache (coming from sendfile) or anonymous (coming from sosend). This was broken internally at Netflix when the sendfile flags were made private, and the M_WRITABLE() check was added. Unfortunately, M_WRITABLE() will always be false for M_NOMAP mbufs, since one cannot just mtod() them.

This change introduces a new flags field to the mbuf_ext_pgs struct by stealing a byte from the tls hdr. Note that the current header is still 2 bytes larger than the largest header we support: AES-CBC with explicit IV. We set MBUF_PEXT_FLAG_ANON when creating an unmapped mbuf in m_uiotombuf_nomap() (which is the path that socket writes take), and we check for that flag in ktls_encrypt() when looking for anon pages.

Test Plan

Tested under 90Gb/s of production traffic at Netflix

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb added inline comments.
sys/kern/uipc_ktls.c
1337 ↗(On Diff #62573)

Since is_anon is bool you can get away without the !!. I think the FreeBSD style that kib@ prefers would be more like:

is_anon = (pgs->flags & MBUF_PEXT_FLAG_ANON) != 0;
sys/sys/mbuf.h
336 ↗(On Diff #62573)

s/data/Data/ and maybe add a trailing period.

352 ↗(On Diff #62573)

Flags instead of flags?

This revision is now accepted and ready to land.Sep 27 2019, 6:10 PM

Applied suggested changes prior to committing.