Re: [PATCH RFC] Support for SOL_IP for MPTCP setsockopt

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Wed, 20 Oct 2021, Poorva Sonparote wrote:

> Hello!
>
> I have been working on adding support for IP_TOS for setsockopt(). I
> am attaching the packetdrill test and the patch but it  still won't set the TOS value
> for the subflows that are created after the setsockopt() call.
>
> https://github.com/multipath-tcp/mptcp_net-next/issues/220#issuecomment-947863717
>
> Thanks,
> Poorva
>
>
>
>
> This patch adds support for IP_TOS for setsockopt(.. ,SOL_IP, ..)
> https://github.com/multipath-tcp/mptcp_net-next/issues/220
>
> Currently, any new subflows that are created after the setsockopt()
> call are not being updated with the set IP_TOS value.
>

Hi Poorva,

Thanks for the patch!


A 'Closes:' tag with the github issue link is also helpful here.

> Signed-off-by: Poorva Sonparote <psonparo@redhat.com>
> ---
> net/mptcp/sockopt.c | 39 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 38 insertions(+), 1 deletion(-)
>

If you have more commentary that isn't part of the commit message you want 
to be permanently stored in git, this space (after the '---' line above 
but before the 'diff --git ...' below) is a good for that! 'git am' will 
drop this text when applying. This is especially not a big deal with RFC 
patches like this one, since they don't end up getting merged anyway.


> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 8137cc3a4296..f5ee49660902 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -598,6 +598,40 @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t
> 	return ret;
> }
>
> +static int mptcp_setsockopt_sol_ip_set_tos(struct mptcp_sock *msk, int optname,
> +					   sockptr_t optval, unsigned int optlen)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct sock *sk = (struct sock *)msk;
> +	int ret, err;
> +	int val = 0;
> +
> +	err = ip_setsockopt(sk, SOL_IP, optname, optval, optlen);
> +	val = inet_sk(sk)->tos;
> +
> +	if (err != 0)
> +		return err;

Minor - need a blank line here.

> +	lock_sock(sk);
> +	mptcp_for_each_subflow(msk, subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +		ip_sock_set_tos(ssk, val);
> +	}
> +	release_sock(sk);

and here

> +	return ret;
> +}
> +
> +static int mptcp_setsockopt_sol_ip(struct mptcp_sock *msk, int optname,
> +				   sockptr_t optval, unsigned int optlen)
> +{
> +	switch (optname) {
> +	case IP_TOS:
> +		return  mptcp_setsockopt_sol_ip_set_tos(msk, optname, optval, optlen);
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
> 				    sockptr_t optval, unsigned int optlen)
> {
> @@ -637,12 +671,14 @@ int mptcp_setsockopt(struct sock *sk, int level, int optname,
> 	if (ssk)
> 		return tcp_setsockopt(ssk, level, optname, optval, optlen);
>
> +	if (level == SOL_IP)
> +		return mptcp_setsockopt_sol_ip(msk, optname, optval, optlen);
> +
> 	if (level == SOL_IPV6)
> 		return mptcp_setsockopt_v6(msk, optname, optval, optlen);
>
> 	if (level == SOL_TCP)
> 		return mptcp_setsockopt_sol_tcp(msk, optname, optval, optlen);
> -

Looks like an accidental line deletion here.

> 	return -EOPNOTSUPP;
> }
>
> @@ -1000,6 +1036,7 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
> 	ssk->sk_priority = sk->sk_priority;
> 	ssk->sk_bound_dev_if = sk->sk_bound_dev_if;
> 	ssk->sk_incoming_cpu = sk->sk_incoming_cpu;
> +	ip_sock_set_tos(ssk, inet_sk(sk)->tos);

I think this does sync the tos value to new subflows, right? Since the ssk 
is already locked, use __ip_sock_set_tos(). Maybe you ran in to a deadlock 
here that interfered with the syncing.

>
> 	if (sk->sk_userlocks & tx_rx_locks) {
> 		ssk->sk_userlocks |= sk->sk_userlocks & tx_rx_locks;
> -- 
> 2.18.2
>
>
>

--
Mat Martineau
Intel