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

[Yumei Huang <yuhuang@redhat.com>]

Thank you Stefano for all the information.
I will send a v2 soon.

On Wed, Sep 10, 2025 at 4:52 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> 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
>


-- 
Thanks,

Yumei Huang