On Tue, Sep 29, 2026 at 11:55:11AM +0200, Paolo Abeni wrote:
On 9/29/26 11:43, Pablo Neira Ayuso wrote:
quoted
Hi Paolo,
On Tue, Sep 29, 2026 at 10:19:55AM +0200, Paolo Abeni wrote:
quoted
On 9/29/26 06:06, Julian Anastasov wrote:
quoted
quoted
quoted
@@ -744,7 +749,7 @@ void ip_vs_sync_conn(struct netns_ipvs *ipvs, struct ip_vs_conn *cp, int pkts)
s->v4.vaddr = cp->vaddr.ip;
s->v4.daddr = cp->daddr.ip;
}
- if (cp->flags & IP_VS_CONN_F_SEQ_MASK) {
+ if (seq_mask) {
*(p++) = IPVS_OPT_SEQ_DATA;
*(p++) = sizeof(struct ip_vs_sync_conn_options);
hton_seq(&cp->in_seq, (struct ip_vs_seq *)p);
[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The flags
are now a consistent snapshot, but the sequence data is still read
without cp->lock. Nothing orders that read against READ_ONCE(cp->flags).
Can the master send stale or half-updated ip_vs_seq values here?
ip_vs_sync_conn() here only holds ipvs->sync_buff_lock. The same applies
to the memcpy() of cp->sync_conn_opt in ip_vs_sync_conn_v0().
On the backup side, ip_vs_proc_conn() sets the SEQ bits in cp->flags under
cp->lock and drops the lock. Only after that does it copy the sequence
data, with no lock held:
net/netfilter/ipvs/ip_vs_sync.c:ip_vs_proc_conn() {
...
cp->flags = flags;
spin_unlock_bh(&cp->lock);
...
if (opt) {
cp->in_seq = opt->in_seq;
cp->out_seq = opt->out_seq;
}
...
}
On a node running both the master and backup daemons, ip_vs_sync_conn()
could see seq_mask set and then send old or torn init_seq, delta and
previous_delta values. A peer with an app bound to that connection would
then apply the wrong TCP seq/ack adjustment after failover.
Agreed, this can be improved to take a snapshot of
flags and seqs together under lock. I'll request to drop
this version, it is not urgent to apply it.
It looks like a respin of the PR is needed, I'll drop the revision from PW.
Just wrote to Jakub with a summary on the LLM report.
I would take this PR as is if it is still possible.
If you feel strong about to need to respin this PR, that's also fine
with me, just let confirm where to go.
Uhm... I must admit that given the pressure we received from Linus on
shrinking/prevent from increasing the net PR size I think we are better
off dropping patch 2/11 entirely.
I'm sorry for the extra work, but the messaging from Linus was pretty
strong:
https://lore.kernel.org/netdev/CAHk-=wiSnTE9vBZ=5_v+3EEkdazCCbBM5YABRzRHUAeRdyd4Xw@mail.gmail.com/ (local)
Yes, I read that. I tried to move what is less relevant to nf-next (if
you look at my nf-next PR, it's contains not so relevant fixes too).
include/linux/netdevice.h | 3 ++
include/net/netfilter/nf_flow_table.h | 2 +-
net/mac80211/iface.c | 7 +++++
net/netfilter/ipset/ip_set_bitmap_gen.h | 2 +-
net/netfilter/ipvs/ip_vs_conn.c | 3 ++
net/netfilter/ipvs/ip_vs_lblc.c | 4 +++
net/netfilter/ipvs/ip_vs_lblcr.c | 3 ++
net/netfilter/ipvs/ip_vs_sync.c | 56 ++++++++++++++++++++++++++-------
net/netfilter/nf_flow_table_core.c | 7 ++++-
net/netfilter/nf_flow_table_offload.c | 14 +++------
net/netfilter/nf_flow_table_path.c | 3 ++
net/netfilter/nf_nat_bpf.c | 3 ++
net/netfilter/nf_nat_core.c | 5 +--
net/netfilter/nft_flow_offload.c | 7 ++++-
net/netfilter/nft_set_rbtree.c | 2 ++
net/sched/act_ct.c | 2 +-
16 files changed, 95 insertions(+), 28 deletions(-)
Yes, this patch is larger in the diffstat. I can just move it to
nf-next if you prefer.
I will respin and send v2 today without this.
Thanks.