Re: [PATCH 1/1] ipvs: preserve conn hash flags when late-binding dest

On Sun, Jun 28, 2026 at 4:47=E2=80=AFAM Julian Anastasov <[email protected]> wrote:
>
>
>         Hello,
>
> On Sun, 28 Jun 2026, Ren Wei wrote:
>
> > From: Zhiling Zou <[email protected]>
> >
> > Synced connections can be created before their destination exists. When
> > the destination is later added, ip_vs_try_bind_dest() binds it to the
> > existing connection through ip_vs_bind_dest().
> >
> > ip_vs_bind_dest() copies destination connection flags into cp->flags.
> > For an already hashed connection, changing flags that define conn_tab
> > membership breaks the hash table invariants. In particular, adding
> > IP_VS_CONN_F_ONE_PACKET after the connection has been hashed can make
> > expiry skip unlinking it from conn_tab. Changing the forwarding method
> > can also make unlink use a different single or double hash-node layout
> > than the one used at insertion time.
> >
> > Preserve the flags that define conn_tab hashing when binding a
> > destination to an already hashed connection.
> >
> > Fixes: 26ec037f9841 ("IPVS: one-packet scheduling")
>
>         The problem with the fix is that we should do it
> in the hard way: the backup server should be able to define
> its own forwarding methods. Otherwise, we can break existing
> setups. For example, master can have localnode for some
> dests, this can not be preserved in the backup for the
> synced conns.
>
> > Cc: [email protected]
> > Reported-by: Yuan Tan <[email protected]>
> > Reported-by: Yifan Wu <[email protected]>
> > Reported-by: Juefei Pu <[email protected]>
> > Reported-by: Xin Liu <[email protected]>
> > Assisted-by: Codex:gpt-5.4
> > Signed-off-by: Zhiling Zou <[email protected]>
> > Signed-off-by: Ren Wei <[email protected]>
> > ---
> >  net/netfilter/ipvs/ip_vs_conn.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs=
_conn.c
> > index cb36641f8d1c..016273906aac 100644
> > --- a/net/netfilter/ipvs/ip_vs_conn.c
> > +++ b/net/netfilter/ipvs/ip_vs_conn.c
> > @@ -998,7 +998,11 @@ static inline int ip_vs_dest_totalconns(struct ip_=
vs_dest *dest)
> >  static inline void
> >  ip_vs_bind_dest(struct ip_vs_conn *cp, struct ip_vs_dest *dest)
> >  {
> > +     const unsigned int hash_flags =3D IP_VS_CONN_F_FWD_MASK |
> > +                                     IP_VS_CONN_F_NOOUTPUT |
> > +                                     IP_VS_CONN_F_ONE_PACKET;
> >       unsigned int conn_flags;
> > +     __u32 old_flags;
> >       __u32 flags;
> >
> >       /* if dest is NULL, then return directly */
> > @@ -1011,7 +1015,8 @@ ip_vs_bind_dest(struct ip_vs_conn *cp, struct ip_=
vs_dest *dest)
> >       conn_flags =3D atomic_read(&dest->conn_flags);
> >       if (cp->protocol !=3D IPPROTO_UDP)
> >               conn_flags &=3D ~IP_VS_CONN_F_ONE_PACKET;
> > -     flags =3D cp->flags;
> > +     old_flags =3D cp->flags;
> > +     flags =3D old_flags;
> >       /* Bind with the destination and its corresponding transmitter */
> >       if (flags & IP_VS_CONN_F_SYNC) {
>
>         We can here unconditionally drop the IP_VS_CONN_F_ONE_PACKET flag=
:
>
>                 conn_flags &=3D ~IP_VS_CONN_F_ONE_PACKET;
>
>         Because IP_VS_CONN_F_ONE_PACKET conns are not synced.
>
>         And here when (flags & IP_VS_CONN_F_HASHED) and the fwd
>         method changes between MASQ and non-MASQ for
>         !IP_VS_CONN_F_TEMPLATE we should call some new func
>         that properly hashes/unhashes just the hn1 node.
>         I can provide such function with proper locking.
>

Thank you for the review.

I agree. Preserving the forwarding method is too conservative and can
break backup setups where the backup server intentionally uses its own
forwarding method.

For v2 I will drop IP_VS_CONN_F_ONE_PACKET from conn_flags
unconditionally, since one-packet connections are not synced.

For the MASQ/non-MASQ transition on already hashed non-template
connections, I agree that the right fix is to update only the hn1 hash
node instead of preserving the old forwarding method. Since this needs
to follow the conn_tab locking rules carefully, I would appreciate the
helper you mentioned and will use it for v2.

Thanks,
Zhiling

> >               /* if the connection is not template and is created
> > @@ -1023,6 +1028,13 @@ ip_vs_bind_dest(struct ip_vs_conn *cp, struct ip=
_vs_dest *dest)
> >               flags &=3D ~(IP_VS_CONN_F_FWD_MASK | IP_VS_CONN_F_NOOUTPU=
T);
> >       }
> >       flags |=3D conn_flags;
> > +
> > +     /* Preserve conn_tab hashing invariants after late binding. */
> > +     if (old_flags & IP_VS_CONN_F_HASHED) {
> > +             flags &=3D ~hash_flags;
> > +             flags |=3D old_flags & hash_flags;
> > +     }
> > +
> >       cp->flags =3D flags;
> >       cp->dest =3D dest;
>
> Regards
>
> --
> Julian Anastasov <[email protected]>
>