netdev - Re: [PATCH net-next v3 2/4] net_tstamp: add SCM_TS_OPT

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+tcoAdgoKdRA-n5OdrD47K8_iJpCpaQ-8BGxXgTbFz1pW9nQ@mail.gmail.com>
Date: Thu, 5 Sep 2024 23:37:36 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: Willem de Bruijn <willemb@...gle.com>, netdev@...r.kernel.org, 
	David Ahern <dsahern@...nel.org>, Jakub Kicinski <kuba@...nel.org>, Simon Horman <horms@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v3 2/4] net_tstamp: add SCM_TS_OPT_ID for TCP sockets

Hello Vadim,

On Thu, Sep 5, 2024 at 10:58 PM Vadim Fedorenko
<vadim.fedorenko@...ux.dev> wrote:
>
> On 04/09/2024 12:31, Vadim Fedorenko wrote:
> > TCP sockets have different flow for providing timestamp OPT_ID value.
> > Adjust the code to support SCM_TS_OPT_ID option for TCP sockets.
> >
> > Signed-off-by: Vadim Fedorenko <vadfed@...a.com>
> > ---
> >   net/ipv4/tcp.c | 13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index 8a5680b4e786..5553a8aeee80 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -474,9 +474,10 @@ void tcp_init_sock(struct sock *sk)
> >   }
> >   EXPORT_SYMBOL(tcp_init_sock);
> >
> > -static void tcp_tx_timestamp(struct sock *sk, u16 tsflags)
> > +static void tcp_tx_timestamp(struct sock *sk, struct sockcm_cookie *sockc)
> >   {
> >       struct sk_buff *skb = tcp_write_queue_tail(sk);
> > +     u32 tsflags = sockc->tsflags;
> >
> >       if (tsflags && skb) {
> >               struct skb_shared_info *shinfo = skb_shinfo(skb);
> > @@ -485,8 +486,12 @@ static void tcp_tx_timestamp(struct sock *sk, u16 tsflags)
> >               sock_tx_timestamp(sk, tsflags, &shinfo->tx_flags);
> >               if (tsflags & SOF_TIMESTAMPING_TX_ACK)
> >                       tcb->txstamp_ack = 1;
> > -             if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK)
> > -                     shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
> > +             if (tsflags & SOF_TIMESTAMPING_TX_RECORD_MASK) {
> > +                     if (tsflags & SOCKCM_FLAG_TS_OPT_ID)
> > +                             shinfo->tskey = sockc->ts_opt_id;
> > +                     else
> > +                             shinfo->tskey = TCP_SKB_CB(skb)->seq + skb->len - 1;
> > +             }
> >       }
> >   }
> >
> > @@ -1318,7 +1323,7 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
> >
> >   out:
> >       if (copied) {
> > -             tcp_tx_timestamp(sk, sockc.tsflags);
> > +             tcp_tx_timestamp(sk, &sockc);
> >               tcp_push(sk, flags, mss_now, tp->nonagle, size_goal);
> >       }
> >   out_nopush:
>
> Hi Willem,
>
> Unfortunately, these changes are not enough to enable custom OPT_ID for
> TCP sockets. There are some functions which rewrite shinfo->tskey in TCP
> flow:

I was planning to test locally after your new version is posted... Now
I can see the good of a useful selftest from this :)

>
> tcp_skb_collapse_tstamp()
> tcp_fragment_tstamp()
> tcp_gso_tstamp()
>
> I believe the last one breaks tests, but the problem is that there is no
> easy way to provide the flag of constant tskey to it. Only
> shinfo::tx_flags are available at the caller side and we have already
> discussed that we shouldn't use the last bit of this field.
>
> So, how should we deal with the problem? Or is it better to postpone
> support for TCP sockets in this case?

I'm not Willem, but my intuition is to postpone it since this will
make the series much bigger than the previous expectation.

Let Willem decide finally :)