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

sockopt: uniform code to get/set values #353

Open
matttbe opened this issue Feb 10, 2023 · 5 comments
Open

sockopt: uniform code to get/set values #353

matttbe opened this issue Feb 10, 2023 · 5 comments
Assignees

Comments

@matttbe
Copy link
Member

matttbe commented Feb 10, 2023

When looking at net/mptcp/sockopt.c code, I can see multiple ways to set and get values. For example, for the setsockopt() side, we have:

IP_TRANSPARENT:

  • ip_setsockopt() is called on the MPTCP sock
  • the value is copied on the first subflow only
  • sync_socket_options() will set the same value to all the new subflows created after

IPV6_TRANSPARENT: the opposite

  • tcp_setsockopt() is called on the first subflow
  • the value is copied on the MPTCP sock
  • same behaviour for sync_socket_options()

IP_TOS: similar to IP_TRANSPARENT but on all subflows

  • ip_setsockopt() is called on the MPTCP sock
  • the value is copied to all subflows
  • same behaviour for sync_socket_options()

TCP_FASTOPEN:

  • tcp_setsockopt() is called on the first subflow
  • no sync
  • (we could set the value on MPTCP sock as anyway we do it)

TCP_INQ:

  • tcp_setsockopt() is not used but we do the same

SO_TIMESTAMPNS:

  • sock_setsockopt() is used
  • subflow fast lock is used to set on each subflow

etc.

@matttbe
Copy link
Member Author

matttbe commented Feb 10, 2023

Suggestion: probably most cases can be handled with such generic function:

static int mptcp_setsockopt_all_sf(struct mptcp_sock *msk, int level,
				   int optname, sockptr_t optval,
				   unsigned int optlen,
				   int (*get_val)(struct sock *),
				   void (*set_val)(struct sock *, int))
{
	struct mptcp_subflow_context *subflow;
	struct sock *sk = (struct sock *)msk;
	int err, val;

	err = tcp_setsockopt(sk, level, optname, optval, optlen);
	if (err != 0)
		return err;

	lock_sock(sk);
	sockopt_seq_inc(msk);
	val = get_val(sk);
	mptcp_for_each_subflow(msk, subflow) {
		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
		bool slow = lock_sock_fast(ssk); /* Not needed? */

		set_val(ssk, val);
		unlock_sock_fast(ssk, slow);
	}
	release_sock(sk);

	return 0;
}

Example:

static int get_ip_treansparent(struct sock *sk)
{
	return inet_sk(sk)->transparent;
}

static int set_ip_treansparent(struct sock *sk, int val)
{
	return inet_sk(sk)->transparent;
}

(...)

	switch (optname) {
	case IP_TRANSPARENT:
		return mptcp_setsockopt_all_sf(msk, level, optname, optval, optlen,
					        &get_ip_treansparent, &set_ip_treansparent);

(and add set_ip_treansparent() in sync_socket_options())

WDYT?

@funglee2k22
Copy link

Hey,

I hit some issue with setsockopt() with IP_TRANSPARENT (IPv4) when using MPTCPv1 (kernel version 5.15.0-69). It seems that IP_TRANSPARENT is not supported in 5.15.0-69. Should I upgrade my kernel ? do you have any recommendation on which version I should use ? Thanks.

@matttbe
Copy link
Member Author

matttbe commented Apr 11, 2023

Hi @funglee2k22,

You need at least the v5.17: https://github.com/multipath-tcp/mptcp_net-next/wiki#changelog
We recommend to use the last stable version (or at least the last LTS version).

when using MPTCPv1

MPTCPv1 is the protocol version, not the implementation.

https://github.com/multipath-tcp/mptcp_net-next/wiki#mptcpv0-vs-mptcpv1

@funglee2k22
Copy link

funglee2k22 commented Apr 13, 2023

@matttbe Thanks Matt, I upgrade to 6.1.23. and IP_TRANSPARENT starts working. Thanks.

Currently, I have some issues with a dual haproxy setup. Before I move to haproxy I had the shadowsock-libev solution working w/ mptcpv1. The similar setup (dual haproxy setup) was working fine with the mptcpv0.97 (4.* kernel). But now, not matter what I did, I could not get the dual-haproxy working (subflow is not added). (Note, both haproxy are running as a transparent proxy.).

Is there any recommendation how to debug this issue ?

@matttbe
Copy link
Member Author

matttbe commented Apr 14, 2023

@funglee2k22 I think it would be better if you open a new issue about that, that's not related to the factoring suggested here.

In the description of this new issue, please explain how you forced HAProxy to use MPTCP (with mptcpize?) + share output of ss -pinmHM and nstat -as Tcp* MPTcp*

matttbe added a commit to matttbe/mptcp_net-next that referenced this issue Jul 7, 2023
Currently, to get socket option, we deal with them case by case by
looking at the field that has been set and copying what is done
elsewhere in SOL_IP(V6). That's probably why there is only one being
supported here with IP_TOS.

Instead, we can use ip(v6)_getsockopt() to retrieve the value. By doing
that, we can also go through the BPF et Netlink hooks if needed and we
can easily support new options later.

While at it, also fix the variable declaration order to respect the
reverse xmas tree convention.

Link: multipath-tcp#353
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
matttbe added a commit to matttbe/mptcp_net-next that referenced this issue Jul 7, 2023
Currently, to set socket option, we mostly deal with them case by case
by setting the right field in the socket structure and in fact, copying
what is done elsewhere in SOL_IP(V6). Supporting a new option is then
more complex and we can see some disparities on how things are done
between the different options.

Instead, we can rely on ip(v6)_setsockopt() to set the different values.
By doing that, we uniform the way we deal with the different options and
we can also go through the BPF et Netlink hooks if needed. We can also
easily support new options later.

Note that there are still two cases where we still need to deal with the
end values directly, thus with something specific case by case:

- For SOL_TCP socket options because we cannot store them in the MPTCP
  socket. Note we can also store info in the different subflows (and
  create a first one if there is no subflow) and retrieve info from
  there.

- To sync the socket options when a new subflow has been created to
  minimise the work. But maybe it is fine to call some getsockopt() /
  setsockopt(). We could also save a list of setsockopt() that have been
  done in the past and only re-do these ones. That should help with the
  maintenance of these socket options and ease the support of new ones.

Link: multipath-tcp#353
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
matttbe added a commit to matttbe/mptcp_net-next that referenced this issue Jul 13, 2023
Currently, to get socket option, we deal with them case by case by
looking at the field that has been set and copying what is done
elsewhere in SOL_IP(V6). That's probably why there is only one being
supported here with IP_TOS.

Instead, we can use ip(v6)_getsockopt() to retrieve the value. By doing
that, we can also go through the BPF et Netlink hooks if needed and we
can easily support new options later.

While at it, also fix the variable declaration order to respect the
reverse xmas tree convention.

Link: multipath-tcp#353
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
matttbe added a commit to matttbe/mptcp_net-next that referenced this issue Jul 13, 2023
Currently, to set socket option, we mostly deal with them case by case
by setting the right field in the socket structure and in fact, copying
what is done elsewhere in SOL_IP(V6). Supporting a new option is then
more complex and we can see some disparities on how things are done
between the different options.

Instead, we can rely on ip(v6)_setsockopt() to set the different values.
By doing that, we uniform the way we deal with the different options and
we can also go through the BPF et Netlink hooks if needed. We can also
easily support new options later.

Link: multipath-tcp#353
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
matttbe added a commit to matttbe/mptcp_net-next that referenced this issue Jul 13, 2023
Currently, to set socket option, we mostly deal with them case by case
by setting the right field in the socket structure and in fact, copying
what is done elsewhere in SOL_IP(V6). Supporting a new option is then
more complex and we can see some disparities on how things are done
between the different options.

Instead, we can rely on ip(v6)_setsockopt() to set the different values.
By doing that, we uniform the way we deal with the different options and
we can also go through the BPF et Netlink hooks if needed. We can also
easily support new options later.

Link: multipath-tcp#353
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

No branches or pull requests

2 participants