Re: [PATCH] tcp: Add flags frames to payload frames arrays

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 10 Sep 2025 15:46:30 +0800
Yumei Huang <yuhuang@redhat.com> wrote:

> There is an issue reported by Volker Diels-Grabsch and Boleyn Su.
> A segmentation fault occurs when executing the following command:
> 
> 	(sleep 0.1; ssh -p 22000 127.0.0.1) & passt -f -t 22000:22
> 
> It's caused by commit 78da088f7bab ("tcp: unify payload and flags
> l2 frames array"). Fix it by adding flags frames to payload frames
> arrays.

Ouch. Nice catch!

I guess in this sentence you meant "Fix it by setting ownership
information for flags frames"? The patch itself looks correct, the
commit message doesn't entirely match it though.

The commit title is also a bit mismatching. The description for
tcp_frame_conns[] is:

/* References tracking the owner connection of frames in the tap outqueue */

A couple of minor details you probably didn't know about:

- in this case we would typically use all these tags in the commit
  message:

Reported-by: Volker Diels-Grabsch <v@njh.eu>
Reported-by: Boleyn Su <boleyn.su@gmail.com>
Fixes: 78da088f7bab ("tcp: unify payload and flags l2 frames array")

- you should Cc: Jon as the author of the faulty commit (I just did)

- David's upstream email address is david@gibson.dropbear.id.au (I just
  changed it)

I can add the tags for you, but it's probably easier if you re-post a
v2 with a more accurate commit message. Thanks!

> Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> ---
>  tcp_buf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tcp_buf.c b/tcp_buf.c
> index bc898de..d351c20 100644
> --- a/tcp_buf.c
> +++ b/tcp_buf.c
> @@ -209,13 +209,14 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	if (ret <= 0)
>  		return ret;
>  
> -	tcp_payload_used++;
> +	tcp_frame_conns[tcp_payload_used++] = conn;
>  	l4len = optlen + sizeof(struct tcphdr);
>  	iov[TCP_IOV_PAYLOAD].iov_len = l4len;
>  	tcp_l2_buf_fill_headers(conn, iov, NULL, seq, false);
>  
>  	if (flags & DUP_ACK) {
>  		struct iovec *dup_iov = tcp_l2_iov[tcp_payload_used++];
> +		tcp_frame_conns[tcp_payload_used - 1] = conn;
>  
>  		memcpy(dup_iov[TCP_IOV_TAP].iov_base, iov[TCP_IOV_TAP].iov_base,
>  		       iov[TCP_IOV_TAP].iov_len);

-- 
Stefano