| Message ID | 20201012093542.15504-1-ceggers@arri.de |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2,1/2] socket: fix option SO_TIMESTAMPING_NEW | expand |
On Mon, Oct 12, 2020 at 5:36 AM Christian Eggers <ceggers@arri.de> 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 <deepa.kernel@gmail.com> 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: > > This suggested fix still sets and clears the flag if calling > > SO_TIMESTAMPING_NEW to disable timestamping. > where is it cleared? > > > 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 clearer > > > 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. > 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 >
> On Mon, Oct 12, 2020 at 5:36 AM Christian Eggers <ceggers@arri.de> wrote: > > v2: > > ----- > > - integrated proposal from Willem de Bruijn > > - added Reviewed-by: from Willem and Deepa You may add my Acked-by: Deepa Dinamani <deepa.kernel@gmail.com> -Deepa
On Mon, 12 Oct 2020 22:10:31 -0700 Deepa Dinamani wrote: > > On Mon, Oct 12, 2020 at 5:36 AM Christian Eggers <ceggers@arri.de> wrote: > > > v2: > > > ----- > > > - integrated proposal from Willem de Bruijn > > > - added Reviewed-by: from Willem and Deepa > > You may add my > Acked-by: Deepa Dinamani <deepa.kernel@gmail.com> Applied both, thanks everyone!
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:
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 <deepa.kernel@gmail.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: > This suggested fix still sets and clears the flag if calling > SO_TIMESTAMPING_NEW to disable timestamping. where is it cleared? > 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 clearer > 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. net/core/sock.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)