ktls: send and recv cmsg #4073
ktls: send and recv cmsg #4073
Conversation
toidiu
force-pushed
the
ak-ktls0_cmsg
branch
6 times, most recently
from
6211759 to
7b69fc3
Compare
toidiu
force-pushed
the
ak-ktls0_cmsg
branch
4 times, most recently
from
a29ffda to
7f3d0e2
Compare
toidiu
force-pushed
the
ak-ktls0_cmsg
branch
4 times, most recently
from
298f03a to
0745051
Compare
| char buf[CMSG_SPACE(sizeof(uint8_t))]; | ||
| struct cmsghdr _align; |
There was a problem hiding this comment.
To understand why alignment is necessary we need to look at cmsghdr (ancillary data). The struct defines a length cmsg_len but then captures the data, cmsg_data[], as a flexible array, which allow you to capture data of arbitrary size. However since the underlying struct is still a cmsghdr, we need to use a union to ensure that the overall allocation aligns with cmsghdr.
struct cmsghdr {
size_t cmsg_len; /* Data byte count, including header */
int cmsg_level; /* Originating protocol */
int cmsg_type; /* Protocol-specific type */
unsigned char cmsg_data[]; */
};
p.s. alignment using union is also included in the man pages for cmsg https://man7.org/linux/man-pages/man3/cmsg.3.html
There was a problem hiding this comment.
https://godbolt.org/z/hbfq91cv1 Neat trick
tls/s2n_ktls_parameters.h
Outdated
There was a problem hiding this comment.
There are some changes coming regarding which headers we define and which we can get from linux/tls.h: #4071 (comment)
However its still possible to review this PR and rebase once that PR is merged/vice-versa. Basically, lets not block this PR on feature probe discussion since that is being handled in the other PR and we can focus only on the cmsg stuff here.
tls/s2n_ktls_io.c
Outdated
| /* Init msghdr */ | ||
| struct msghdr msg = { 0 }; | ||
|
|
||
| #if S2N_KTLS_SUPPORTED /* CMSG_* macros are platform specific */ |
There was a problem hiding this comment.
interleaving this check everywhere makes it a bit hard to follow, IMO. why not just wrap the whole file in this? none of these should even be called if kTLS isn't supported, right?
There was a problem hiding this comment.
could do that but thought we try to limit the ifdef usage to the minimum surface area. Thoughts @lrstewart ?
There was a problem hiding this comment.
i would much rather read a single ifdef at the top of the file and say "ok this whole module is disabled if ktls is disabled" rather than having to make sure each individual function does the right thing.
There was a problem hiding this comment.
Love this idea. We don't so much try to limit ifdef surface area as keep ifdefs readable and minimal. If we can ifdef just one part of one method in a file, then great, keep the surface area as low as possible. But when more ifdefs start creeping in that becomes a problem and ifdefing the whole file definitely becomes the better option :)
But don't forget to have versions of any methods required outside of this file in the else. Otherwise the ifdefs just spread to other files.
There was a problem hiding this comment.
addressed in 0dfbfb3
toidiu
force-pushed
the
ak-ktls0_cmsg
branch
2 times, most recently
from
cd2a9a3 to
8171241
Compare
There was a problem hiding this comment.
For discoverability, this should probably be "s2n_ktls_io_cmsg_test". Or just "s2n_ktls_io_test" if you don't expect to have multiple s2n_ktls_io* test suites.
|
|
||
| /* set send buffer */ | ||
| msg->msg_iov = msg_iov; | ||
| msg->msg_iovlen = count; |
There was a problem hiding this comment.
I don't think this belongs in this method. Mutating your inputs like this seems unexpected. Since you do other initialization of msghdr elsewhere, this initialization should happen elsewhere too.
| char buf[CMSG_SPACE(sizeof(uint8_t))]; | ||
| struct cmsghdr _align; |
There was a problem hiding this comment.
https://godbolt.org/z/hbfq91cv1 Neat trick
| S2N_RESULT s2n_ktls_send_msg_impl(int sock, struct msghdr *msg, | ||
| struct iovec *msg_iov, size_t count, s2n_blocked_status *blocked, ssize_t *send_len); | ||
| S2N_RESULT s2n_ktls_recv_msg_impl(struct s2n_connection *conn, int sock, struct msghdr *msg, | ||
| struct iovec *msg_iov, s2n_blocked_status *blocked, ssize_t *bytes_read); | ||
| S2N_RESULT s2n_ktls_set_ancillary_data(struct msghdr *msg, uint8_t record_type); | ||
| S2N_RESULT s2n_ktls_parse_ancillary_data(struct msghdr *msg, uint8_t *record_type); |
There was a problem hiding this comment.
You're doing the model where you test all the component private methods separately again instead of testing the methods of interest :/ And if you can't actually call s2n_ktls_send_msg / s2n_ktls_recv_msg, doesn't that also prevent you from properly testing any method that calls them? What is your plan for the next layer of tests?
Maybe you need to reuse the refactor from https://github.com/aws/s2n-tls/pull/4071/files#r1268745257 for sendmsg and recvmsg. And at that point you've probably got too many "mocks" to reasonably argue they aren't just for testing, so I'd probably switch to the global version instead of the config version.
|
I will be adding mock IO testing and there are going to be some significant changes. Closing this for now |
Description of changes:
This PR adds methods to read and write cmsg via sendmsg and recvmsg. These methods can be used to send data but also ancillary data, which is needed to communicate the record type when using kTLS.
Since some of the functionality is linux specific (the first platform for which kTLS support will be added), I gate compilation to only that platform. Finally, since kTLS specific headers are not expose by linux uapi (#4064), I also define these headers inline in
s2n_ktls_linux.h.Testing:
Unit tests.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.