-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
sockets: tls: Make secure sockets support posix APIs #11438
sockets: tls: Make secure sockets support posix APIs #11438
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11438 +/- ##
==========================================
- Coverage 48.17% 48.13% -0.04%
==========================================
Files 281 281
Lines 43362 43425 +63
Branches 10393 10406 +13
==========================================
+ Hits 20889 20902 +13
- Misses 18318 18368 +50
Partials 4155 4155
Continue to review full report at Codecov.
|
@@ -98,6 +91,15 @@ int _impl_zsock_socket(int family, int type, int proto) | |||
return fd; | |||
} | |||
|
|||
int _impl_zsock_socket(int family, int type, int proto) | |||
{ | |||
#if defined(CONFIG_NET_SOCKETS_SOCKOPT_TLS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this is still far from ideal. Could go as a continued quick hack, but we should implement a generic framework for "socket factories" dispatched on input params (to cover cases like #11454).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention was not to inject too much TLS realated code into sockets.c
. But I can see that as there are more socket 'types' coming into Zephyr, this might be unavoidable. Will rethink this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave as is for now.
subsys/net/lib/sockets/sockets.c
Outdated
@@ -131,6 +133,17 @@ int zsock_close_ctx(struct net_context *ctx) | |||
return 0; | |||
} | |||
|
|||
int zsock_close_dispatch(struct net_context *ctx) | |||
{ | |||
#if defined(CONFIG_NET_SOCKETS_SOCKOPT_TLS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not dispatch on vtable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to touch the vtable, mostly due to sock_to_net_ctx
used e. g. in poll
. Using different vtable will not allow me to use it out-of-the-box as it is now. Yet I understand your point regarding switching vtable, will have to rethink this approach.
subsys/net/lib/sockets/sockets.c
Outdated
SET_ERRNO(net_context_connect(ctx, addr, addrlen, NULL, K_FOREVER, | ||
NULL)); | ||
SET_ERRNO(net_context_recv(ctx, zsock_received_cb, K_NO_WAIT, ctx->user_data)); | ||
#if defined(CONFIG_NET_SOCKETS_SOCKOPT_TLS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is harder case, vtable doesn't support it, and bloat it up right away doesn't seem right. Comments welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there's offloading vtable which does that ;-).
@rlubos, Thanks for the patch, it definitely a step in the right direction. But very small and shy step. And we have people crowding up for wider road (e.g. #11454). And no matter if we say it's their problem, it's actually ours, as more experiences with socket subsys. So, I would like to encourage us to not take goose steps here, as it'll only require more red/green paint (i.e. changing things back and forth). We have a foundation for implement generic solution to support different types of socket implementation - it's fdtable, on which sockets depend. So, we already decouple socket namespace from polymorphic namespace(s) of actual implementation(s). The issue we have is that operation space of the fdtable is limited, with literally read and write only, and everything else multiplexed thru ioctl. We apparently won't be routing recv() thru ioctl, so need to come up with a better solution. The crux of my proposal is to "subclass" fdtable_ops vtable, and add more methods to the subclass:
Then we need fdtable getter which not just checks vtable, but instead returns it, allowing us to dispatch on it ourselves. And thus, all top-level connect(), listen() just dispatches on it. Type checking would be an issue, but we can use a known trick of checking type based on vtable method identity. I.e., all sockets would have same read() method (redirecting to recv), and we can test that. Let me know what you think. @GAnthony: likewise. It should be clear this this method supercedes existing socket offloading completely. |
@pfalcon Thank you for the feedback. I agree that it might be a good time to make it more generic given the fact that implementations like #11454 are coming in. I can try to implement something according to your proposal and update the PR. I just wanted to clarify if I understand the proposal correctly. So my understaing is that we would need to extend
Then, provided we created fd with socket "subclass" of the After that, we can call specific function from the vtable, with object as an argument (or one of the arguments). The function from vtable will have to interpret the object argument correctly, according to the implementation. Is that correct or did I mix up the idea?
I don't get this part, could you elaborate? |
Thanks, much appreciated! (I feel that my plate is filled with work at least for this year otherwise ;-/).
Ack, exactly how I pictured it myself too.
Yep, typecast, and type-check as discussed above.
Yes, we just need to pre-check that it's socket object, not a bare fd object (which doesn't have extended socket vtable). To do that, we check that a method pointer as stored in the base fd vtable has a well-known socket value. I.e. for sockets, e.g. read would be:
Of course, that means that read() will make an extra call redirect, but that's the price of magic (can be optimized to tail call where it's merely a jump). If you have other ideas how to typecheck bare-fd vs socket objects, let's discuss. |
@pfalcon Thanks for explaination, hopefully I understood the proposal correctly now. I've implemented it this way for POSIX sockets so far, yet I've faced one obstacle in The problem is that I'm missing some information in these functions that would allow me to make them common for all socket implementations (so that we can verify that we deal with a socket). For instance, for
In order to make this call generic I need either:
Both would require O(n) function that would crawl through the fd table to retrieve information needed (perhaps that's not an issue, it would only be needed for functions mentioned above). What do you think, is that fine? An alternative solution I can see is to leave current fd vtable as is, and extend the object passed to the fd table to something like:
But that would require some extra memory in the socket module to store those extended objects, therefore injecting a limitation on the maximum number of sockets into the socket subsystem itself. |
Damn, that appears to be a similar issue which I faced when converting socket.c to fdtable, why I needed all those refactors to it, which broke tls.c.
Nope, that's not fine. Well, even direct calls aren't "fine" (they're a chore for CPU to predict apparently), but a necessary compromise to achieve genericity/extensibility. But O(n) should be disallowed anywhere possible. Another obvious solution would be to pass extra arg to read/write/ioctl, but that goes against the original requirement that fdtable support is fully transparent for underlying objects, they just need to implement POSIX-like API to be plugged into it. I would be possible to solve that by supporting different method signature types, and using flags to select one, but that's a bit rabbit-hole'ish on this stage I guess. Let's keep thinking about it, but for now scale back to the approach where each socket impl has its own (duplicated, yeah) impl of read/write/ioctl. |
That also would be a bad compromise, again, the idea was to not burden clients of fdtable with (too much) of its impl details. As I say, even passing extra non-POSIX arg sucks, and burdening clients with spending more (RAM) memory on fdtable support should be also no-no. Well, that's just my quick thoughts, following the original fdtable requirements. We may need to re-schedule them, but based on arguments. So, I propose for now to follow the "natural" schedule of priority: 1) Have clean, workaround-less API; 2) Save RAM; 3) Save ROM. I.e. if there's a conflict, wasting ROM is better (unless later proven otherwise). |
7b7b89d
to
2f26c44
Compare
Pushed updated version, based on extended socket vtable. Still need to rework |
Pushed new commit with The proposal adds two new functions to the socket vtable: |
@rlubos: Thanks for keeping up the great work!
The poll support is a harder issue, thanks for looking into it. Can comment on the above right away: 1) supporting poll() just on socket level isn't general enough, it needs to be supported on fd level; 2) the original fd_vtable was designed to provide read/write direct methods, and route everything else via ioctl method. I'd suggest to rework your approach to follow the above. That of course means there's bigger overhead to sustain on each poll call. But poll() on its own is quite overheady with its O(n) design. The only way out for efficient implementation is to switch to epoll design, where initial one-time setup cost is amortized across multiple "do the stuff" calls. Raising an RFC for epoll design is on my TODO (have too many things on hands otherwise). |
A (more) ideal design would involve just prepare-like call, but would store a higher-level object in "user data" of poll structure. As we don't support that with current k_poll structures, I would understand the need for the "update" call. Need for user data should be a requirement for epoll(-like) design. |
@pfalcon Thanks, so I understand that I should:
Is that correct? |
Yes, please see sock_ioctl_vmeth(), which so far just implements ZFD_IOCTL_CLOSE. You would need to define something like ZFD_IOCTL_POLL_PREPARE, ZFD_IOCTL_POLL_UPDATE, and implement them, instead of dedicated socket vmethods. |
ce5dc54
to
e41e894
Compare
Pushed updated version, with |
Cool, so should this now be ready for the final review? |
@pfalcon Yes. |
Just an optimization note - if we really like, we probably can make ->ioctl() return error as negative value, and make caller set errno based on that. But that would apply to all ioctl ops, and given that lseek implemented via it, probably not good idea. And it would be a pretty minor optimization indeed. |
Well, nope, no need it to bring up all ops here. PREPARE/UPDATE are internal ops, and have there own naming conventions, so I'd do that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for great work here, I very well see that it took much more refactoring than initially thought.
My only request is let's keep things frugal - close, send, recv, fcntl vmethods are superfluous.
Beyond that, I don't understand why/how TLS sockets instead of keeping going nicely via normal socket interface started to fish in normal socket implementation details. But as I told, this already looks complex enough, if we'll need cleanup/optimization iterations beyond this code, can do that, later.
@@ -28,4 +30,25 @@ static inline u32_t sock_get_flag(struct net_context *ctx, u32_t mask) | |||
#define sock_set_eof(ctx) sock_set_flag(ctx, SOCK_EOF, SOCK_EOF) | |||
#define sock_is_nonblock(ctx) sock_get_flag(ctx, SOCK_NONBLOCK) | |||
|
|||
struct socket_op_vtable { | |||
struct fd_op_vtable fd_vtable; | |||
int (*close)(void *obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but close is handled via ioctl(ZFD_IOCTL_CLOSE).
socklen_t addrlen); | ||
int (*listen)(void *obj, int backlog); | ||
int (*accept)(void *obj, struct sockaddr *addr, socklen_t *addrlen); | ||
ssize_t (*send)(void *obj, const void *buf, ssize_t len, int flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
send() is just a special case of sendto(), likewise recv() vs recvfrom(). Doesn't make sense to have methods/implementations for both.
const struct sockaddr *dest_addr, socklen_t addrlen); | ||
ssize_t (*recvfrom)(void *obj, void *buf, size_t max_len, int flags, | ||
struct sockaddr *src_addr, socklen_t *addrlen); | ||
int (*fcntl)(void *obj, int cmd, int flags); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's implement this via ioctl().
My original idea was to route setsockopt()/getsockopt() via ioctl too, but due to the need to devise packing scheme for level/optname, that can be done later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, same with other redundant functions.
subsys/net/lib/sockets/sockets_tls.c
Outdated
static inline struct net_context *sock_to_net_ctx(int sock) | ||
{ | ||
return z_get_fd_obj(sock, | ||
(const struct fd_op_vtable *)&tls_sock_fd_op_vtable, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably could use &tls_sock_fd_op_vtable.fd_vtable
here "for more typesafety", but that's minor.
sock = zsock_socket(family, type, proto); | ||
if (sock < 0) { | ||
/* errno will be propagated */ | ||
ret = net_context_get(family, type, proto, &ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, why this replacement? Why go away from zsock_socket() to raw net_context_get(), then do manual k_fifo_init(&ctx->recv_q), etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for that is that I need to create file descriptor with my own vtable, yet I cannot rely on zsock_socket_internal
in current form. Alternatively, I could separate generic part of zsock_socket_internal
into a different function and then call it from both zsock_
and ztls_
, leaving the fd management up to the implementation.
return -1; | ||
} | ||
|
||
if (parent_context->tls) { | ||
child_context = sock_to_net_ctx(child_sock); | ||
child = k_fifo_get(&parent->accept_q, K_FOREVER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, why replace zsock_accept with going down implementation details which can change any time?
{ | ||
u32_t elapsed = k_uptime_get_32() - start; | ||
|
||
return timeout - elapsed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure MISRA will want cast here, and I'd add it for clarity anyway.
@@ -724,14 +718,66 @@ Z_SYSCALL_HANDLER(zsock_fcntl, sock, cmd, flags) | |||
} | |||
#endif | |||
|
|||
int zsock_poll_internal(struct zsock_pollfd *fds, int nfds, int timeout) | |||
static int zsock_poll_prepare_ctx(struct net_context *ctx, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this signature somewhat adhoc on the current implementation. Wanted to propose to change, but nevermind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But note that the other way is to pass structure with all these args, which would avoid too much va_list'ing. But I'm not sure it will make things better (more optimal). Please don't change unless you're sure that would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about having a strucutre to wrap the parameters. Yet I figured that in order to make it right I would need to specify it in fdtable.h
, yet creating dependency to socket.h
there (zsock_pollfd
) so I dropped the idea.
@@ -98,6 +91,15 @@ int _impl_zsock_socket(int family, int type, int proto) | |||
return fd; | |||
} | |||
|
|||
int _impl_zsock_socket(int family, int type, int proto) | |||
{ | |||
#if defined(CONFIG_NET_SOCKETS_SOCKOPT_TLS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave as is for now.
return zsock_close_ctx(obj); | ||
} | ||
|
||
static int sock_bind_vmeth(void *obj, const struct sockaddr *addr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ummm, and we need all these wrappers because ... ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beacuse for instance sock_bind_vmeth
and zsock_bind_ctx
ahve different signatures (void *obj
vs struct net_context *ctx
). GCC will throw warning when using zsock_bind_ctx
in the vtable directly.
@rlubos: Thanks for responses to my questions, I see that while there're "different ways to do it", it's unclear if a different way would be better than how you coded it. because there're unobvious details with it all, so let's just minimize socket op vtable as discussed, and move on with this. |
e41e894
to
adecc06
Compare
Pushed updated version with vtable trimmed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for great work here!
@jukkar, @tbursztyka : Please help with reviewing this. @andrewboie, @andyross : You guys are pulled in this review because it contains fairly small changes to lib/fdtable.c as introduced by me recently. Surely please review if you can, but if there will be enough reviews collected and someone else will be about to merge it, I hope you won't object. |
@carlescufi, @nashif : So, I'm not sure how many more approvals we'll be able to collect here. Please see if you can approve and/or merge this. |
@rlubos can you please rebase? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add function that allows to obtain both object and vtable of the file descriptor. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
This commit extends socket vtable, allowing to redirect socket calls to alternate implementations (e.g. TLS sockets). Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
Implement extended socket vtable for TLS sockets, therefore allowing to integrate the implementation with socket subsystem. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
This commit reworks socket poll implementation to support multiple socket implementations. To achieve that, two ioctl poll helper requests were added: ZFD_IOCTL_POLL_PREPARE and ZFD_IOCTL_POLL_UPDATE. The poll implementation calls ioctl with these requests for each socket requested in the fds table. The first request is responsible for preparing k_poll_event objects for specific socket. It can request to skip waiting in k_poll by returning EALREADY through errno. The latter request is responsible for processing outcome of k_poll for each socket. It can request to retry the k_poll by returning EAGAIN through errno. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
adecc06
to
1b1611d
Compare
Rebased |
|
||
/* Pass the call to the core socket implementation. */ | ||
va_start(args, request); | ||
err = sock_fd_op_vtable.fd_vtable.ioctl(obj, request, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rlubos: Sorry, I didn't notice this before. What is happening here? did you test this? Let me conjecture this isn't correct. You pass a va_list
here, but what va_list is an effectively a pointer to first argument. But ...
in ioctl() means an argument value, not address.
So, while there's almost certainly an issue, the way to fix it may be actually to make ->ioctl() take va_list
as the last argument, not ...
. Because you code shows a common problem with variadic functions: how to dispatch them, preserving the original args? The only portable solution would be to convert ...
to va_list
and pass va_list
around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pfalcon Right, giving it a second look I see it's not correct. I"ve ran the only sample that use fcntl
(echo_async) with TLS socket and it worked so I assumed solutions works, but yeah apparently that's not correct.
I can try to fix it in a way you described, yet only converting ...
to va_list
doesn't seem to be enough. We still want a code like this to work:
Line 202 in c012a28
return fdtable[fd].vtable->ioctl(fdtable[fd].obj, ZFD_IOCTL_LSEEK, |
Shall we provide a generic wrapper to ioctl which would dispatch to vtable and call it there? Something like this:
int ioctl(int fildes, int request, ... /* arg */)
{
...
va_start(args, request);
return fdtable[fd].vtable->ioctl(fdtable[fd].obj, request, args);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rlubos: I didn't mention that I'm working on all this stuff, so please don't spend cycles on that, but please be prepared to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, thanks for clarifying this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A hasty WIP PR, just to show progress is posted: #12002
Move secure sockets integration below
zsock_*
API, to allow them to use FD posix APIs - read/write/ioctl.Signed-off-by: Robert Lubos robert.lubos@nordicsemi.no