netdev - Re: [PATCH net-next] net-timestamp: support TCP GSO case for a few missing flags

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+tcoDg1mQ+7DtYNgYomum9o=gzwtrjedYf7VmHud54VfSynQ@mail.gmail.com>
Date: Mon, 3 Mar 2025 10:53:01 +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, ncardwell@...gle.com, kuniyu@...zon.com, 
	dsahern@...nel.org, willemb@...gle.com, horms@...nel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net-timestamp: support TCP GSO case for a few
 missing flags

On Mon, Mar 3, 2025 at 10:17 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> Jason Xing wrote:
> > When I read through the TSO codes, I found out that we probably
> > miss initializing the tx_flags of last seg when TSO is turned
> > off,
>
> When falling back onto TCP GSO. Good catch.
>
> This is a fix, so should target net?

After seeing your comment below, I'm not sure if the next version
targets the net branch because SKBTX_BPF flag is not in the net branch.

If target net-net tree, I would add the following:
Fixes: 6b98ec7e882af ("bpf: Add BPF_SOCK_OPS_TSTAMP_SCHED_CB callback")
Fixes: 4ed2d765dfacc ("net-timestamp: TCP timestamping")

>
> > which means at the following points no more timestamp
> > (for this last one) will be generated. There are three flags
> > to be handled in this patch:
> > 1. SKBTX_HW_TSTAMP
> > 2. SKBTX_HW_TSTAMP_USE_CYCLES
>
> Nit: this no longer exists

Right, I wrote on the old branch, sorry.

>
> (But it will affect the upcoming completion timestamp.)
>
> > 3. SKBTX_BPF
> >
> > This patch initializes the tx_flags to SKBTX_ANY_TSTAMP like what
> > the UDP GSO does. But flag like SKBTX_SCHED_TSTAMP is not useful
> > and will not be used in the remaining path since the skb has already
> > passed the QDisc layer.
>
> Unless multiple layers of qdiscs (e.g., bonding or tunneling) and
> GSO is applied on the first layer, and SKBTX_SW_TSTAMP is not
> requested. But SCHED without SW is an unlikely configuration
> Probably best to just drop this.

Got it. I will remove this description.

>
> >
> > Signed-off-by: Jason Xing <kerneljasonxing@...il.com>
> > ---
> >  net/ipv4/tcp_offload.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
> > index 2308665b51c5..886582002425 100644
> > --- a/net/ipv4/tcp_offload.c
> > +++ b/net/ipv4/tcp_offload.c
> > @@ -13,12 +13,15 @@
> >  #include <net/tcp.h>
> >  #include <net/protocol.h>
> >
> > -static void tcp_gso_tstamp(struct sk_buff *skb, unsigned int ts_seq,
> > +static void tcp_gso_tstamp(struct sk_buff *skb, struct sk_buff *gso_skb,
> >                          unsigned int seq, unsigned int mss)
> >  {
> > +     u32 flags = skb_shinfo(gso_skb)->tx_flags & SKBTX_ANY_TSTAMP;
> > +     u32 ts_seq = skb_shinfo(gso_skb)->tskey;
> > +
> >       while (skb) {
> >               if (before(ts_seq, seq + mss)) {
> > -                     skb_shinfo(skb)->tx_flags |= SKBTX_SW_TSTAMP;
> > +                     skb_shinfo(skb)->tx_flags |= flags;
> >                       skb_shinfo(skb)->tskey = ts_seq;
> >                       return;
> >               }
> > @@ -193,8 +196,8 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb,
> >       th = tcp_hdr(skb);
> >       seq = ntohl(th->seq);
> >
> > -     if (unlikely(skb_shinfo(gso_skb)->tx_flags & SKBTX_SW_TSTAMP))
> > -             tcp_gso_tstamp(segs, skb_shinfo(gso_skb)->tskey, seq, mss);
> > +     if (unlikely(skb_shinfo(gso_skb)->tx_flags & (SKBTX_ANY_TSTAMP)))
>
> no need for the extra parentheses

Will correct it.

Thank for the review,
Jason


>
> > +             tcp_gso_tstamp(segs, gso_skb, seq, mss);
> >
> >       newcheck = ~csum_fold(csum_add(csum_unfold(th->check), delta));
> >
> > --
> > 2.43.5
> >
>
>