Skip to content
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

nixos/mptcp: multipath TCP module #59342

Draft
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@teto
Copy link
Contributor

commented Apr 12, 2019

Multipath TCP is an IETF extension of TCP that makes use of several TCP connections to speed up transfer/increase reliability/confidentiality. Applications don't have to be changed (yet they can use the new MPTCP API for even better performance), the kernel will use MPTCP when applicable and fall back to TCP otherwise.

I had this in my repo for a while but #59301 convinced me to propose it. It only works with networkmanager for now (you need to update routing tables depending on added/removed interfaces).

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

nixos/mptcp: multipath TCP module
Sane defaults for configuration of MPTCP kernel.
@memberbetty
Copy link
Contributor

left a comment

fi


if [ `grep "$DEVICE_IFACE" "$RT_TABLE" | wc -l` -eq 0 ]; then

This comment has been minimized.

Copy link
@memberbetty

memberbetty Apr 16, 2019

Contributor

grep -c?

fi

if [ "$DHCP4_IP_ADDRESS" ]; then
SUBNET=`echo $IP4_ADDRESS_0 | cut -d \ -f 1 | cut -d / -f 2`

This comment has been minimized.

Copy link
@memberbetty

memberbetty Apr 16, 2019

Contributor

Extract this code to a function with a meaningful name, because it's duplicated below.

echo "$NUM $DEVICE_IFACE" >> "$RT_TABLE"
fi

if [ "$DHCP4_IP_ADDRESS" ]; then

This comment has been minimized.

Copy link
@memberbetty

memberbetty Apr 16, 2019

Contributor

Did you forget an operator here?

}

(mkIf (!config.networking.networkmanager.enable) {
warnings = [ "You have `networkmanager` disabled. Expect things to break." ];

This comment has been minimized.

Copy link
@memberbetty

memberbetty Apr 16, 2019

Contributor

Spelling (NetworkManager). Why is there only support for NetworkManager?

This comment has been minimized.

Copy link
@teto

teto Apr 17, 2019

Author Contributor

because that's the only thing I use.

This comment has been minimized.

Copy link
@memberbetty

memberbetty Apr 17, 2019

Contributor

I don't use NetworkManager, but I might switch because of this change.

Is there a reason to not enable this by default for everyone?

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 17, 2019

Contributor

Well, given the dependency on the dispatcher script this should be a failure rather a warning.


RT_TABLE=/etc/iproute2/rt_tables

if [ "$DEVICE_IFACE" = "lo" ]; then

This comment has been minimized.

Copy link
@memberbetty

memberbetty Apr 16, 2019

Contributor

Redundant quotes around lo.

This comment has been minimized.

Copy link
@teto

teto Apr 17, 2019

Author Contributor

Thanks for the comments, I can definitely improve the hook (copied from the mptcp wiki) and will do but it should already allow you to get started with mptcp (which is why I hurried with the PR :p ).

@memberbetty
Copy link
Contributor

left a comment

Interesting PR. I made some minor comments.


# TODO remove
echo $PATH
env > /tmp/if_up_env

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 17, 2019

Contributor

Insecure use of temporary files. /tmp/if_up_env could be an attacker controlled symlink.

This comment has been minimized.

Copy link
@memberbetty

memberbetty Apr 17, 2019

Contributor

He said in the comment that he is going to remove that.

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 17, 2019

Contributor

I still added the comment so it won't be forgotten.

if [ `grep "$DEVICE_IFACE" "$RT_TABLE" | wc -l` -eq 0 ]; then
logger -p user.info -t mptcp "Adding new routing table $DEVICE_IFACE"
NUM=$(wc -l < "$RT_TABLE")
echo "$NUM $DEVICE_IFACE" >> "$RT_TABLE"

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 17, 2019

Contributor

Why do we need this to be created at runtime rather then controlling the whole etc/iproute2/rt_tables file?

This comment has been minimized.

Copy link
@teto

teto Apr 17, 2019

Author Contributor

One big motivation for these multipath solutions (multipath QUIC/MPTCP/SCTP) is to deal with mobility scenarios so you need to handle dynamic usecases.

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 17, 2019

Contributor

By default /etc/iproute2/rt_tables does not exists, which would make:

NUM=$(wc -l < /etc/iproute2/rt_tables)
zsh: no such file or directory: /etc/iproute2/rt_tables

fail. If it is an empty file it would also collide with the local table (number 0)

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 17, 2019

Contributor

Is it needed at all to maintain rt_table for this to work?

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 17, 2019

Contributor

might be fixed by networking.iproute2.enable = true; so.

ip rule add from "$DHCP4_IP_ADDRESS" table "$DEVICE_IFACE"
else
# PPP-interface
IPADDR=`echo $IP4_ADDRESS_0 | cut -d \ -f 1 | cut -d / -f 1`

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 17, 2019

Contributor

What about ipv6?

This comment has been minimized.

Copy link
@teto

teto Apr 17, 2019

Author Contributor

what's IPv6 :D ? I haven't tested with IPv6 (haven't done much IPv6 in general). I can try but if someone wanna step up before or after the PR, I would be really happy.

fi

if [ -z "$DEVICE_IFACE" ]; then
logger -p user.warn -t mptcp "invalid $DEVICE_IFACE"

This comment has been minimized.

Copy link
@Mic92

Mic92 Apr 17, 2019

Contributor

Is logging to stdout/stderr not better so it would end up in the journald log of network manager?

This comment has been minimized.

Copy link
@teto

teto Apr 17, 2019

Author Contributor

logger also ends up in journald. I used it because it was in the base example and seemed good enough: like you can set debug levels and a keyword to grep to ("mptcp" here). Now that I am looking to upstream, stdout/stderr may be better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.