[LibOS, PAL] Support corking flags in socket related syscalls

[llly]

Description of the changes

Gramine currently supports setsockopt(SOL_TCP, TCP_CORK/TCP_NODELAY) and doesn't support setsockopt(SOL_UDP, UDP_CORK) and send(MSG_MORE). These flags are inter-related to cork partial frames/segments or sent data as soon as possible.

Some applications use these flags and fails when they are not supported, such Pytorch. Source code can be seen in
https://github.com/pytorch/pytorch/blob/614d6f19e3d30cac0d77059e738d1f25d75eb408/torch/csrc/distributed/c10d/Utils.hpp#L559

This PR supports MSG_MORE flag in send/sendto/sendmsg syscalls by add a force_cork parameter to PalSocketSend interface and related functions.
Also support UDP_CORK flags in setsockopt and getsockopt syscalls.

Fixes #823

How to test this PR

updated regression test getsockopt

---

This change is 

[dimakuv]

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @llly)

a discussion (no related file):
In general, very good PR.

But please close this PR and re-create it as two PRs:

1. Support UDP_CORK flag.
2. Support MSG_MORE flag.

(The second PR can be created after the first PR is merged, or you can use the PR-to-PR functionality of GitHub.)

Also, please fix my high-level comments in the new PRs. See them in this review.

---

---

libos/test/regression/getsockopt.c line 12 at r1 (raw file):

#include <sys/socket.h>

int main(int argc, char** argv) {

Could you reformat this test a bit, so it's more modular? Like this:

static int test_tcp_getsockopt(void) {
    ... already existing code moved here ...
}

static int test_udp_getsockopt(void) {
    ... your new code moved here ...
}

int main(int argc, char** argv) {
    test_tcp_getsockopt();
    test_udp_getsockopt();
}

Also, please use the macro CHECK() instead of perror; return;. For inspiration, please see e.g. sigprocmask_pending.c test.

---

pal/src/host/linux/pal_host.h line 83 at r1 (raw file):

            bool tcp_nodelay;
            bool ipv6_v6only;
            bool udp_cork;

Why do you need to introduce a new field? You can just re-use the already-existing tcp_cork field, but just rename it to simply cork.

---

pal/src/host/linux-sgx/pal_host.h line 106 at r1 (raw file):

            bool tcp_nodelay;
            bool ipv6_v6only;
            bool udp_cork;

ditto

[llly]

Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @llly)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

In general, very good PR.

But please close this PR and re-create it as two PRs:

1. Support UDP_CORK flag.
2. Support MSG_MORE flag.

(The second PR can be created after the first PR is merged, or you can use the PR-to-PR functionality of GitHub.)

Also, please fix my high-level comments in the new PRs. See them in this review.

OK. I'll split the changes.

---

[boryspoplawski]

Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @llly)

a discussion (no related file):
Does anything even use UDP_CORK? Why did you add it? Actually does anything even use TCP_CORK? I'm not sure why we have it implemented.

---

a discussion (no related file):
The link you've posted uses MSG_MORE with TCP socket. What's the point of adding it for UDP too? I think ignoring this flag would be correct semantically for TCP and it probably would be even better performance-wise in SGX PAL (but for UDP we couldn't ignore it).

---

[llly]

Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @boryspoplawski and @dimakuv)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

Does anything even use UDP_CORK? Why did you add it? Actually does anything even use TCP_CORK? I'm not sure why we have it implemented.

I don't see a Gramine workload using UDP_CORK nor TCP_CORK. It's wired that already support TCP_CORK but not support UDP_CORK. Current networking framework can also support UDP_CORK just in case.

---

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

The link you've posted uses MSG_MORE with TCP socket. What's the point of adding it for UDP too? I think ignoring this flag would be correct semantically for TCP and it probably would be even better performance-wise in SGX PAL (but for UDP we couldn't ignore it).

Support MSG_MORE for only TCP socket can enable the case. Same reason as above, It's wired that already support MSG_MORE for TCP socket, but not support it for UDP socket.
Not sure about performance of TCP socket, unless we emulate the MSG_MORE behavior to reduce number of send ocall.

---

[boryspoplawski]

Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @llly)

a discussion (no related file):

Previously, llly (Li Xun) wrote…

I don't see a Gramine workload using UDP_CORK nor TCP_CORK. It's wired that already support TCP_CORK but not support UDP_CORK. Current networking framework can also support UDP_CORK just in case.

Sure, we can remove TCP_CORK then!

---

a discussion (no related file):
What's weird with that? We support stuff that is used by real world apps.

Not sure about performance of TCP socket, unless we emulate the MSG_MORE behavior to reduce number of send ocall.

Just passing through that flag doesn't make things better - in enclave it's better to copy the data and make one ocall than issue two and tell the kernel to wait with sending the first packed.
Emulating it fully in Gramine will be hard - kernel has some timeout after which it sends the packed anyway.

---

[dimakuv]

Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @boryspoplawski and @llly)

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

Sure, we can remove TCP_CORK then!

What's the point of removing TCP_CORK? Why not keep it? We don't know if there is any app that relies on this flag, and I see no good (security) reason to stop supporting this flag.

I agree that we don't need to add UDP_CORK just yet. Only when we find an application that actually requires it.

---

a discussion (no related file):

Previously, boryspoplawski (Borys Popławski) wrote…

What's weird with that? We support stuff that is used by real world apps.

Not sure about performance of TCP socket, unless we emulate the MSG_MORE behavior to reduce number of send ocall.

Just passing through that flag doesn't make things better - in enclave it's better to copy the data and make one ocall than issue two and tell the kernel to wait with sending the first packed.
Emulating it fully in Gramine will be hard - kernel has some timeout after which it sends the packed anyway.

I agree that we don't need to add MSG_MORE for UDP sockets. Only when we find an application that actually requires it.

I think I agree with @boryspoplawski that MSG_MORE for TCP sockets can be emulated by ignoring the flag. @llly Could you try this "dummy" solution on your app and see if it still behaves correctly and with decent performance?

---

[llly]

Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @boryspoplawski and @dimakuv)

a discussion (no related file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I agree that we don't need to add MSG_MORE for UDP sockets. Only when we find an application that actually requires it.

I think I agree with @boryspoplawski that MSG_MORE for TCP sockets can be emulated by ignoring the flag. @llly Could you try this "dummy" solution on your app and see if it still behaves correctly and with decent performance?

OK. I'll try ignoring the flag solution and create another PR.

---

[boryspoplawski]

Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @llly)

a discussion (no related file):

What's the point of removing TCP_CORK? Why not keep it?

It's unclear why it was added in the first place. I should have removed it while rewriting sockets, but I left it with wrong assumption that if we had something supported it was needed by some apps.

and I see no good (security) reason to stop supporting this flag.

This is one of those options that are blindly passed to host OS - the less we have of those the better

---

[dimakuv]

Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @boryspoplawski and @llly)

a discussion (no related file):

This is one of those options that are blindly passed to host OS - the less we have of those the better

Ok... I don't see problems with this approach (other than leaking a bit of meta information), but in general I agree.

---

[dimakuv]

This PR was superseded by #966. Closing.