Support multiple socket types in the network interface by stevenengler · Pull Request #1144 · shadow/shadow

stevenengler · 2021-03-04T15:39:10Z

This adds a CompatSocket struct containing a union of socket pointers and a type. The network interface code now works with these CompatSocket objects instead of just Socket objects so that we can support more socket types in the future. To prevent us from having to dynamically allocate memory for theseCompatSocket whenever we store them (which occurs whenever a plugin binds a socket or writes to a socket), we store a tagged pointer instead that replaces the low-order bits of the socket object pointer with a tag containing the type of socket object.

codecov · 2021-03-04T16:05:07Z

Codecov Report

Merging #1144 (697f3b8) into dev (0fe6231) will increase coverage by 0.27%.
The diff coverage is 58.78%.

@@            Coverage Diff             @@
##              dev    #1144      +/-   ##
==========================================
+ Coverage   56.62%   56.89%   +0.27%     
==========================================
  Files         137      140       +3     
  Lines       20067    20181     +114     
  Branches     4826     4889      +63     
==========================================
+ Hits        11362    11482     +120     
+ Misses       5731     5671      -60     
- Partials     2974     3028      +54

Flag	Coverage Δ
tests	`56.89% <58.78%> (+0.27%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/main/host/network_queuing_disciplines.c	`33.33% <33.33%> (ø)`
src/main/utility/tagged_ptr.c	`54.54% <54.54%> (ø)`
src/main/host/descriptor/compat_socket.c	`61.11% <61.11%> (ø)`
src/main/host/network_interface.c	`72.09% <75.86%> (+4.69%)`	⬆️
src/main/host/host.c	`67.57% <83.33%> (-0.46%)`	⬇️
src/main/host/descriptor/socket.c	`66.03% <100.00%> (+0.89%)`	⬆️
src/main/host/process.c	`71.50% <100.00%> (+0.68%)`	⬆️
src/main/host/syscall/socket.c	`80.02% <100.00%> (+0.11%)`	⬆️
src/main/routing/packet.c	`73.39% <100.00%> (+1.26%)`	⬆️
src/main/core/logger/shadow_logger.c	`78.71% <0.00%> (-0.50%)`	⬇️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fe6231...697f3b8. Read the comment docs.

sporksmith

Overall looks good!

sporksmith · 2021-03-05T17:07:22Z

src/main/host/descriptor/compat_socket.c

+    if (new_socket.type == CST_LEGACY_SOCKET) {
+        descriptor_ref(new_socket.object.as_legacy_socket);
+    } else {
+        error("Unexpected CompatSocket type");


Please wait to merge this until after #1148 is merged into dev. (It'll need a follow-up PR to merge master->dev)

I haven't rebased yet, but will get this functionality when I do.

sporksmith · 2021-03-05T17:13:27Z

src/main/host/descriptor/compat_socket.c

+        object.as_legacy_socket = object_ptr;
+    } else {
+        error("Unexpected socket pointer tag");
+        abort();


Throughout: can drop the explit abort-after-error after #1148

I can do this in some places, but when it's a function that has a return value, we sometimes need to abort, otherwise older compilers give a warning about not returning a value from a non-void function.

Ah, right. We could use the noreturn attribute, but I think we'd need to make error a function (or a macro invoking a noreturn wrapper function). Up to you.

The error() macro now calls abort at the end.

sporksmith · 2021-03-05T17:19:30Z

src/main/host/descriptor/compat_socket.h

+CompatSocket compatsocket_fromLegacySocket(Socket* socket);
+
+/* reference counting */
+CompatSocket compatsocket_cloneRef(const CompatSocket* socket);


Optional: maybe _refAs and _unref to more closely mirror _ref and _unref on other objects?

sporksmith · 2021-03-05T17:29:51Z

src/main/host/network_interface.c

    g_free(key);

-    if(!socket) {
+    if (!found) {


Can check for socket.type == CST_NONE here instead of tracking a separate flag (assuming you add one as suggested in the other comment)

sporksmith · 2021-03-05T17:34:36Z

src/main/host/descriptor/compat_socket.h

+#include "main/host/descriptor/socket.h"
+#include "main/utility/tagged_ptr.h"
+
+enum _CompatSocketTypes {


In general it's often useful to have the first enum value be something like '_NONE' or '_INVALID', to ensure a zero-initialized value can't be mistaken for a real value.

It'd be particularly useful for this case, since it'd let you directly represent a converted NULL pointer instead of having to track a separate valid/found flag.

Added a CST_NONE type.

sporksmith · 2021-03-05T17:40:28Z

src/main/host/host.c

    in_addr_t bindAddress;
-    socket_getSocketName(socket, &bindAddress, NULL);
+    if (socket->type == CST_LEGACY_SOCKET) {
+        if (!socket_isBound(socket->object.as_legacy_socket)) {


Should these be extracted out to compatsocket_isBound and compatsocket_getSocketName?

I was able to simplify this since we already have a compatsocket_getSocketName(), which already checks and returns false if the socket is bound.

sporksmith · 2021-03-05T17:43:16Z

src/main/host/host.c


    in_addr_t bindAddress;
-    socket_getSocketName(socket, &bindAddress, NULL);
+    if (socket->type == CST_LEGACY_SOCKET) {


Throughout - consider making these exhaustive switch statements. That way when a new enumerator is added, any such switch statements will become compiler warning (-> error with -werror) until they're updated.

Do be aware that it's a good idea to handle the case where the enum doesn't match any of the enumerator values, though that's unlikely to happen in practice since we're not e.g. deserializing these from some untrusted source.

More on this topic / Blatant self-promotion: https://abseil.io/tips/147

Done, and also added the -Wswitch flag so that we get warnings about this.

sporksmith · 2021-03-05T17:56:24Z

src/main/host/network_interface.c

-        Socket* socket = priorityqueue_pop(interface->fifoQueue);
-        packet = socket_pullOutPacket(socket);
-        *socketHandle = *descriptor_getHandleReference((LegacyDescriptor*)socket);
+        CompatSocket socket;


Throughout - generally a good idea to default-initialize stack-allocated structs as a precaution to avoid use of uninit'd values becoming a "heisenbugs".

Can be done succinctly as CompatSocket socket = {0};

sporksmith · 2021-03-05T18:03:20Z

src/main/utility/tagged_ptr.c

+#include "main/utility/utility.h"
+
+// two low-order bits
+const uintptr_t TAG_MASK = (1 << 2) - 1;


IIRC on x86_64, objects are 8-byte-aligned, so we could use one more bit. We shouldn't end up needing it for CompatSocket, but maybe worth doing now since this is a standalone utility module.

Definitely double-check first though, or if you don't want to spend time doing that just leave a TODO comment to consider verifying that and adding one more bit.

I see you have a utility_assert that should detect if I'm wrong, but I could imagine some objects being 8-byte-aligned in debug builds, but "packed" into 4-byte-alignment in release builds (where the assert is disabled).

Good point! I've added explicit error calls here. I've also added an extra bit to the tag. I originally didn't just because we didn't need it, but there should be no harm in making it 3 bits instead of 2.

sporksmith · 2021-03-12T13:51:58Z

src/main/host/descriptor/compat_socket.h

 };

 union _CompatSocketObject {
+    void* as_none;


Removed this.

sporksmith · 2021-03-12T13:56:25Z

src/main/host/network_interface.c

-        return false;
+        CompatSocket compatSocket = {
+            .type = CST_NONE,
+            .object.as_none = NULL,


Optional - Ok if you'd rather be explicit, but functionally not needed, since members not mentioned in a struct initializer are zero-initialized.

In particular though, being able to explicitly NULL initialize here probably isn't worth having the superfluous as_none in the first place (if that's why you added it). You could drop it and not mention .object here at all, or maybe explicitly init it as .object = {0}.

Okay I'm zero-initializing the struct now, but still setting the type explicitly to CST_NONE.

sporksmith · 2021-03-12T13:59:25Z

src/main/host/network_interface.c

@@ -498,7 +501,7 @@ static Packet* _networkinterface_selectRoundRobin(NetworkInterface* interface, g



Codecov says that this entire method, and a fair number of corner cases below, aren't covered by tests. Probably doesn't make sense to add them in this PR, but maybe worth opening an issue to increase coverage for this file?

(See comment below.)

sporksmith · 2021-03-12T14:05:39Z

src/main/host/network_queuing_disciplines.c

@@ -25,7 +25,7 @@ void rrsocketqueue_destroy(RrSocketQueue* self, void (*fn_processItem)(const Com

    if (fn_processItem != NULL) {


It looks like this is a new file, or did I miss it getting moved from somewhere else? Does this belong in this PR? It looks like it's untested...?

This is new code, and any of the round-robin qdisc methods aren't used in our tests. I've verified the round-robin qdisc manually by running phold with --interface-qdisc=RR and the tests pass. I've also now added another phold configuration test using this option.

The only thing weird to me in the coverage results is that _compareSocketTagged() isn't being run, which means there might be an issue with the priority queue. But I haven't made any changes to the priority queue, so I don't know what's going on there. I don't think we have any tests where a host has multiple sockets sending data at the same time, so I think it makes sense that _compareSocketTagged() is never being used since the network interface never has more than one socket in a queue at a time.

sporksmith · 2021-03-12T16:57:19Z

src/test/phold/CMakeLists.txt

+    CONFIGURATIONS ilibc)
+
+# Run tests with the round-robin queueing discipline (the current default is fifo).
+add_shadow_tests(


Optional: is there a more focused test to which we could add an RR configuration? phold is more of a stress/integration test than a unit test.

Ideally we'd want to use the RR configuration on a test which uses the network interface a lot, so one which reads and writes a lot of data, and one that sends data on many sockets at once per host. Phold is probably too simple of a test for this as it only writes to one socket at a time, but I figured it's easier for now to use phold rather than writing a new plugin that duplicates a lot of phold's functionality.

stevenengler force-pushed the compatsocket branch from b637204 to 37770a2 Compare March 4, 2021 15:54

stevenengler requested a review from sporksmith March 4, 2021 16:02

robgjansen assigned stevenengler Mar 4, 2021

robgjansen added this to the Enable alternative modular descriptor implementations milestone Mar 4, 2021

robgjansen added Component: Main Composing the core Shadow executable Type: Enhancement New functionality or improved design labels Mar 4, 2021

sporksmith requested changes Mar 5, 2021

View reviewed changes

stevenengler requested a review from sporksmith March 11, 2021 00:45

robgjansen mentioned this pull request Mar 12, 2021

Remove antiquated names #1172

Merged

sporksmith requested changes Mar 12, 2021

View reviewed changes

stevenengler requested a review from sporksmith March 12, 2021 15:54

sporksmith approved these changes Mar 12, 2021

View reviewed changes

stevenengler added 10 commits March 12, 2021 13:33

Moved socket desc port binding to outside of 'host_associateInterface()'

30f2e61

Added support for different socket types in the network interface

aaccf42

Added abort() to end of error() macro

f9e4b74

Enabled the Wreturn-type and Wswitch warnings

aa4b1b7

Small simplification to the netiface disassociate function

97c3b1e

Added a CST_NONE CompatSocket type

9ed97ea

Renamed 'compatsocket_cloneRef()' and 'compatsocket_drop()'

455fa69

Using 'switch' statements instead of 'if'

ab10cc7

Added extra bit to tagged pointers, and improved error handling

cd2b2e9

Test phold with the round-robin qdisc

697f3b8

stevenengler force-pushed the compatsocket branch from 6eff4af to 697f3b8 Compare March 12, 2021 19:11

stevenengler merged commit a36dbe2 into shadow:dev Mar 12, 2021

stevenengler deleted the compatsocket branch March 12, 2021 19:46

		@@ -498,7 +501,7 @@ static Packet* _networkinterface_selectRoundRobin(NetworkInterface* interface, g

		@@ -25,7 +25,7 @@ void rrsocketqueue_destroy(RrSocketQueue* self, void (*fn_processItem)(const Com

		if (fn_processItem != NULL) {

Conversation

stevenengler commented Mar 4, 2021

Uh oh!

codecov bot commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sporksmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenengler Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenengler Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

codecov bot commented Mar 4, 2021 •

edited

Loading

stevenengler Mar 12, 2021 •

edited

Loading

stevenengler Mar 12, 2021 •

edited

Loading