Page MenuHomeFreeBSD

Add support for TLS RX via IFNET
Needs ReviewPublic

Authored by hselasky on Oct 7 2021, 2:51 PM.

Details

Summary

MFC after: 1 week
Sponsored by: NVIDIA Networking

Test Plan

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sys/kern/uipc_ktls.c
1051

If we’re running under network epoch, this is not necessary. Nhops are guaranteed to exists within the epoch and each nexthop references the transmit ifp.

sys/kern/uipc_ktls.c
1051

I'll have a look at this. Thank you for your comment!

Include all patches needed on top of the FreeBSD main branch.

FYI, I'm going to pull out the non-TOE-specific bits of the toe_tls_rxswitch branch and post them for review. Once you are able to rebase on those that will help narrow down the diff a bit.

@jhb - just put a note here when your done pushing upstream and I'll rebase.

Update ktls_mbuf_crypto_state() function and rebase.

Implemented split decryption support.

Rebase patch and some fixes.

Add support for TLS RX via LAGG.

TLS RX doesn't support VLAN.

Fixed some issues with TLS RX resync. Rebased and removed TOE patches.

Use next TLS record TCP SN instead of current TLS record. (maybe should pass both so driver can decide)

hselasky edited the test plan for this revision. (Show Details)

Fix uninitialized trail_len variable in offload case.

hselasky edited the summary of this revision. (Show Details)
hselasky edited the test plan for this revision. (Show Details)

Several fixes and minor improvements.

Some other things we talked about on a call today:

  1. While using the route works ok in some cases, it doesn't work for lagg really. Eventually this needs to be replaced with storing a leaf_ifp in an mbuf packet header somehow and using mismatches of that to drive resetting the tag as well as using the leaf_ifp to find the ifp to allocate the initial tag on.
  1. For the case where we are rencrypting an mbuf, I think what we want is to have OCF allocate a separate session just using the cipher (e.g. AES-CTR). We would then encrypt a block of zeroes and then XOR the resulting keystream against the M_ENCRYPTED mbufs to get a fully decrypted TLS record.
  1. We probably want an optimization for the common case in sbappend_ktls_rx() that directly appends a fully decrypted TLS record to sb_m instead of always running the mbufs through the kthread.

In general I think it is also good to figure out if there are some smaller things we can pull out. Some of these changes above might also be version 2 optimization type things. 3) I will want anyway for future TOE TLS changes (and might get around to doing soon).

sys/kern/uipc_ktls.c
1333

Hmm, I need to think about moving this. My TOE patches did move it, but TOE as it is today isn't ready for this to move I think.

1471

I don't think this will really do the right thing if there are multiple TLS records in flight? Or at least, it seems to return an arbitrary future TCP sequence against an older TLS record?

Ah, it seems this needs the first TLS record, and is only really useful in the case that sb_tlscc is zero. For correctness you might also want to subtract off the count of data currently being decrypted as well? We should also document what semantics this returns (first TLS record rather than last).

1644–1655

We might be able to infer this from the existing send tag's type instead and avoid adding the new field to the TLS session if this is the only place it is used.

1651

I would use __assume_notreached() or whatever the wrapper we for that is for cases that are impossible here and in other places.

2164

I think this doesn't handle the case that there are multiple TLS records in the queue? The fetch_info routine tries to handle that case. Or perhaps what happens is that you keep getting TCP sequences that have already passed and can't get in sync until there is only zero or 1 TLS records in the sb_mtls queue?

2278

It might be easier to read if 'state' is an enum of some sort, or at least uses #define values.

2281

I would maybe move these into the case 0 declaring variables here is a bit unusual style.

sys/sys/ktls.h
193

I would perhaps defer renaming this for now until we rename the underlying type? This would also reduce the diff for now?

Rebase patch on top of latest main branch.

Is some description of the changes, or a documentation update, available?

sys/kern/uipc_ktls.c
1069

This check is racy. Is that ok?

1454

Shouldn't this check happen under the inp lock?

hselasky marked an inline comment as done.

Add support for VLAN.

Make defines for all mbuf crypto states.

hselasky marked an inline comment as done.

Address comment from @markj .

hselasky added inline comments.
sys/kern/uipc_ktls.c
1069

Yes, that's OK for now. We also need to implement the RXTLS version of these flags!

kib@ is working on that.

2164

This is not how this mechanism works. The hardware speculates into the TLS header detection, and if a valid chain is found based on a past TCP sequence number which is re-loaded every now and then by the HW, the decryption immediately starts, after that the software confirms this TCP sequence number really points to the start of a valid TLS record.

The callback mechanism into the driver, simply builds a small database record of past TLS TCP start sequence numbers. Later on the driver queries the hardware, where it currently started decrypting, and if a match is found, we are good.

That means this function only provides the TCP SN of the current TLS record being processed and not the last one.

hselasky marked 2 inline comments as done.

Rebase patch for FreeBSD main branch.