netdev - Re: [PATCH net-next v2 0/2] timestamp: control SOF_TIMESTAMPING_RX

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAL+tcoD6s0rrCAvMeMDE3-QVemPy21Onh4mHC+9PE-DDLkdj-Q@mail.gmail.com>
Date: Thu, 29 Aug 2024 23:27:31 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, 
	pabeni@...hat.com, dsahern@...nel.org, willemb@...gle.com, 
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v2 0/2] timestamp: control SOF_TIMESTAMPING_RX_SOFTWARE
 feature per socket

On Thu, Aug 29, 2024 at 10:14 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > From: Jason Xing <kernelxing@...cent.com>
> >
> > Prior to this series, when one socket is set SOF_TIMESTAMPING_RX_SOFTWARE
> > which measn the whole system turns on this button, other sockets that only
> > have SOF_TIMESTAMPING_SOFTWARE will be affected and then print the rx
> > timestamp information even without SOF_TIMESTAMPING_RX_SOFTWARE flag.
> > In such a case, the rxtimestamp.c selftest surely fails, please see
> > testcase 6.
> >
> > In a normal case, if we only set SOF_TIMESTAMPING_SOFTWARE flag, we
> > can't get the rx timestamp because there is no path leading to turn on
> > netstamp_needed_key button in net_enable_timestamp(). That is to say, if
> > the user only sets SOF_TIMESTAMPING_SOFTWARE, we don't expect we are
> > able to fetch the timestamp from the skb.
>
> I already happened to stumble upon a counterexample.
>
> The below code requests software timestamps, but does not set the
> generate flag. I suspect because they assume a PTP daemon (sfptpd)
> running that has already enabled that.

To be honest, I took a quick search through the whole onload program
and then suspected the use of timestamp looks really weird.

1. I searched the SOF_TIMESTAMPING_RX_SOFTWARE flag and found there is
no other related place that actually uses it.
2. please also see the tx_timestamping.c file[1]. The author similarly
only turns on SOF_TIMESTAMPING_SOFTWARE report flag without turning on
any useful generation flag we are familiar with, like
SOF_TIMESTAMPING_TX_SOFTWARE, SOF_TIMESTAMPING_TX_SCHED,
SOF_TIMESTAMPING_TX_ACK.

[1]: https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/tx_timestamping.c#L247

>
> https://github.com/Xilinx-CNS/onload/blob/master/src/tests/onload/hwtimestamping/rx_timestamping.c
>
> I suspect that there will be more of such examples in practice. In
> which case we should scuttle this. Please do a search online for
> SOF_TIMESTAMPING_SOFTWARE to scan for this pattern.

I feel that only the buggy program or some program particularly takes
advantage of the global netstamp_needed_key...

>
> > More than this, we can find there are some other ways to turn on
> > netstamp_needed_key, which will happenly allow users to get tstamp in
> > the receive path. Please see net_enable_timestamp().
> >
> > How to solve it?
> >
> > setsockopt interface is used to control each socket separately but in
> > this case it is affected by other sockets. For timestamp itself, it's
> > not feasible to convert netstamp_needed_key into a per-socket button
> > because when the receive stack just handling the skb from driver doesn't
> > know which socket the skb belongs to.
> >
> > According to the original design, we should not use both generation flag
> > (SOF_TIMESTAMPING_RX_SOFTWARE) and report flag (SOF_TIMESTAMPING_SOFTWARE)
> > together to test if the application is allowed to receive the timestamp
> > report in the receive path. But it doesn't hold for receive timestamping
> > case. We have to make an exception.
> >
> > So we have to test the generation flag when the applications do recvmsg:
> > if we set both of flags, it means we want the timestamp; if not, it means
> > we don't expect to see the timestamp even the skb carries.
> >
> > As we can see, this patch makes the SOF_TIMESTAMPING_RX_SOFTWARE under
> > setsockopt control. And it's a per-socket fine-grained now.
> >
> > v2
> > Link: https://lore.kernel.org/all/20240825152440.93054-1-kerneljasonxing@gmail.com/
> > Discussed with Willem
> > 1. update the documentation accordingly
> > 2. add more comments in each patch
> > 3. remove the previous test statements in __sock_recv_timestamp()
> >
> > Jason Xing (2):
> >   tcp: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket
> >   net: make SOF_TIMESTAMPING_RX_SOFTWARE feature per socket
> >
> >  Documentation/networking/timestamping.rst |  7 +++++++
> >  include/net/sock.h                        |  7 ++++---
> >  net/bluetooth/hci_sock.c                  |  4 ++--
> >  net/core/sock.c                           |  2 +-
> >  net/ipv4/ip_sockglue.c                    |  2 +-
> >  net/ipv4/ping.c                           |  2 +-
> >  net/ipv4/tcp.c                            | 11 +++++++++--
> >  net/ipv6/datagram.c                       |  4 ++--
> >  net/l2tp/l2tp_ip.c                        |  2 +-
> >  net/l2tp/l2tp_ip6.c                       |  2 +-
> >  net/nfc/llcp_sock.c                       |  2 +-
> >  net/rxrpc/recvmsg.c                       |  2 +-
> >  net/socket.c                              | 11 ++++++++---
> >  net/unix/af_unix.c                        |  2 +-
> >  14 files changed, 40 insertions(+), 20 deletions(-)
> >
> > --
> > 2.37.3
> >
>
>