Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce allocs in ktls app data send #4181

Merged
merged 6 commits into from
Sep 13, 2023
Merged

Reduce allocs in ktls app data send #4181

merged 6 commits into from
Sep 13, 2023

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Sep 1, 2023

Resolved issues:

Resolves #4174 (comment)

Description of changes:

s2n_sendv_with_offset requires us to allocate and copy the iovecs array in some cases. This is disappointing since one of the benefits of ktls is that we pass application data as-is to the kernel, with no buffering or copies.

Likely part of the public ktls documentation will be to advise customers to prefer s2n_sendv over s2n_sendv_with_offset when using ktls. But if an application does call s2n_sendv_with_offset, we can try to limit when we need to copy and/or allocate heap memory. This is a little complicated, but I think it's necessary.

Callouts

The stack memory optimization might be excessive, but it's also probably the simplest part of this logic so I think it's worthwhile.

Testing:

Unit tests, including tests that make assertions about memory allocations.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Sep 1, 2023
@lrstewart lrstewart force-pushed the ktls_send branch 6 times, most recently from 6f33df4 to 5894bf5 Compare September 5, 2023 05:30
@lrstewart lrstewart marked this pull request as ready for review September 5, 2023 05:30
tls/s2n_ktls_io.c Outdated Show resolved Hide resolved
Comment on lines +370 to +371
POSIX_ENSURE_REF(io_context);
POSIX_ENSURE_REF(buf);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this to handle a failure I saw in the tests after rebasing. This change matches our existing non-ktls implementation of the send callback:

s2n-tls/utils/s2n_socket.c

Lines 209 to 212 in bb6052e

int s2n_socket_write(void *io_context, const uint8_t *buf, uint32_t len)
{
POSIX_ENSURE_REF(io_context);
POSIX_ENSURE_REF(buf);
Considering NULL an error makes sense, since the buf is always a reference to known non-null memory in the conn->out buffer.

The root cause of the test failures was #4191 Before I added the tests for this PR, the last sendmsg callback set before the s2n_ktls_send_cb safety tests was s2n_test_ktls_sendmsg_fail. When giving a zeroed context, s2n_test_ktls_sendmsg_fail causes s2n_sendmsg to fail with an errno of 0, which we interpret as S2N_ERR_IO. After I added the tests for this PR, the last sendmsg callback set was the testutils stuffer io callback, which fails with S2N_ERR_IO_BLOCKED when given a zeroed context. So the behavior changed.

I could have fixed this by explicitly setting a callback with known handling of NULLs, but it seemed more correct to just add the NULL checks to s2n_ktls_send_cb.

@lrstewart lrstewart requested a review from goatgoose September 8, 2023 07:16
cppcheck says:
[tls/s2n_ktls_io.c:356]: (style:variableScope) The scope of the variable 'new_bufs_mem' can be reduced.
No. Bad cppcheck. That's how we get stack-use-after-scope.
Comment on lines +928 to +934
struct {
struct s2n_test_iovecs *iovecs;
size_t offset;
uint32_t expected_malloc;
uint32_t expected_malloc_count;
} test_cases[] = {
/* Small iovecs never require an allocation */
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like these tests. Nice work!

@lrstewart lrstewart merged commit 765afea into aws:main Sep 13, 2023
@lrstewart lrstewart deleted the ktls_send branch September 13, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants