| Message ID | 20250314-net-mptcp-fix-data-stream-corr-sockopt-v1-2-122dbb249db3@kernel.org (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | mptcp: fix data stream corruption and missing sockopts | expand |
On Fri, Mar 14, 2025 at 09:11:32PM +0100, Matthieu Baerts (NGI0) wrote: > When adding a socket option support in MPTCP, both the get and set parts > are supposed to be implemented. > > IPV6_V6ONLY support for the setsockopt part has been added a while ago, > but it looks like the get part got forgotten. It should have been > present as a way to verify a setting has been set as expected, and not > to act differently from TCP or any other socket types. > > Not supporting this getsockopt(IPV6_V6ONLY) blocks some apps which want > to check the default value, before doing extra actions. On Linux, the > default value is 0, but this can be changed with the net.ipv6.bindv6only > sysctl knob. On Windows, it is set to 1 by default. So supporting the > get part, like for all other socket options, is important. > > Everything was in place to expose it, just the last step was missing. > Only new code is added to cover this specific getsockopt(), that seems > safe. > > Fixes: c9b95a135987 ("mptcp: support IPV6_V6ONLY setsockopt") > Cc: stable@vger.kernel.org > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/550 > Reviewed-by: Mat Martineau <martineau@kernel.org> > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> Hi Matthieu, all, TBH, I would lean towards this being net-next material rather than a fix for net. But that notwithstanding this looks good to me. Reviewed-by: Simon Horman <horms@kernel.org>
Hi Simon, Thank you for your review! On 19/03/2025 16:38, Simon Horman wrote: > On Fri, Mar 14, 2025 at 09:11:32PM +0100, Matthieu Baerts (NGI0) wrote: >> When adding a socket option support in MPTCP, both the get and set parts >> are supposed to be implemented. >> >> IPV6_V6ONLY support for the setsockopt part has been added a while ago, >> but it looks like the get part got forgotten. It should have been >> present as a way to verify a setting has been set as expected, and not >> to act differently from TCP or any other socket types. >> >> Not supporting this getsockopt(IPV6_V6ONLY) blocks some apps which want >> to check the default value, before doing extra actions. On Linux, the >> default value is 0, but this can be changed with the net.ipv6.bindv6only >> sysctl knob. On Windows, it is set to 1 by default. So supporting the >> get part, like for all other socket options, is important. >> >> Everything was in place to expose it, just the last step was missing. >> Only new code is added to cover this specific getsockopt(), that seems >> safe. >> >> Fixes: c9b95a135987 ("mptcp: support IPV6_V6ONLY setsockopt") >> Cc: stable@vger.kernel.org >> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/550 >> Reviewed-by: Mat Martineau <martineau@kernel.org> >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > Hi Matthieu, all, > > TBH, I would lean towards this being net-next material rather than a fix > for net. But that notwithstanding this looks good to me. I understand. This patch and the next one target "net" because, with MPTCP, we try to mimic TCP when interacting with the userspace. Not supporting "getsockopt(IPV6_V6ONLY)" breaks some legacy apps forced to use MPTCP instead of TCP. These apps apparently "strangely" check this "getsockopt(IPV6_V6ONLY)" before changing the behaviour with "setsockopt(IPV6_V6ONLY)" which is supported for a long time. The "get" part should have been added from the beginning, and I don't see this patch as a new feature. Because it simply sets an integer like most other "get" options, it seems better to target net and fix these apps ASAP rather than targeting net-next and delay this "safe" fix. If that's OK, I would then prefer if these patches are applied in "net". Or they can be applied in "net-next" if we can keep their "Cc: stable" and "Fixes" tags, but that looks strange. Cheers, Matt
On 3/19/25 5:26 PM, Matthieu Baerts wrote: > On 19/03/2025 16:38, Simon Horman wrote: >> On Fri, Mar 14, 2025 at 09:11:32PM +0100, Matthieu Baerts (NGI0) wrote: >>> When adding a socket option support in MPTCP, both the get and set parts >>> are supposed to be implemented. >>> >>> IPV6_V6ONLY support for the setsockopt part has been added a while ago, >>> but it looks like the get part got forgotten. It should have been >>> present as a way to verify a setting has been set as expected, and not >>> to act differently from TCP or any other socket types. >>> >>> Not supporting this getsockopt(IPV6_V6ONLY) blocks some apps which want >>> to check the default value, before doing extra actions. On Linux, the >>> default value is 0, but this can be changed with the net.ipv6.bindv6only >>> sysctl knob. On Windows, it is set to 1 by default. So supporting the >>> get part, like for all other socket options, is important. >>> >>> Everything was in place to expose it, just the last step was missing. >>> Only new code is added to cover this specific getsockopt(), that seems >>> safe. >>> >>> Fixes: c9b95a135987 ("mptcp: support IPV6_V6ONLY setsockopt") >>> Cc: stable@vger.kernel.org >>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/550 >>> Reviewed-by: Mat Martineau <martineau@kernel.org> >>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> >> >> Hi Matthieu, all, >> >> TBH, I would lean towards this being net-next material rather than a fix >> for net. But that notwithstanding this looks good to me. > I understand. This patch and the next one target "net" because, with > MPTCP, we try to mimic TCP when interacting with the userspace. > > Not supporting "getsockopt(IPV6_V6ONLY)" breaks some legacy apps forced > to use MPTCP instead of TCP. These apps apparently "strangely" check > this "getsockopt(IPV6_V6ONLY)" before changing the behaviour with > "setsockopt(IPV6_V6ONLY)" which is supported for a long time. The "get" > part should have been added from the beginning, and I don't see this > patch as a new feature. Because it simply sets an integer like most > other "get" options, it seems better to target net and fix these apps > ASAP rather than targeting net-next and delay this "safe" fix. > > If that's OK, I would then prefer if these patches are applied in "net". > Or they can be applied in "net-next" if we can keep their "Cc: stable" > and "Fixes" tags, but that looks strange. As per off-line discussion I'm going to apply only the first patch in this series to net, and leave the other for net-next. /P
On Wed, Mar 19, 2025 at 05:26:35PM +0100, Matthieu Baerts wrote: > Hi Simon, > > Thank you for your review! > > On 19/03/2025 16:38, Simon Horman wrote: > > On Fri, Mar 14, 2025 at 09:11:32PM +0100, Matthieu Baerts (NGI0) wrote: > >> When adding a socket option support in MPTCP, both the get and set parts > >> are supposed to be implemented. > >> > >> IPV6_V6ONLY support for the setsockopt part has been added a while ago, > >> but it looks like the get part got forgotten. It should have been > >> present as a way to verify a setting has been set as expected, and not > >> to act differently from TCP or any other socket types. > >> > >> Not supporting this getsockopt(IPV6_V6ONLY) blocks some apps which want > >> to check the default value, before doing extra actions. On Linux, the > >> default value is 0, but this can be changed with the net.ipv6.bindv6only > >> sysctl knob. On Windows, it is set to 1 by default. So supporting the > >> get part, like for all other socket options, is important. > >> > >> Everything was in place to expose it, just the last step was missing. > >> Only new code is added to cover this specific getsockopt(), that seems > >> safe. > >> > >> Fixes: c9b95a135987 ("mptcp: support IPV6_V6ONLY setsockopt") > >> Cc: stable@vger.kernel.org > >> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/550 > >> Reviewed-by: Mat Martineau <martineau@kernel.org> > >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > > > Hi Matthieu, all, > > > > TBH, I would lean towards this being net-next material rather than a fix > > for net. But that notwithstanding this looks good to me. > I understand. This patch and the next one target "net" because, with > MPTCP, we try to mimic TCP when interacting with the userspace. > > Not supporting "getsockopt(IPV6_V6ONLY)" breaks some legacy apps forced > to use MPTCP instead of TCP. These apps apparently "strangely" check > this "getsockopt(IPV6_V6ONLY)" before changing the behaviour with > "setsockopt(IPV6_V6ONLY)" which is supported for a long time. The "get" > part should have been added from the beginning, and I don't see this > patch as a new feature. Because it simply sets an integer like most > other "get" options, it seems better to target net and fix these apps > ASAP rather than targeting net-next and delay this "safe" fix. > > If that's OK, I would then prefer if these patches are applied in "net". > Or they can be applied in "net-next" if we can keep their "Cc: stable" > and "Fixes" tags, but that looks strange. Hi Matthieu, Thanks for your detailed explanation. With that in mind I agree that these seem appropriate for net.
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index 505445a9598fafa1cde6d8f4a1a8635c3e778051..4b99eb796855e4578d14df90f9d1cc3f1cd5b8c7 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -1430,6 +1430,20 @@ static int mptcp_getsockopt_v4(struct mptcp_sock *msk, int optname, return -EOPNOTSUPP; } +static int mptcp_getsockopt_v6(struct mptcp_sock *msk, int optname, + char __user *optval, int __user *optlen) +{ + struct sock *sk = (void *)msk; + + switch (optname) { + case IPV6_V6ONLY: + return mptcp_put_int_option(msk, optval, optlen, + sk->sk_ipv6only); + } + + return -EOPNOTSUPP; +} + static int mptcp_getsockopt_sol_mptcp(struct mptcp_sock *msk, int optname, char __user *optval, int __user *optlen) { @@ -1469,6 +1483,8 @@ int mptcp_getsockopt(struct sock *sk, int level, int optname, if (level == SOL_IP) return mptcp_getsockopt_v4(msk, optname, optval, option); + if (level == SOL_IPV6) + return mptcp_getsockopt_v6(msk, optname, optval, option); if (level == SOL_TCP) return mptcp_getsockopt_sol_tcp(msk, optname, optval, option); if (level == SOL_MPTCP)
When adding a socket option support in MPTCP, both the get and set parts are supposed to be implemented. IPV6_V6ONLY support for the setsockopt part has been added a while ago, but it looks like the get part got forgotten. It should have been present as a way to verify a setting has been set as expected, and not to act differently from TCP or any other socket types. Not supporting this getsockopt(IPV6_V6ONLY) blocks some apps which want to check the default value, before doing extra actions. On Linux, the default value is 0, but this can be changed with the net.ipv6.bindv6only sysctl knob. On Windows, it is set to 1 by default. So supporting the get part, like for all other socket options, is important. Everything was in place to expose it, just the last step was missing. Only new code is added to cover this specific getsockopt(), that seems safe. Fixes: c9b95a135987 ("mptcp: support IPV6_V6ONLY setsockopt") Cc: stable@vger.kernel.org Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/550 Reviewed-by: Mat Martineau <martineau@kernel.org> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- net/mptcp/sockopt.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)