netdev - Re: [PATCH net-next 6/9] 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: <670682ed93e9c_1cca3129431@willemb.c.googlers.com.notmuch>
Date: Wed, 09 Oct 2024 09:19:41 -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 6/9] net-timestamp: add tx OPT_ID_TCP support for
 bpf case

Jason Xing wrote:
> On Wed, Oct 9, 2024 at 2:56 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.
> > >
> > > Signed-off-by: Jason Xing <kernelxing@...cent.com>
> > > ---
> > >  net/core/skbuff.c | 16 +++++++++++++---
> > >  net/ipv4/tcp.c    | 19 +++++++++++++++----
> > >  2 files changed, 28 insertions(+), 7 deletions(-)
> > >
> >
> > > @@ -491,10 +491,21 @@ static u32 bpf_tcp_tx_timestamp(struct sock *sk)
> > >       if (!(flags & SOF_TIMESTAMPING_TX_RECORD_MASK))
> > >               return 0;
> > >
> > > +     /* We require users to set both OPT_ID and OPT_ID_TCP flags
> > > +      * together here, or else the key might be inaccurate.
> > > +      */
> > > +     if (flags & SOF_TIMESTAMPING_OPT_ID &&
> > > +         flags & SOF_TIMESTAMPING_OPT_ID_TCP &&
> > > +         !(sk->sk_tsflags & (SOF_TIMESTAMPING_OPT_ID | SOF_TIMESTAMPING_OPT_ID_TCP))) {
> > > +             atomic_set(&sk->sk_tskey, (tcp_sk(sk)->write_seq - copied));
> > > +             sk->sk_tsflags |= (SOF_TIMESTAMPING_OPT_ID | SOF_TIMESTAMPING_OPT_ID_TCP);
> >
> > So user and BPF admin conflict on both sk_tsflags and sktskey?
> >
> > I think BPF resetting this key, or incrementing it, may break user
> > expectations.
> 
> Yes, when it comes to OPT_ID and OPT_ID_TCP, conflict could happen.
> The reason why I don't use it like BPF_SOCK_OPS_TS_SCHED_OPT_CB flags
> (which is set along with each last skb) is that OPT_ID logic is a
> little bit complex. If we want to avoid touching sk_tsflags field in
> struct sock, we have to re-implement a similiar logic as you've
> already done in these years.

One option may be to only allow BPF to use sk_tsflags and sk_tskey if
sk_tsflags is not set by the user, and to fail user access to these
fields later.

That enforces mutual exclusion between either user or admin
timestamping.

Of course, it may still break users if BPF is first, but the user
socket tries to enable it later. So an imperfect solution.

Ideally the two would use separate per socket state. I don't know
all the options the various BPF hooks may have for this.