netdev - Re: [PATCH net-next v2 04/12] net-timestamp: add static key to control the whole bpf extension

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: <6718601526686_15cbc9294ef@willemb.c.googlers.com.notmuch>
Date: Tue, 22 Oct 2024 22:31:49 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 Martin KaFai Lau <martin.lau@...ux.dev>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
 Jason Xing <kerneljasonxing@...il.com>
Cc: davem@...emloft.net, 
 edumazet@...gle.com, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 dsahern@...nel.org, 
 willemb@...gle.com, 
 ast@...nel.org, 
 daniel@...earbox.net, 
 andrii@...nel.org, 
 eddyz87@...il.com, 
 song@...nel.org, 
 yonghong.song@...ux.dev, 
 john.fastabend@...il.com, 
 kpsingh@...nel.org, 
 sdf@...ichev.me, 
 haoluo@...gle.com, 
 jolsa@...nel.org, 
 bpf@...r.kernel.org, 
 netdev@...r.kernel.org, 
 Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v2 04/12] net-timestamp: add static key to
 control the whole bpf extension

Willem de Bruijn wrote:
> Martin KaFai Lau wrote:
> > On 10/20/24 2:51 PM, Willem de Bruijn wrote:
> > > Jason Xing wrote:
> > >> From: Jason Xing <kernelxing@...cent.com>
> > >>
> > >> Willem suggested that we use a static key to control. The advantage
> > >> is that we will not affect the existing applications at all if we
> > >> don't load BPF program.
> > >>
> > >> In this patch, except the static key, I also add one logic that is
> > >> used to test if the socket has enabled its tsflags in order to
> > >> support bpf logic to allow both cases to happen at the same time.
> > >> Or else, the skb carring related timestamp flag doesn't know which
> > >> way of printing is desirable.
> > >>
> > >> One thing important is this patch allows print from both applications
> > >> and bpf program at the same time. Now we have three kinds of print:
> > >> 1) only BPF program prints
> > >> 2) only application program prints
> > >> 3) both can print without side effect
> > >>
> > >> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > > 
> > > Getting back to this thread. It is long, instead of responding to
> > > multiple messages, let me combine them in a single response.
> > > 
> > > 
> > > * On future extensions:
> > > 
> > > +1 that the UDP case, and datagrams more broadly, must have a clear
> > > development path, before we can merge TCP.
> > > 
> > > Similarly, hardware timestamps need not be supported from the start,
> > > but must clearly be supportable.
> > > 
> > > 
> > > * On queueing packets to userspace:
> > > 
> > >>> the current behavior is to just queue to the sk_error_queue as long
> > >>> as there is "SOF_TIMESTAMPING_TX_*" set in the skb's tx_flags and it
> > >>> is regardless of the sk_tsflags. "
> > > 
> > >> Totally correct. SOF_TIMESTAMPING_SOFTWARE is a report flag while
> > >> SOF_TIMESTAMPING_TX_* are generation flags. Without former, users can
> > >> read the skb from the errqueue but are not able to parse the
> > >> timestamps
> > > 
> > > Before queuing a packet to userspace on the error queue, the relevant
> > > reporting flag is always tested. sock_recv_timestamp has:
> > > 
> > >          /*
> > >           * generate control messages if
> > >           * - receive time stamping in software requested
> > >           * - software time stamp available and wanted
> > >           * - hardware time stamps available and wanted
> > >           */
> > >          if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> > >              (tsflags & SOF_TIMESTAMPING_RX_SOFTWARE) ||
> > >              (kt && tsflags & SOF_TIMESTAMPING_SOFTWARE) ||
> > >              (hwtstamps->hwtstamp &&
> > >               (tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)))
> > >                  __sock_recv_timestamp(msg, sk, skb);
> > > 
> > > Otherwise applications could get error messages queued, and
> > > epoll/poll/select would unexpectedly behave differently.
> > 
> > I just tried the following diff to remove setsockopt from txtimestamp.c and run 
> > "./txtimestamp -6 -c 1 -C -N -L ::1". It is getting the skb from the error queue 
> > with only cmsg flag.
> 
> That it surprising and against the API intent as I understand it.
> Let me reproduce and take a closer look.

Interesting. I guess my interpretation was wrong.

The reporting flags prevent reporting of the timestamp, but not
queuing of the skb on the error queue. Even if the only purpose is to
report a timestamp.

It goes back until well before all the API extensions. At least v3.6.

It still does suppress the timestamp itself if the relevant reporting
flag, SOF_TIMESTAMPING_SOFTWARE or SOF_TIMESTAMPING_RAW_HARDWARE, is
not set. So BPF should really still match that, I guess.