Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201009103121.1004-1-ceggers@arri.de> <20201009103121.1004-2-ceggers@arri.de>
In-Reply-To: <20201009103121.1004-2-ceggers@arri.de>
From:   Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date:   Fri, 9 Oct 2020 20:34:40 -0400
Message-ID: <CA+FuTSfjZL-AMQy5PvVs6f3K8SEkWzdUrXz_4LniWFezVdfL8A@mail.gmail.com>
Subject: Re: [PATCH net 2/2] socket: don't clear SOCK_TSTAMP_NEW when
 SO_TIMESTAMPNS is disabled
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 9, 2020 at 6:32 AM Christian Eggers <ceggers@arri.de> wrote:
>
> SOCK_TSTAMP_NEW (timespec64 instead of timespec) is also used for
> hardware time stamps (configured via SO_TIMESTAMPING_NEW).
>
> User space (ptp4l) first configures hardware time stamping via
> SO_TIMESTAMPING_NEW which sets SOCK_TSTAMP_NEW. In the next step, ptp4l
> disables SO_TIMESTAMPNS(_NEW) (software time stamps), but this must not
> switch hardware time stamps back to "32 bit mode".
>
> 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: 887feae36aee ("socket: Add SO_TIMESTAMP[NS]_NEW")
> Fixes: 783da70e8396 ("net: add sock_enable_timestamps")
> Signed-off-by: Christian Eggers <ceggers@arri.de>

Acked-by: Willem de Bruijn <willemb@google.com>

Yes, we should just select SOCK_TSTAMP_NEW based on which of the two
syscall variants the process uses.

There is no need to reset on timestamp disable: in the common case the
selection is immaterial as timestamping is disabled.

As this commit message shows, with SO_TIMESTAMP(NS) and
SO_TIMESTAMPING that can be independently turned on and off, disabling
one can incorrectly switch modes while the other is still active.