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: <CAL+tcoBR80V_760RoZ9w0cdAfyoHwGS0fL7ckuAF7JSpdzAvEQ@mail.gmail.com>
Date: Wed, 9 Oct 2024 21:52:13 +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, 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

On Wed, Oct 9, 2024 at 9:19 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> 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.

Adding a new sk state or skb state is a much clearer way. That is also
what I commented on the patch [0/9]. While waiting for BPF experts'
reply, I keep investigating if there is a good way to add a new field.

Thanks,
Jason