| Message ID | 20201009103121.1004-1-ceggers@arri.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,1/2] socket: fix option SO_TIMESTAMPING_NEW | expand |
On Fri, Oct 9, 2020 at 6:32 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> > --- > net/core/sock.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/net/core/sock.c b/net/core/sock.c > index 34a8d12e38d7..3926804702c1 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1024,16 +1024,15 @@ int sock_setsockopt(struct socket *sock, int level, int optname, > } > > sk->sk_tsflags = val; > + if (optname != SO_TIMESTAMPING_NEW) > + sock_reset_flag(sk, SOCK_TSTAMP_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); > - The idea is to select new vs old behavior depending on which of the two setsockopts is called. This suggested fix still sets and clears the flag if calling SO_TIMESTAMPING_NEW to disable timestamping. 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); + if (val & SOF_TIMESTAMPING_OPT_ID && That is a subtle behavioral change, because it now leaves SOCK_TSTAMP_NEW set also when timestamping is turned off. But this is harmless, as in that case the versioning does not matter. A more pedantic version would be + sock_valbool_flag(sk, SOCK_TSTAMP_NEW, + optname == SO_TIMESTAMPING_NEW && + val & SOF_TIMESTAMPING_TX_RECORD_MASK);
On Fri, Oct 9, 2020 at 3:32 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. The SOCK_TSTAMP_NEW is reset only in the case when SOF_TIMESTAMPING_RX_SOFTWARE is not set. Note that we only call sock_enable_timestamp() at that time. Why would SOCK_TSTAMP_NEW be relevant otherwise? -Deepa
On Fri, Oct 9, 2020 at 8:30 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote: > > On Fri, Oct 9, 2020 at 3:32 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. > > The SOCK_TSTAMP_NEW is reset only in the case when > SOF_TIMESTAMPING_RX_SOFTWARE is not set. > Note that we only call sock_enable_timestamp() at that time. > > Why would SOCK_TSTAMP_NEW be relevant otherwise? Other timestamps can be configured, such as hardware timestamps. As the follow-on patch shows, there is also the issue of overlap between SO_TIMESTAMP(NS) and SO_TIMESTAMPING. Don't select OLD on timestamp disable, which may only disable some of the ongoing timestamping. Setting based on the syscall is simpler, too. __sock_set_timestamps already uses for SO_TIMESTAMP(NS) the valbool approach I suggest for SO_TIMESTAMPING. The fallthrough can also be removed. My rough patch missed that.
On Fri, Oct 9, 2020 at 5:43 PM Willem de Bruijn <willemb@google.com> wrote: > > On Fri, Oct 9, 2020 at 8:30 PM Deepa Dinamani <deepa.kernel@gmail.com> wrote: > > > > On Fri, Oct 9, 2020 at 3:32 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. > > > > The SOCK_TSTAMP_NEW is reset only in the case when > > SOF_TIMESTAMPING_RX_SOFTWARE is not set. > > Note that we only call sock_enable_timestamp() at that time. > > > > Why would SOCK_TSTAMP_NEW be relevant otherwise? > > Other timestamps can be configured, such as hardware timestamps. > > As the follow-on patch shows, there is also the issue of overlap > between SO_TIMESTAMP(NS) and SO_TIMESTAMPING. I see. Thanks for clarification. I think I had missed that you could have both software and hardware timestamps enabled at the same time. > Don't select OLD on timestamp disable, which may only disable > some of the ongoing timestamping. > > Setting based on the syscall is simpler, too. __sock_set_timestamps > already uses for SO_TIMESTAMP(NS) the valbool approach I > suggest for SO_TIMESTAMPING. > > The fallthrough can also be removed. My rough patch missed that. Sounds good. -Deepa
diff --git a/net/core/sock.c b/net/core/sock.c index 34a8d12e38d7..3926804702c1 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1024,16 +1024,15 @@ int sock_setsockopt(struct socket *sock, int level, int optname, } sk->sk_tsflags = val; + if (optname != SO_TIMESTAMPING_NEW) + sock_reset_flag(sk, SOCK_TSTAMP_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> --- net/core/sock.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)