From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Christian Eggers <ceggers@arri.de>
Cc: "David S . Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Deepa Dinamani <deepa.kernel@gmail.com>,
Christoph Hellwig <hch@lst.de>,
Network Development <netdev@vger.kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net v2 1/2] socket: fix option SO_TIMESTAMPING_NEW
Date: Mon, 12 Oct 2020 09:37:49 -0400 [thread overview]
Message-ID: <CA+FuTSfkBHtKqjppMmqudj9GwZBidSqvOP6WCzoxLGihqiz5Qw@mail.gmail.com> (raw)
In-Reply-To: <20201012093542.15504-1-ceggers@arri.de>
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
>
next prev parent reply other threads:[~2020-10-12 14:01 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-12 9:35 [PATCH net v2 1/2] socket: fix option SO_TIMESTAMPING_NEW Christian Eggers
2020-10-12 9:35 ` [PATCH net v2 2/2] socket: don't clear SOCK_TSTAMP_NEW when SO_TIMESTAMPNS is disabled Christian Eggers
2020-10-12 13:37 ` Willem de Bruijn [this message]
2020-10-13 5:10 ` [PATCH net v2 1/2] socket: fix option SO_TIMESTAMPING_NEW Deepa Dinamani
2020-10-14 0:22 ` Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CA+FuTSfkBHtKqjppMmqudj9GwZBidSqvOP6WCzoxLGihqiz5Qw@mail.gmail.com \
--to=willemdebruijn.kernel@gmail.com \
--cc=ceggers@arri.de \
--cc=davem@davemloft.net \
--cc=deepa.kernel@gmail.com \
--cc=hch@lst.de \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).