Re: [PATCH net v2 1/2] socket: fix option SO_TIMESTAMPING_NEW
flat view
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2020-10-12 14:01:28
Also in:
lkml
On Mon, Oct 12, 2020 at 5:36 AM Christian Eggers [off-list ref] wrote:
The comparison of optname with SO_TIMESTAMPING_NEW is wrong way around,
so SOCK_TSTAMP_NEW will first be set and than reset again. Additionally
move it out of the test for SOF_TIMESTAMPING_RX_SOFTWARE as this seems
unrelated.
This problem happens on 32 bit platforms were the libc has already
switched to struct timespec64 (from SO_TIMExxx_OLD to SO_TIMExxx_NEW
socket options). ptp4l complains with "missing timestamp on transmitted
peer delay request" because the wrong format is received (and
discarded).
Fixes: 9718475e6908 ("socket: Add SO_TIMESTAMPING_NEW")
Signed-off-by: Christian Eggers <ceggers@arri.de>
Reviewed-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Reviewed-by: Deepa Dinamani <redacted>We have not yet reviewed this second patch. Please do not add such tags on behalf of other people. That said, I now have and do agree with the change, so Acked-by: Willem de Bruijn <willemb@google.com>
--- v2: ----- - integrated proposal from Willem de Bruijn - added Reviewed-by: from Willem and Deepa On Saturday, 10 October 2020, 02:23:10 CEST, Willem de Bruijn wrote:quoted
This suggested fix still sets and clears the flag if calling SO_TIMESTAMPING_NEW to disable timestamping.where is it cleared?quoted
Instead, how about case SO_TIMESTAMPING_NEW: - sock_set_flag(sk, SOCK_TSTAMP_NEW); fallthrough; case SO_TIMESTAMPING_OLD: [..] + sock_valbool_flag(sk, SOCK_TSTAMP_NEW, + optname == SO_TIMESTAMPING_NEW); +using you version looks clearerquoted
if (val & SOF_TIMESTAMPING_OPT_ID &&I would like to keep this below the "ret = -FOO; break" statements. IMHO the setsockopt() call should either completely fail or succeed.
Agreed.
quoted hunk ↗ jump to hunk
net/core/sock.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)diff --git a/net/core/sock.c b/net/core/sock.c index 34a8d12e38d7..669cf9b8bb44 100644 --- a/net/core/sock.c +++ b/net/core/sock.c@@ -994,8 +994,6 @@ int sock_setsockopt(struct socket *sock, int level, int optname, __sock_set_timestamps(sk, valbool, true, true); break; case SO_TIMESTAMPING_NEW: - sock_set_flag(sk, SOCK_TSTAMP_NEW); - fallthrough; case SO_TIMESTAMPING_OLD: if (val & ~SOF_TIMESTAMPING_MASK) { ret = -EINVAL;@@ -1024,16 +1022,14 @@ int sock_setsockopt(struct socket *sock, int level, int optname, } sk->sk_tsflags = val; + sock_valbool_flag(sk, SOCK_TSTAMP_NEW, optname == SO_TIMESTAMPING_NEW); + if (val & SOF_TIMESTAMPING_RX_SOFTWARE) sock_enable_timestamp(sk, SOCK_TIMESTAMPING_RX_SOFTWARE); - else { - if (optname == SO_TIMESTAMPING_NEW) - sock_reset_flag(sk, SOCK_TSTAMP_NEW); - + else sock_disable_timestamp(sk, (1UL << SOCK_TIMESTAMPING_RX_SOFTWARE)); - } break; case SO_RCVLOWAT: --Christian Eggers Embedded software developer Arnold & Richter Cine Technik GmbH & Co. Betriebs KG Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918 Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477 Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler