netdev - Re: [PATCH net-next v2 09/12] net-timestamp: add tx OPT_ID

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: <670dd59de9a73_2e58fb29464@willemb.c.googlers.com.notmuch>
Date: Mon, 14 Oct 2024 22:38:21 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>, 
 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, 
 ast@...nel.org, 
 daniel@...earbox.net, 
 andrii@...nel.org, 
 martin.lau@...ux.dev, 
 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 09/12] net-timestamp: add tx OPT_ID_TCP
 support for bpf case

Jason Xing wrote:
> On Tue, Oct 15, 2024 at 9:38 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > Jason Xing wrote:
> > > From: Jason Xing <kernelxing@...cent.com>
> > >
> > > We can set OPT_ID|OPT_ID_TCP before we initialize the last skb
> > > from each sendmsg. We only set the socket once like how we use
> > > setsockopt() with OPT_ID|OPT_ID_TCP flags.
> > >
> > > Note: we will check if non-bpf _and_ bpf sk_tsflags have OPT_ID
> > > flag. If either of them has been set before, we will not initialize
> > > the key any more,
> >
> > Where and how is this achieved?
> 
> Please see this patch and you will find the following codes.
> +       tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
> +                   sk->sk_tsflags[BPFPROG_TS_REQUESTOR]);

I saw that, but it's not a condition that stops reinitializing. Which
I think is the intent, based on "If either of them has been set
before, we will not initialize the key anymore"?

Reinitialization is actually supported behavior.

                if (val & SOF_TIMESTAMPING_OPT_ID &&
                    !(sk->sk_tsflags & SOF_TIMESTAMPING_OPT_ID)) {

But the sk_tsflags bit may be repeatedly set and cleared.

Anyway, the current patch sets it if either requests it?

+	tsflags |= (sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] |
+		    sk->sk_tsflags[BPFPROG_TS_REQUESTOR]);
 	if (val & SOF_TIMESTAMPING_OPT_ID &&
-	    !(sk->sk_tsflags[SOCKETOPT_TS_REQUESTOR] & SOF_TIMESTAMPING_OPT_ID)) {
+	    !(tsflags & SOF_TIMESTAMPING_OPT_ID)) {

 
> But the difference/problem is that the non-bpf feature only init it
> when connect() is done, but the bpf feature could do it at the
> beginning of connect(). If running txtimestamp -l 1000, the former
> will generate 999 for turkey while the latter 1000.