Multicast node discovery for fastd #13
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1) Adds sourceaddr (packet source address for outgoing packets) and interval (discovery interval for multicast enabled sockets) to the corresponding bind address structure. 2) Implements the corresponding grammar for the configuration file. 3) Adds option to let the configuration resolve the actual address to bind to for explicit link-local IPv6 binds. 4) Adds sanity checks for the address setup in configuration.
…ddressing 1) Socket options that are set in src/socket.c are now explicitly selected for IPv4 or IPv6. IP_FREEBIND works - normally - for both socket families, but as the newer Linux and glibc versions also implement the specific IPV6_FREEBIND socket option, defer to this if available. 2) Socket options that are only relevant on an AF_INET/AF_INET6 socket are correspondingly only set anymore for the specific socket type. 3) Resolution of the IPv6 scope identifier in the actual socket bind is no longer necessary, as it's resolved in the configuration setup for the bind address anyway (normalize_address()).
1) Update socket binding function to handle IPv4/IPv6 multicast addresses by binding to the corresponding address family any address (otherwise, at least on Linux, you won't see the multicast packets on the socket), and setting up the corresponding multicast join for the socket. This does not need to be cleaned up, as the multicast bind is automatically left when the socket is closed. 2) Preferrably, use IPv6 sockets, even for IPv4 addresses. This means that IPv4 binds and also IPv4 any binds are always done on IPv6 sockets, with the corresponding option IPV6_ONLY not set and the address widened for the bind. As the underlying socket, even when it's only used for IPv4 traffic, supports all of the IPv6 socket functionality, means that IPV6_PKTINFO is also available, even when receiving only IPv4 traffic on the socket, so removes the reliance on IP_PKTINFO. This is relevant on some older Linux releases. 3) Multicast IPv4 sockets still require to be bound on an IPv4 socket, as they explicitly use socket options that are IPv4-socket only. This is the only class of addresses remaining which initializes an AF_INET socket explicitly. TODO: Especially 2) needs to be checked. Linux behaves correctly (i.e., when binding a widened IPv4 address on the IPv6 socket, the socket behaves just like an IPv4 only socket, except that all addresses are wide), and AFAIK this is also required behavior for the corresponding IPv6 sockets API. But, possibly, there are some other implementations around which do not properly map the IPv4 address space for IPv6 sockets. Needs to be checked.
…dress, not just bind. 1) bound_addr in the socket structure was a pointer to an allocated structure, which, as it's always set up anyway, simply not required to be a dynamic pointer. Change this to be a member structure of the socket structure. 2) Use the host part of sourceaddr if that is set up as the bound address for the socket, as this is the address that is supposed to be processed for unicast packets and also used as the source address for outgoing packets. This means that bound_addr does not explicitly represent the actual bind anymore in some places, but that's not required by any of the code that currently uses bound_addr.
…ets. 1) Move socket and peer address structures out to a separate header file which defines the corresponding structures. Add a task structure to the socket structure to represent the task pqueue element of the socket. 2) Implement a new task type (TASK_TYPE_SOCKET), which is dispatched to a corresponding function in socket.c for later implementing the actual muticast discovery packet send. This function currently just reschedules the task in the configured interval. 3) Initialize the socket task when creating the socket by checking whether the bind address has an interval set up (i.e., non-zero and not FASTD_TIMEOUT_INV), in which case the socket task is immediately started. When no interval set up (for non-multicast sockets) or for ephemeral sockets (for an individual peer), don't set up a socket task, as it is useless. 4) Make sure to remove the socket task from the task queue when a socket is closed.
1) Don't send out error results for handshakes that have been received on a multicast local address. 2) Use the bound address (which is the source address by a previous patch) to reply to multicast packets, or otherwise use the default socket bind address for packets received on a multicast socket. 3) Don't signal a connect event for multicast discovery packets. 4) Handle multicast discovery packets only for type 1 handshake packets (the actual discovery) and ignore multicast discovery from peers where we already have an established connection.
1) Remove copying of the in(6)_pktinfo structure from the cmsg headers on receive path. The data contents of the fastd_peer_address_t are filled from the fields directly pointed to in the CMSG_DATA section. 2) Remove copying of the in(6)_pktinfo structure to the cmsg headers on the send path. The underlying buffer is initialized to 1024 bytes of zeros in the send function, so zeroing the buffer contents is not required and the fields can be immediately referenced in the corresponding pktinfo helper. 3) Resize buffers on send and receive for ancilliary messages; the ancilliary messages that are received are a lot smaller than the currently allocated 1kb, so that stack usage (and zeroing in the send path) can be restricted to smaller sizes.
1) Retrying a send on a socket without the corresponding control message attached if required for choosing the local packet source works counter to having sourceaddr respected. In case sourceaddr is not set up, the local_addr in the send will have been NULLed or have sa_family as AF_UNSPEC, so that no ancilliary message is attached at all.
1) Socket receive functions which don't dispatch the buffer to a corresponding protocol handler need to free the data buffer on exit. This implements a common error: label on exit, which can be jumped to from any part of the function to ensure buffer cleanup. Additional tests which also drop the packet then simply need to jump to error to have proper handling.
1) Sanity check incoming packets for proper multicast packet type (only handshake possible) and for received address (when listening on the any address with an explicit source). 2) Sanity check PKTINFO data for IPv6 protocol sockets even if USE_PKTINFO is off, as IPV6_PKTINFO should always be on. 3) Don't simplify local address for the packet, due to the fact that sock->bound_addr is also not simplified but rather specified in the socket type address format.
1) Send out socket discovery packets (handshake type 1 messages without peer binding) from the socket task in intervals specified by sock->addr->interval.
1) The resolved LL6 address of an interface for automatic configuration of an interface may be tentative (privacy extensions, etc.) and/or have failed DAD. Make sure to evaluate the address flags at least on Linux where this is possible.
1) Fix an oversight while implementing the SO_FREEBIND6 support in src/socket.c where a corresponding configure test was defined, but the configure test never made it to the set of feature macros.
…esses 1) Partially revert a change that set up socket options only for IPPROTO_IPV6 on IPv6 connected sockets; some socket options apply to both protocols (independent of where they are set), some don't, so simply set all in case the socket is dual address family.
…file 1) Due to the fact that multicasting requires interface binding (and possibly other binding configurations also use interface binding), make sure to include config trigger support for OpenWRT configuration file to initiate a start/reload/reconnect on interface change for specific interfaces.
|
Thanks for updating the PR, I'll start reviewing this soon. |
|
Hi, thanks again for the PR, and sorry for the long delay. I have just started having a deeper look, but I'll add a few comments to the changes now. |
neocturne
reviewed
Feb 19, 2021
| if (bindtodev && !fastd_peer_address_is_v6_ll(address)) | ||
| exit_error("config error: device bind configuration not supported on this system"); | ||
| /** Normalization of a bind address, initializing the target address structure */ | ||
| static void normalize_address(fastd_peer_address_t *address, const fastd_peer_address_t *config, const char *bindtodev) { |
There was a problem hiding this comment.
The normalize_address() approach is problematic. Interface names must not be resolved to indices during configuration, as index changes should not require a restart of fastd.
neocturne
reviewed
Feb 19, 2021
| @@ -35,33 +35,31 @@ static inline void add_pktinfo(struct msghdr *msg, const fastd_peer_address_t *l | |||
|
|
|||
| #ifdef USE_PKTINFO | |||
| if (local_addr->sa.sa_family == AF_INET) { | |||
| struct in_pktinfo *pktinfo = (struct in_pktinfo *)CMSG_DATA(cmsg); | |||
There was a problem hiding this comment.
In many libc's (for example musl) cmsghr doesn't have a flexible member for the data and CMSG_DATA() is defined as follows:
#define CMSG_DATA(cmsg) ((unsigned char *) (((struct cmsghdr *)(cmsg)) + 1))
Casting this to struct in_pktinfo * is an aliasing violation and memcpy should be used.
neocturne
reviewed
Feb 19, 2021
| } | ||
| } | ||
|
|
||
| if (ret < 0) { |
There was a problem hiding this comment.
Removing this code block breaks fast source address switching without a full socket timeout and reestablish.
neocturne
reviewed
Feb 19, 2021
| goto error; | ||
| } | ||
| if (addr->bindtodev && !fastd_peer_address_is_v6_ll(&addr->addr) && | ||
| setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE, addr->bindtodev, strlen(addr->bindtodev))) { |
There was a problem hiding this comment.
In my opinion changing the nested if to a single condition that mixes condition and side-effects makes the code unclearer.
neocturne
reviewed
Feb 19, 2021
| pr_warn_errno("setsockopt: unable to set IP_FREEBIND"); | ||
| if (af != AF_INET && setsockopt(fd, IPPROTO_IPV6, IPV6_FREEBIND, &one, sizeof(one))) | ||
| pr_warn_errno("setsockopt: unable to set IPV6_FREEBIND"); | ||
| #else |
There was a problem hiding this comment.
What's the advantage of IPV6_FREEBIND? I can't find it in the documentation (man 7 ipv6 as of Linux man-pages 5.10), so I think it should not be used unless necessary.
neocturne
reviewed
Feb 19, 2021
|
|
||
| if (addr->addr.sa.sa_family != AF_INET) { | ||
| const int v6only = bind_address.sa.sa_family == AF_INET6 || addr->sourceaddr.sa.sa_family == AF_INET6; | ||
| if (!fastd_peer_address_is_v4_multicast(&bind_address)) { |
There was a problem hiding this comment.
What is the reason you change the logic to use PF_INET6 for IPv4 by default? The PF_INET fallback must work anyways for systems without IPv6 support, so I think it's a good idea to do that by default when IPv6 is not needed, so the PF_INET code paths receive more testing.
|
Generally, I think this PR tries to do too many things at once. Many commits mix coding style changes, cleanup with functional changes, and entirely new features, which makes the review very cumbersome. I understand that opportunities for cleanup often get noticed during the implementation of new features, but it would be of great benefit for my understanding of your changes if cleanups were committed separately. To keep the code size impact of the multicast feature low, as much code as possible should be To make sure my high-level understanding of your changes is correct, does the new feature basically consist of the following parts?
Another question, are you (or is someone you're aware of) actually using IPv4 multicast, or could this feature be limited to IPv6 multicast to keep the code simpler? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch series implements multicast node discovery (multicast handshakes) for fastd. Multicast node discovery allows for plug and play network setup of fastd instances on an ethernet backbone, without explicitly setting up IP addressing for peers. This PR forward-ports the patches in #11 for fastd-v21.
To enable multicast, use a bind statement of the form in the configuration:
bind [multicastaddr] port [port] [interface "[multicast interface]"] [source [unicast sourceaddr]] [interval [seconds]];
where interval can be zero to disable active multicast packets, and just passively listen for incoming multicast handshake packets. Further documentation on the implementation can be found in the patch requests.
Additionally, this PR changes the socket behaviour so that fastd always prefers IPv6 sockets with (possibly) IPv4-mapped addresses, even if binding to IPv4 only, except for the IPv4 multicast socket case, which remains on IPv4. This allows using IPV6_PKTINFO for source address retrieval for most use cases, even if IP_PKTINFO is unavailable. For the dual address family bind (two binds, for IPv4 and IPv6 respectively), this does not change the semantics.
Finally, by using getifaddrs(), the bind or source address can be specified as the token ll6, which is resolved to the first link-local IPv6 address that's retrieved from the specified bind interface.
This PR does not update the documentation with the new bind statement syntax as specified above.