[v2] tcp: update outdated comment for removed _decode_session6()

[PATCH v2] tcp: update outdated comment for removed _decode_session6()

Posted by Kexin Sun 1 week, 5 days ago

The comment in tcp_v6_fill_cb() states that the memmove of
IP6CB into TCP_SKB_CB must be done after xfrm6_policy_check()
because _decode_session6() uses IP6CB().

This ordering constraint was introduced by commit 2dc49d1680b5
("tcp6: don't move IP6CB before xfrm6_policy_check()") and was
valid at the time.  However, commit 7a0207094f1b ("xfrm: policy:
replace session decode with flow dissector") replaced the
per-family decode_session callbacks with the flow dissector,
which parses packet headers directly without accessing IP6CB().

Since xfrm6_policy_check() no longer reads IP6CB() anywhere in
its call chain, the ordering constraint no longer applies.
Simplify the comment to describe only what the memmove and
barrier() actually do.

Assisted-by: unnamed:deepseek-v3.2 coccinelle
Suggested-by: Kuniyuki Iwashima <kuniyu@google.com>
Signed-off-by: Kexin Sun <kexinsun@smail.nju.edu.cn>
---
 net/ipv6/tcp_ipv6.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index d10487b4e5bf..619b35669361 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1688,10 +1688,9 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
 static void tcp_v6_fill_cb(struct sk_buff *skb, const struct ipv6hdr *hdr,
 			   const struct tcphdr *th)
 {
-	/* This is tricky: we move IP6CB at its correct location into
-	 * TCP_SKB_CB(). It must be done after xfrm6_policy_check(), because
-	 * _decode_session6() uses IP6CB().
-	 * barrier() makes sure compiler won't play aliasing games.
+	/* Move IP6CB to its saved location in TCP_SKB_CB().
+	 * barrier() makes sure compiler won't play aliasing
+	 * games.
 	 */
 	memmove(&TCP_SKB_CB(skb)->header.h6, IP6CB(skb),
 		sizeof(struct inet6_skb_parm));
-- 
2.25.1

Re: [PATCH v2] tcp: update outdated comment for removed _decode_session6()

Posted by Eric Dumazet 1 week, 4 days ago

On Sat, Mar 21, 2026 at 10:24 PM Kexin Sun <kexinsun@smail.nju.edu.cn> wrote:
>
> The comment in tcp_v6_fill_cb() states that the memmove of
> IP6CB into TCP_SKB_CB must be done after xfrm6_policy_check()
> because _decode_session6() uses IP6CB().
>
> This ordering constraint was introduced by commit 2dc49d1680b5
> ("tcp6: don't move IP6CB before xfrm6_policy_check()") and was
> valid at the time.  However, commit 7a0207094f1b ("xfrm: policy:
> replace session decode with flow dissector") replaced the
> per-family decode_session callbacks with the flow dissector,
> which parses packet headers directly without accessing IP6CB().
>
> Since xfrm6_policy_check() no longer reads IP6CB() anywhere in
> its call chain, the ordering constraint no longer applies.
> Simplify the comment to describe only what the memmove and
> barrier() actually do.

Adding Florian to this thread.

So... the real question is :

Do we still need to call tcp_v6_fill_cb() from 4 different points,
and having this dance with tcp_v6_restore_cb() ?

Otherwise I see no real value in rephrasing a comment...

Re: [PATCH v2] tcp: update outdated comment for removed _decode_session6()

Posted by Kuniyuki Iwashima 1 week, 4 days ago

On Sat, Mar 21, 2026 at 10:24 PM Kexin Sun <kexinsun@smail.nju.edu.cn> wrote:
>
> The comment in tcp_v6_fill_cb() states that the memmove of
> IP6CB into TCP_SKB_CB must be done after xfrm6_policy_check()
> because _decode_session6() uses IP6CB().
>
> This ordering constraint was introduced by commit 2dc49d1680b5
> ("tcp6: don't move IP6CB before xfrm6_policy_check()") and was
> valid at the time.  However, commit 7a0207094f1b ("xfrm: policy:
> replace session decode with flow dissector") replaced the
> per-family decode_session callbacks with the flow dissector,
> which parses packet headers directly without accessing IP6CB().
>
> Since xfrm6_policy_check() no longer reads IP6CB() anywhere in
> its call chain, the ordering constraint no longer applies.
> Simplify the comment to describe only what the memmove and
> barrier() actually do.
>
> Assisted-by: unnamed:deepseek-v3.2 coccinelle
> Suggested-by: Kuniyuki Iwashima <kuniyu@google.com>
> Signed-off-by: Kexin Sun <kexinsun@smail.nju.edu.cn>
> ---
>  net/ipv6/tcp_ipv6.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index d10487b4e5bf..619b35669361 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1688,10 +1688,9 @@ int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
>  static void tcp_v6_fill_cb(struct sk_buff *skb, const struct ipv6hdr *hdr,
>                            const struct tcphdr *th)
>  {
> -       /* This is tricky: we move IP6CB at its correct location into
> -        * TCP_SKB_CB(). It must be done after xfrm6_policy_check(), because
> -        * _decode_session6() uses IP6CB().
> -        * barrier() makes sure compiler won't play aliasing games.

nit: unnecessary change.

Also, please check this guidline for next submission.

> The new version of patches should be posted as a separate thread, not as a reply to the previous posting.
https://docs.kernel.org/process/maintainer-netdev.html#resending-after-review


> +       /* Move IP6CB to its saved location in TCP_SKB_CB().
> +        * barrier() makes sure compiler won't play aliasing
> +        * games.
>          */
>         memmove(&TCP_SKB_CB(skb)->header.h6, IP6CB(skb),
>                 sizeof(struct inet6_skb_parm));
> --
> 2.25.1
>