| Message ID | 20240214195407.3175-1-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [v2,net-next] net: Deprecate SO_DEBUG and reclaim SOCK_DBG bit. | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 94 lines checked |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug__except_selftest_mptcp_join_ | warning | Unstable: 1 failed test(s): packetdrill_regressions |
| matttbe/KVM_Validation__debug__only_selftest_mptcp_join_ | success | Success! ✅ |
Hi Kuniyuki,
Thank you for your modifications, that's great!
Our CI (GitHub Action) did some validations and here is its report:
- KVM Validation: normal:
- Success! ✅:
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7906812299
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/a36a5e8538ed
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Kuniyuki,
Thank you for your modifications, that's great!
Our CI (Cirrus) did some validations with a debug kernel and here is its report:
- KVM Validation: debug (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5986065517903872
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5986065517903872/summary/summary.txt
- KVM Validation: debug (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5423115564482560
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5423115564482560/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/a36a5e8538ed
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-debug
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Kuniyuki, On 14/02/2024 20:54, Kuniyuki Iwashima wrote: > Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") > removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 ("net: > remove SOCK_DEBUG macro") removed the macro. > > Now is the time to deprecate the oldest socket option. > > Note that setsockopt(SO_DEBUG) is moved not to acquire lock_sock(). > > Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > --- > v2: > * Move setsockopt(SO_DEBUG) code not to acquire lock_sock(). Thank you for the v2! Good idea to have modified that in net/core/sock.c too! Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> I don't think we need to do anything else, but just to be sure: do we need to tell the userspace this socket option has been deprecated? SO_DEBUG is a bit particular, so I guess it is fine not to do anything else, but except by looking at the kernel version, I don't think the userspace can know it has no more effect. I didn't find many examples of other deprecated socket options, apart from SO_BSDCOMPAT. For years, there was a warning, removed a few years ago: f4ecc748533d ("net: Stop warning about SO_BSDCOMPAT usage"). I guess we don't want that for SO_DEBUG, right? Cheers, Matt
Hi Kuniyuki,
Thank you for your modifications, that's great!
Our CI (GitHub Action) did some validations and here is its report:
- KVM Validation: normal:
- Success! ✅:
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/7914279679
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/4b23a08aba91
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Kuniyuki, Thank you for your modifications, that's great! Our CI (Cirrus) did some validations with a debug kernel and here is its report: - KVM Validation: debug (except selftest_mptcp_join): - Unstable: 1 failed test(s): packetdrill_regressions
On 14.02.24 20:54, Kuniyuki Iwashima wrote: > + case SO_DEBUG: > + /* deprecated, but kept for compatibility */ > + if (val && !sockopt_capable(CAP_NET_ADMIN)) > + ret = -EACCES; > + return 0; Setting ret has no effect here. Maybe you mean something like: > + if (val && !sockopt_capable(CAP_NET_ADMIN)) > + return -EACCES; > + return 0; or return (val && !sockopt_capable(CAP_NET_ADMIN)) ? -EACCESS : 0;
On Wed, 14 Feb 2024 11:54:07 -0800 Kuniyuki Iwashima wrote: > Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") > removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 ("net: > remove SOCK_DEBUG macro") removed the macro. Unrelated to this patch but speaking of deprecating things - do you want to go ahead with deleting DCCP? I don't recall our exact plan, I thought it was supposed to happen early in the year :)
On Thu, 2024-02-15 at 07:07 -0800, Jakub Kicinski wrote: > On Wed, 14 Feb 2024 11:54:07 -0800 Kuniyuki Iwashima wrote: > > Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") > > removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 ("net: > > remove SOCK_DEBUG macro") removed the macro. > > Unrelated to this patch but speaking of deprecating things - do you > want to go ahead with deleting DCCP? I don't recall our exact plan, > I thought it was supposed to happen early in the year :) My personal "current year" counter tend to be outdated till at least May, but I *think* it's supposed to happen the next year: https://elixir.bootlin.com/linux/v6.8-rc4/source/net/dccp/proto.c#L193 :) /P
On Thu, 15 Feb 2024 17:05:58 +0100 Paolo Abeni wrote: > > Unrelated to this patch but speaking of deprecating things - do you > > want to go ahead with deleting DCCP? I don't recall our exact plan, > > I thought it was supposed to happen early in the year :) > > My personal "current year" counter tend to be outdated till at least > May, but I *think* it's supposed to happen the next year: > > https://elixir.bootlin.com/linux/v6.8-rc4/source/net/dccp/proto.c#L193 Apparently mine runs ahead :D Thank you!
From: Matthieu Baerts <matttbe@kernel.org> Date: Thu, 15 Feb 2024 10:58:58 +0100 > Hi Kuniyuki, > > On 14/02/2024 20:54, Kuniyuki Iwashima wrote: > > Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") > > removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 ("net: > > remove SOCK_DEBUG macro") removed the macro. > > > > Now is the time to deprecate the oldest socket option. > > > > Note that setsockopt(SO_DEBUG) is moved not to acquire lock_sock(). > > > > Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > > --- > > v2: > > * Move setsockopt(SO_DEBUG) code not to acquire lock_sock(). > > Thank you for the v2! > > Good idea to have modified that in net/core/sock.c too! > > Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > > I don't think we need to do anything else, but just to be sure: do we > need to tell the userspace this socket option has been deprecated? > SO_DEBUG is a bit particular, so I guess it is fine not to do anything > else, but except by looking at the kernel version, I don't think the > userspace can know it has no more effect. > > I didn't find many examples of other deprecated socket options, apart > from SO_BSDCOMPAT. For years, there was a warning, removed a few years > ago: f4ecc748533d ("net: Stop warning about SO_BSDCOMPAT usage"). I > guess we don't want that for SO_DEBUG, right? I was also wondering if I should add pr_info_once(), but it seems not worth adding. With a rough grep, $ git log -S SOCK_DEBUG net except for the legacy protocols (appletalk, dccp, x25) that 8e5443d2b866 touched recently, TCP was the last real user of SOCK_DEBUG() and it's 2019-02-25 (5 years ago!) that 9946b3410b61 removed the use. So, I think we don't need such warning. Thanks!
From: Alexandra Winter <wintera@linux.ibm.com> Date: Thu, 15 Feb 2024 15:14:21 +0100 > On 14.02.24 20:54, Kuniyuki Iwashima wrote: > > + case SO_DEBUG: > > + /* deprecated, but kept for compatibility */ > > + if (val && !sockopt_capable(CAP_NET_ADMIN)) > > + ret = -EACCES; > > + return 0; > > Setting ret has no effect here. Maybe you mean something like: > > + if (val && !sockopt_capable(CAP_NET_ADMIN)) > > + return -EACCES; > > + return 0; > > or > > return (val && !sockopt_capable(CAP_NET_ADMIN)) ? -EACCESS : 0; oops, thanks for catching! will fix in v3.
diff --git a/include/net/sock.h b/include/net/sock.h index a9d99a9c583f..e20d55a36f9c 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -909,7 +909,6 @@ enum sock_flags { SOCK_TIMESTAMP, SOCK_ZAPPED, SOCK_USE_WRITE_QUEUE, /* whether to call sk->sk_write_space in sock_wfree */ - SOCK_DBG, /* %SO_DEBUG setting */ SOCK_RCVTSTAMP, /* %SO_TIMESTAMP setting */ SOCK_RCVTSTAMPNS, /* %SO_TIMESTAMPNS setting */ SOCK_LOCALROUTE, /* route locally only, %SO_DONTROUTE setting */ diff --git a/net/core/sock.c b/net/core/sock.c index 88bf810394a5..c4c406f4742e 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1115,6 +1115,11 @@ int sk_setsockopt(struct sock *sk, int level, int optname, /* handle options which do not require locking the socket. */ switch (optname) { + case SO_DEBUG: + /* deprecated, but kept for compatibility */ + if (val && !sockopt_capable(CAP_NET_ADMIN)) + ret = -EACCES; + return 0; case SO_PRIORITY: if ((val >= 0 && val <= 6) || sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) || @@ -1193,12 +1198,6 @@ int sk_setsockopt(struct sock *sk, int level, int optname, sockopt_lock_sock(sk); switch (optname) { - case SO_DEBUG: - if (val && !sockopt_capable(CAP_NET_ADMIN)) - ret = -EACCES; - else - sock_valbool_flag(sk, SOCK_DBG, valbool); - break; case SO_REUSEADDR: sk->sk_reuse = (valbool ? SK_CAN_REUSE : SK_NO_REUSE); break; @@ -1619,7 +1618,8 @@ int sk_getsockopt(struct sock *sk, int level, int optname, switch (optname) { case SO_DEBUG: - v.val = sock_flag(sk, SOCK_DBG); + /* deprecated. */ + v.val = 0; break; case SO_DONTROUTE: diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c index da37e4541a5d..31d09009332a 100644 --- a/net/mptcp/sockopt.c +++ b/net/mptcp/sockopt.c @@ -80,9 +80,6 @@ static void mptcp_sol_socket_sync_intval(struct mptcp_sock *msk, int optname, in bool slow = lock_sock_fast(ssk); switch (optname) { - case SO_DEBUG: - sock_valbool_flag(ssk, SOCK_DBG, !!val); - break; case SO_KEEPALIVE: if (ssk->sk_prot->keepalive) ssk->sk_prot->keepalive(ssk, !!val); @@ -183,7 +180,6 @@ static int mptcp_setsockopt_sol_socket_int(struct mptcp_sock *msk, int optname, case SO_KEEPALIVE: mptcp_sol_socket_sync_intval(msk, optname, val); return 0; - case SO_DEBUG: case SO_MARK: case SO_PRIORITY: case SO_SNDBUF: @@ -329,7 +325,6 @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname, case SO_RCVBUFFORCE: case SO_MARK: case SO_INCOMING_CPU: - case SO_DEBUG: case SO_TIMESTAMP_OLD: case SO_TIMESTAMP_NEW: case SO_TIMESTAMPNS_OLD: @@ -363,6 +358,7 @@ static int mptcp_setsockopt_sol_socket(struct mptcp_sock *msk, int optname, case SO_WIFI_STATUS: case SO_NOFCS: case SO_SELECT_ERR_QUEUE: + case SO_DEBUG: /* deprecated */ return 0; } @@ -1458,8 +1454,6 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk) sk_dst_reset(ssk); } - sock_valbool_flag(ssk, SOCK_DBG, sock_flag(sk, SOCK_DBG)); - if (inet_csk(sk)->icsk_ca_ops != inet_csk(ssk)->icsk_ca_ops) tcp_set_congestion_control(ssk, msk->ca_name, false, true); __tcp_sock_set_cork(ssk, !!msk->cork); diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c index 66763c74ab76..062e16a2766a 100644 --- a/net/smc/af_smc.c +++ b/net/smc/af_smc.c @@ -445,7 +445,6 @@ static int smc_bind(struct socket *sock, struct sockaddr *uaddr, (1UL << SOCK_LINGER) | \ (1UL << SOCK_BROADCAST) | \ (1UL << SOCK_TIMESTAMP) | \ - (1UL << SOCK_DBG) | \ (1UL << SOCK_RCVTSTAMP) | \ (1UL << SOCK_RCVTSTAMPNS) | \ (1UL << SOCK_LOCALROUTE) | \ @@ -511,8 +510,8 @@ static void smc_copy_sock_settings_to_clc(struct smc_sock *smc) #define SK_FLAGS_CLC_TO_SMC ((1UL << SOCK_URGINLINE) | \ (1UL << SOCK_KEEPOPEN) | \ - (1UL << SOCK_LINGER) | \ - (1UL << SOCK_DBG)) + (1UL << SOCK_LINGER)) + /* copy only settings and flags relevant for smc from clc to smc socket */ static void smc_copy_sock_settings_to_smc(struct smc_sock *smc) {
Recently, commit 8e5443d2b866 ("net: remove SOCK_DEBUG leftovers") removed the last users of SOCK_DEBUG(), and commit b1dffcf0da22 ("net: remove SOCK_DEBUG macro") removed the macro. Now is the time to deprecate the oldest socket option. Note that setsockopt(SO_DEBUG) is moved not to acquire lock_sock(). Reviewed-by: Gerd Bayer <gbayer@linux.ibm.com> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> --- v2: * Move setsockopt(SO_DEBUG) code not to acquire lock_sock(). v1: https://lore.kernel.org/netdev/20240213223135.85957-1-kuniyu@amazon.com/ --- include/net/sock.h | 1 - net/core/sock.c | 14 +++++++------- net/mptcp/sockopt.c | 8 +------- net/smc/af_smc.c | 5 ++--- 4 files changed, 10 insertions(+), 18 deletions(-)