Rewrite sockets #579
Rewrite sockets #579
Conversation
boryspoplawski
force-pushed
the
borys/what_will_we_do_with_a_drunken_socket
branch
6 times, most recently
from
dacacda
to
93ab82e
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 55 files reviewed, 15 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
a discussion (no related file):
TODO: add fixes for all fixed issues
a discussion (no related file):
TODO: add an issue with leftovers for follow up PRs (though after main review is done)
a discussion (no related file):
TODO: rewrite PAL regression tests (at least SendHandle and add one for testing TCP + UDP communication)
LibOS/shim/src/bookkeep/shim_handle.c line 753 at r2 (raw file):
if (hdl->type == TYPE_SOCK) { /* TODO: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
No idea what to do with this one.
LibOS/shim/src/fs/socket/fs.c line 64 at r2 (raw file):
memset(stat, 0, sizeof(*stat)); /* TODO: what do we put in `dev` and `ino`? */
Is it important? I guess something might look at this info... Otoh it's a handle, not an inode, so why would it?
LibOS/shim/src/fs/socket/fs.c line 71 at r2 (raw file):
stat->st_blksize = PAGE_SIZE; /* TODO: maybe set `st_size` - query PAL for pending size? */
I wouldn't bother, if nothing uses that info.
LibOS/shim/src/net/ip.c line 289 at r2 (raw file):
} // TODO: maybe inline all functions used here? or at least join them into one - they contain
These function first query PAL, then change one option, then update stuff via PAL API.
So either:
A) we leave it as it is
B) create one function (maybe even inline it here) with a nested switch.
C) ?
LibOS/shim/src/net/ip.c line 370 at r2 (raw file):
} // TODO: maybe inline all functions used here? or at least join them into one - they contain
ditto
LibOS/shim/src/net/unix.c line 11 at r2 (raw file):
*/ // TODO: Pathname UNIX sockets are not visible on the filesystem. Is that a problem?
In this version UNIX socket do not create an entry in FS. I don't think we need it for anything, but I'm not sure. Only thing I could think of was that some apps might get confused when trying to unlink the socket after being done with it.
LibOS/shim/src/sys/shim_socket.c line 429 at r2 (raw file):
out: if (ret == -EINTR) { /* XXX: timeout could have been changed in the meantime. Do we care? Antyhing we can do
We cannot hold the lock protecting this field for the whole accept (especially since it might be blocking)
LibOS/shim/src/sys/shim_socket.c line 452 at r2 (raw file):
} /* TODO: non blocking connect should set `last_error` after actually connecting (or failing to)
Currently PAL APIs always block on connect, waiting for it to finish (just like the old version worked). How bad is that? The problem is I have no idea how to implement that in Gramine - this connect happens asynchronously outside of Gramine...
LibOS/shim/src/sys/shim_socket.c line 766 at r2 (raw file):
} return -ENOSYS; #if 0
Implementing MSG_PEEK turned out to be more cumbersome than expected, so I've left it missing for now. We need to check if anything real-life requires it.
Problems:
- UDP (SOCK_DGRAM) peeking - I'm not even sure what are the semantics where there are 2 datagrams...
- UDP - the host OS kernel will discard a datagram, even if we read only a part of it; if an app does a peek of 32 bytes, we read 32 bytes here and the actual packet was longer, it's now gone and we have no way to obtain the rest; maybe we could workaround this by always peeking >= MAX_DATAGRAM_SIZE, but uhhh...
- if a socket is blocking, we've already peeked some data and the app requests more, what do we do? if we always return the partial data, app might deadlock, maybe it awaits for more data doing peeks (I'm too afraid to even think about how this interacts with
EPOLLET); if we peek for more, well, there might not be more and we just blocked - we would have to add a per callis_blockingargument - that would be nice, but is that portable? to expect that from every PAL - concurrency - we would need all reads to be done under a lock - it's not doable otherwise (e.g. read + peek at the same time)
Pal/include/pal_internal.h line 39 at r2 (raw file):
extern struct pal_public_state g_pal_public_state; // TODO: clean unused stuff
For a follow up PR. TODO: add this to an issue summarizing leftovers from this PR.
Pal/include/pal_internal.h line 114 at r2 (raw file):
return NULL; if (handle->hdr.ops) { /* TODO: remove this hack or (preferably) add this for every type of handle. */
I think we should follow this approach everywhere and get rid of all the stringly-typed interfaces.
Pal/include/pal/pal.h line 36 at r2 (raw file):
#define URI_MAX 4096 /* TODO: dependency order is UGHMMMM */
This struct is used by LibOS, PAL and host specific PAL code, including pal_host.h which is included just right below.
This is a very weird place to put this struct - ideally it would go with rest of the socket related stuff.
Pal/src/host/Linux/db_sockets.c line 304 at r2 (raw file):
}; /* TODO: if this is used to change two fields and the second set fails, caller won't know about it
This is rather a corner case and LibOS never changes more than one field...
Pal/src/host/Linux/db_sockets.c line 306 at r2 (raw file):
/* TODO: if this is used to change two fields and the second set fails, caller won't know about it * do we care? */ /* TODO: this would need some locking, but LibOS provides it. Should we add redundant locks here? */
Currently LibOS never calls it concurrently on the same handle. Should we add redundant locks here, or just add this as a part of the interface (that caller must ensure proper locking)?
Pal/src/host/Linux-SGX/db_sockets.c line 77 at r2 (raw file):
handle->sock.recv_buf_size = __atomic_load_n(&g_default_recv_buf_size, __ATOMIC_RELAXED); if (!handle->sock.recv_buf_size) { /* TODO: this or just ignore this?
It would be good to have these, it wouldn't even be that slow (because we cache the values), but we would need to add getsockopt ocall.
|
Jenkins, retest this please (just testing, all was green) |
There was a problem hiding this comment.
Reviewable status: 0 of 55 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
TODO: add fixes for all fixed issues
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 55 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL)
LibOS/shim/src/sys/shim_socket.c line 766 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Implementing
MSG_PEEKturned out to be more cumbersome than expected, so I've left it missing for now. We need to check if anything real-life requires it.
Problems:
- UDP (SOCK_DGRAM) peeking - I'm not even sure what are the semantics where there are 2 datagrams...
- UDP - the host OS kernel will discard a datagram, even if we read only a part of it; if an app does a peek of 32 bytes, we read 32 bytes here and the actual packet was longer, it's now gone and we have no way to obtain the rest; maybe we could workaround this by always peeking >= MAX_DATAGRAM_SIZE, but uhhh...
- if a socket is blocking, we've already peeked some data and the app requests more, what do we do? if we always return the partial data, app might deadlock, maybe it awaits for more data doing peeks (I'm too afraid to even think about how this interacts with
EPOLLET); if we peek for more, well, there might not be more and we just blocked - we would have to add a per callis_blockingargument - that would be nice, but is that portable? to expect that from every PAL- concurrency - we would need all reads to be done under a lock - it's not doable otherwise (e.g. read + peek at the same time)
Ok, nginx uses MSG_PEEK on stream sockets in nonblocking mode, seems we need this now.
There was a problem hiding this comment.
Reviewable status: 0 of 60 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
TODO: rewrite PAL regression tests (at least
SendHandleand add one for testing TCP + UDP communication)
SendHandle rewritten. Do we want some more tests, or is this one enough (it also tests some simple communication, the only thing not checked is address retrieval).
boryspoplawski
force-pushed
the
borys/what_will_we_do_with_a_drunken_socket
branch
4 times, most recently
from
fdedaef
to
1cef6fe
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 60 files reviewed, 17 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski)
LibOS/shim/src/sys/shim_socket.c line 589 at r3 (raw file):
return -ENOTSOCK; } if (!WITHIN_MASK(flags, MSG_NOSIGNAL)) {
The checking is incorrect. It means only MSG_NOSIGNAL can be supported. All other flags are not supported.
LibOS/shim/src/sys/shim_socket.c line 742 at r3 (raw file):
return -ENOTSOCK; } if (!WITHIN_MASK(flags, MSG_PEEK)) {
The checking is incorrect. It means only MSG_PEEK is supported. All other flags are not supported.
LibOS/shim/test/regression/test_libos.py line 1202 at r3 (raw file):
os.remove("u") stdout, _ = self.run_binary(['unix'])
unix test case exit with error.
[P2:T2:unix] warning: error clearing POSIX locks: -3
[P2:T2:unix] error: Sending IPC process-exit notification failed: -104
[P2:T2:unix] warning: IPC pid release failed
debug: DkProcessExit: Returning exit code 1
There was a problem hiding this comment.
Reviewable status: 0 of 60 files reviewed, 17 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @llly)
LibOS/shim/src/sys/shim_socket.c line 589 at r3 (raw file):
Previously, llly (Li Xun) wrote…
The checking is incorrect. It means only
MSG_NOSIGNALcan be supported. All other flags are not supported.
It is correct, only this flag is supported.
LibOS/shim/src/sys/shim_socket.c line 742 at r3 (raw file):
Previously, llly (Li Xun) wrote…
The checking is incorrect. It means only
MSG_PEEKis supported. All other flags are not supported.
It is correct, only this flag is supported.
There will also be MSG_DONTWAIT, but I'm waiting for CI to be fixed before I submit a patch with it.
LibOS/shim/test/regression/test_libos.py line 1202 at r3 (raw file):
Previously, llly (Li Xun) wrote…
unix test case exit with error.
[P2:T2:unix] warning: error clearing POSIX locks: -3 [P2:T2:unix] error: Sending IPC process-exit notification failed: -104 [P2:T2:unix] warning: IPC pid release failed debug: DkProcessExit: Returning exit code 1
It works for me and it works in CI. Any more info? The log you've pasted dosn't help much.
There was a problem hiding this comment.
Reviewable status: 61 of 64 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
a discussion (no related file):
@mythi From the log you've pasted seems that the code is setting SO_REUSEADDR on a UNIX socket - this is meaningless, that option is completely ignored there. Could you modify the code not to set it?
There is a slight discrepancy between Gramine and Linux - we fail, Linux ignores it.
LibOS/shim/src/fs/socket/fs.c line 107 at r8 (raw file):
Previously, oshogbo (Mariusz Zaborski) wrote…
return pal_to_unix_errno(ret);
Sorry.
I'm suggesting something like:ret = 0; if (attr.nonblocking != nonblocking) { attr.nonblocking = nonblocking; ret = DkStreamAttributesSetByHandle(pal_handle, &attr); } return pal_to_unix_errno(ret);;The pal_to_unix_errno(0) return a 0.
Done.
There was a problem hiding this comment.
Reviewed 1 of 3 files at r20, 3 of 3 files at r21, 1 of 1 files at r22, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, @mythi, and @oshogbo)
a discussion (no related file):
There is a slight discrepancy between Gramine and Linux - we fail, Linux ignores it.
Could you remind why can't we follow Linux for this? Is it because we have some generic logic in Gramine, and we'll need to "circumvent" this logic to support this weird case?
LibOS/shim/src/sys/shim_socket.c line 351 at r18 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
The first version was named
read_shutdownto indicate that it's mostly modified byshutdown, but it's also used in some other cases (e.g. not connected TCP socket), so the name was changed.
You can think ofacceptas a read on a listening socket :) Well, that's what it is, actually. I dont likecan_be_read_or_accepted, it gives the impression that the socket can be both read and accepted on
I'm with Borys on this, read is how people refer to accept() on a listening socket. Think of poll/select -- you can specify the read event on the listening socket. So I definitely prefer to keep the current short name can_be_read.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 13 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
a discussion (no related file):
Could you modify the code not to set it?
It's possible but not very straightforward. I will test if that helps anyway. But I think it's worth pointing out that this may not be something everybody could do...
There was a problem hiding this comment.
Reviewed 3 of 3 files at r21, 1 of 1 files at r22, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
a discussion (no related file):
Previously, mythi (Mikko Ylinen) wrote…
Could you modify the code not to set it?
It's possible but not very straightforward. I will test if that helps anyway. But I think it's worth pointing out that this may not be something everybody could do...
Yeah, we should ignore it if Linux does.
But I think Borys point was more like: "this looks like a bug in your app", just poorly worded ;) But I may be wrong.
LibOS/shim/src/net/ip.c line 432 at r15 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
That sounds like a rather arbitrary interpretation
Mayhaps ¯\(ツ)/¯
LibOS/shim/src/sys/shim_socket.c line 351 at r18 (raw file):
readis how people refer toaccept()on a listening socket
Umm, in an object which has a recv method I definitely interpret can_be_read as "can I call .recv() on this". But I won't block on this if you both prefer the current version. Just saying that this was super confusing for me when reviewing this PR.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Yeah, we should ignore it if Linux does.
But I think Borys point was more like: "this looks like a bug in your app", just poorly worded ;) But I may be wrong.
FWIW, this is my code (CGO_ENABLED=0 go build ./main.go)
package main
import (
"log"
"net"
)
func main() {
l, err := net.Listen("unix", "/tmp/foobar.sock")
if err != nil {
log.Fatal(err)
}
defer l.Close()
}
There was a problem hiding this comment.
Reviewable status: 60 of 64 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
a discussion (no related file):
But I think Borys point was more like: "this looks like a bug in your app", just poorly worded ;) But I may be wrong.
Well, not sure if a bug, but a useless thing to do.
@mythi So this is just go net package doing this - nothing you can change
Could you remind why can't we follow Linux for this?
@dimakuv we can, just not sure how easy it will be with the current design (hopefully easy) - I kind of expected SOL_SOCKET options to be "backend" (socket type) agnostic, but well, they arent.
a discussion (no related file):
TODO (add to list): create a private structure in net/ip.c caching socket options similar to reuseaddr in generic socket handle. Maybe also add void* private to generic socket handle for storing this info.
LibOS/shim/src/sys/shim_socket.c line 351 at r18 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
readis how people refer toaccept()on a listening socketUmm, in an object which has a
recvmethod I definitely interpretcan_be_readas "can I call .recv() on this". But I won't block on this if you both prefer the current version. Just saying that this was super confusing for me when reviewing this PR.
I just don't like can_be_read_or_accepted, to me it's more confusing than explanatory.
I'll try to add some comments in the header, maybe that will help.
There was a problem hiding this comment.
Reviewable status: 60 of 64 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
But I think Borys point was more like: "this looks like a bug in your app", just poorly worded ;) But I may be wrong.
Well, not sure if a bug, but a useless thing to do.
@mythi So this is just go net package doing this - nothing you can change
Could you remind why can't we follow Linux for this?
@dimakuv we can, just not sure how easy it will be with the current design (hopefully easy) - I kind of expected
SOL_SOCKEToptions to be "backend" (socket type) agnostic, but well, they arent.
Ok, added support fro this. @mythi could you check if it works for you?
There was a problem hiding this comment.
Reviewable status: 60 of 64 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
a discussion (no related file):
@mythi So this is just go net package doing this - nothing you can change
It is possible to specify a custom "ListenConfig" and I was going to give it a try (just to confirm it works). Perhaps my code/use-case is special (and the ListenConfig approach would be preferred) since no-one has not seen it before. Anyway, I can confirm your latest commit makes this to work too (thanks!).
There was a problem hiding this comment.
Reviewed 4 of 4 files at r23, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @mkow, and @oshogbo)
LibOS/shim/src/net/ip.c line 406 at r23 (raw file):
switch (level) { case IPPROTO_IP:
It's a bit weird that you have this assymetry now in set and get functions (former has SOL_SOCKET handling, latter doesn't). But I understand that we don't need any special handling for gets -- we already cached the value in sock.reuseaddr so we can do it in the generic function in shim_socket.c.
Anyway, I don't see how to make it less confusing (like adding a comment). I think it is understandable from reading the code.
LibOS/shim/src/net/unix.c line 311 at r15 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto
So what is the conclusion on this?
LibOS/shim/src/net/unix.c line 288 at r23 (raw file):
return -EINVAL; } memcpy(&val, optval, sizeof(val));
Can we have uniformity with the func in ip.c? That one has a nice structure, with
/* All currently supported options use `int`. */
...
Please reformat as there, with break and setting reuseaddr = !!val right before return 0.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r23, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @oshogbo)
LibOS/shim/src/net/unix.c line 311 at r15 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
So what is the conclusion on this?
Borys changed all other places, but I think he missed this one?
LibOS/shim/src/sys/shim_socket.c line 351 at r18 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
I just don't like
can_be_read_or_accepted, to me it's more confusing than explanatory.
I'll try to add some comments in the header, maybe that will help.
Ok, it's a bit better now.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)
a discussion (no related file):
Anyway, I can confirm your latest commit makes this to work too (thanks!).
one update after I continued testing: the socket I create does not show up on the host (/tmp). The fs.mounts is correct because normal directories created by the same app show up OK.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dimakuv)
a discussion (no related file):
Previously, mythi (Mikko Ylinen) wrote…
Anyway, I can confirm your latest commit makes this to work too (thanks!).
one update after I continued testing: the socket I create does not show up on the host (
/tmp). Thefs.mountsis correct because normal directories created by the same app show up OK.
That's a known limitation of the current code, see the comment at the top of LibOS/shim/src/net/unix.c.
Is this something critical to your app, or were you just testing everything out?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
That's a known limitation of the current code, see the comment at the top of
LibOS/shim/src/net/unix.c.
Is this something critical to your app, or were you just testing everything out?
Yes, as @mkow said it's not visible on host FS. That shouldn't be a problem though - two Gramine processes can connect to it and host processes wouldn't be able to connect to it anyway (because we have a seamless encryption of UNIX sockets).
I've checked ListenConfig before and it only had two options (keepalive and something I dont remember), none of them allowed changing reuseaddr behavior.
LibOS/shim/src/net/ip.c line 406 at r23 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
It's a bit weird that you have this assymetry now in
setandgetfunctions (former hasSOL_SOCKEThandling, latter doesn't). But I understand that we don't need any special handling forgets -- we already cached the value insock.reuseaddrso we can do it in the generic function inshim_socket.c.Anyway, I don't see how to make it less confusing (like adding a comment). I think it is understandable from reading the code.
Yeah, I didn't want to duplicate the code. It might change if we ever implement the TODO from top level comment (adding a private struct holding options, different for IP and UNIX sockets).
LibOS/shim/src/net/unix.c line 311 at r15 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Borys changed all other places, but I think he missed this one?
Done. This time I've grepped whole repo and changed all of them (2 were not introduced by this PR, but in files that should not introduce more conflicts, so I stashed them here).
LibOS/shim/src/net/unix.c line 288 at r23 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can we have uniformity with the func in
ip.c? That one has a nice structure, with/* All currently supported options use `int`. */ ...Please reformat as there, with
breakand settingreuseaddr = !!valright beforereturn 0.
Done. Partially. I do not understand the part: and setting `reuseaddr = !!val` right before return 0.. We have to set it inside switch, and then we either return 0 (so no break) or have break but then setting is not right before return.
There was a problem hiding this comment.
Reviewable status: 60 of 66 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)
a discussion (no related file):
host processes wouldn't be able to connect to it anyway (because we have a seamless encryption of UNIX sockets).
bummer. my use case was to run a gRPC server that takes client commands from the host. I need to digest my options. @boryspoplawski @mkow btw, thanks a lot for quick replies!
none of them allowed changing reuseaddr behavior.
Something like this is possible but I did not go back to check it with the previous behavior anymore:
setNoReuseAddr := func(network, address string, conn syscall.RawConn) error {
return conn.Control(func(fd uintptr) {
syscall.SetsockoptInt(int(fd), unix.SOL_SOCKET, unix.SO_REUSEADDR, 0)
})
}
lc := net.ListenConfig{
Control: setNoReuseAddr,
}
// Listen and serve
l, err := lc.Listen(context.Background(), "unix", os.Args[1])
There was a problem hiding this comment.
Reviewable status: 60 of 66 files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)
a discussion (no related file):
my use case was to run a gRPC server that takes client commands from the host.
Not sure what exactly you are doing, but are you sure that it's secure? But I don't know your exact usecase, so maybe, who knows. Anyway, it won't work, unless you can change it to use IP (v4 or v6) sockets, like TCP or UDP, but then please make sure sending malicious commands (host can easily do that) isn't an issue for you.
Something like this is possible but I did not go back to check it with the previous behavior anymore:
This wouldn't work, it just turns off SO_REUSEADDR. That option is a complete no-op on UNIX sockets and as such was not supported (before the recent changes).
There was a problem hiding this comment.
Reviewed 6 of 6 files at r24, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)
a discussion (no related file):
This wouldn't work, it just turns off SO_REUSEADDR. That option is a complete no-op on UNIX sockets and as such was not supported (before the recent changes).
Right, I came to the same conclusion myself. Looking at strace notice Go enables it first and then just allows me to disable ;-)
On my use-case, yes it's safe. I'll see what I can do with IP sockets. Thanks for all the comments!
There was a problem hiding this comment.
Reviewed 2 of 6 files at r24, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)
LibOS/shim/src/net/unix.c line 288 at r23 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Done. Partially. I do not understand the part:
and setting `reuseaddr = !!val` right before return 0.. We have to set it inside switch, and then we eitherreturn 0(so nobreak) or havebreakbut then setting is not right beforereturn.
Yes, my last part of the comment was nonsense. It's good now.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @mkow)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
TODO: add an issue with leftovers for follow up PRs (though after main review is done)
Done (#657).
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
TODO (add to list): create a private structure in
net/ip.ccaching socket options similar to reuseaddr in generic socket handle. Maybe also addvoid* privateto generic socket handle for storing this info.
Done (#657).
Pal/include/pal_internal.h line 39 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Just a reminder for you to do this.
Done (#657).
Pal/src/host/Linux-SGX/enclave_ocalls.h line 56 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@mkow At some point we had a vague idea of introducing a generic "batch of OCALLs" logic. But that idea went nowhere, and also this may be not possible to implement in a nice and generic way at all.
Anyway, yes, I agree that having such "batched OCALL" is not hard to review and may be beneficial for performance. Let's leave this discussion for now.
Done (#657).
Pal/src/host/Linux-SGX/enclave_ocalls.c line 1137 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Blocking, to add this to leftovers/todo list
Done (#657).
The old version was full of bugs and barely readable. After the inability to overcome another limitation, we decided it is hopeless to fix and augment the old version and a complete rewrite is needed. LibOS part now follows more modular approach. There's a generic part, implementing all necessary syscall interfaces and calling protocol specific callbacks, where necessary. Currently implemented domains are: IPv4 and IPv6 (TCP + UDP) and UNIX stream sockets (named and abstract). PAL interfaces around sockets were redesigned and split from other, stringly-typed interfaces - there are now dedicated, socket related PAL API functions. This new interface follows those of BSD sockets. This makes implementing certain functionalities a lot easier and should not be a limitation for any new PAL - it's quite flexible and most of OSes follow this design. Signed-off-by: Borys Popławski <[email protected]>
boryspoplawski
force-pushed
the
borys/what_will_we_do_with_a_drunken_socket
branch
from
0591ae1
to
c1283b7
Compare
There was a problem hiding this comment.
Reviewable status: 58 of 66 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv, @mkow, and @oshogbo)
Previously, boryspoplawski (Borys Popławski) wrote…
Well, art could be misunderstood, :)
Done.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
sockets
Done.
There was a problem hiding this comment.
Reviewed 8 of 8 files at r25, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv)
There was a problem hiding this comment.
Reviewed 1 of 8 files at r25, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
Description of the changes
Well, not much to describe here. The old version was removed and the new was written from scratch. Hopefully everything is verbose enough that it doen't require explanation (or is commented in code, when it does).
Suggested order of review: common -> LibOS common code -> LibOS protocol callbacks -> PAL Linux -> PAL SGX
fixes #87, fixes #164, fixes #261, fixes #262, fixes #322, fixes #326, fixes #458, fixes #500, fixes #538, fixes #556,
fixes gramineproject/graphene#674, fixes gramineproject/graphene#2046, fixes gramineproject/graphene#2335, fixes gramineproject/graphene#2391, fixes gramineproject/graphene#2393
This change is