Re: [PATCH net 08/15] ipvs: update flags and seqs in backup server under lock

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message overstates the old bug.

--

ipvs: update flags and seqs in backup server under lock

This patch extends the cp->lock critical section in ip_vs_proc_conn().
On the backup server, cp->flags, in_seq/out_seq, state, old_state and
timeout are now updated together. New connections also take cp->lock,
and ip_vs_try_bind_dest() is called after the lock is dropped.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927220816.268206-1-pablo%40netfilter.org

> Sashiko reports that we modify the conn flags and seq values in
> the backup server without holding cp->lock. The same is true for
> other fields such as state, old_state and timeout which are
> modified by protocols under lock.

[Severity: Low]
Is the first sentence accurate for cp->flags?

Before this patch, the existing-connection path in ip_vs_proc_conn()
already updated cp->flags under cp->lock:

net/netfilter/ipvs/ip_vs_sync.c:ip_vs_proc_conn() {
    ...
		spin_lock_bh(&cp->lock);
		...
		flags &= IP_VS_CONN_F_BACKUP_UPD_MASK;
		flags |= cp->flags & ~IP_VS_CONN_F_BACKUP_UPD_MASK;
		cp->flags = flags;
		spin_unlock_bh(&cp->lock);
		if (!dest)
			ip_vs_try_bind_dest(cp);
    ...
}

ip_vs_try_bind_dest() also takes cp->lock before ip_vs_bind_dest()
changes cp->flags.

The fields written without the lock were the seqs, state, old_state
and timeout, after spin_unlock_bh():

	if (opt) {
		cp->in_seq = opt->in_seq;
		cp->out_seq = opt->out_seq;
	}
	atomic_set(&cp->in_pkts, sysctl_sync_threshold(ipvs));
	cp->state = state;
	cp->old_state = cp->state;

So the IP_VS_CONN_F_SEQ_MASK bits were set under the lock, but the
in_seq/out_seq values they describe were written outside it. The
subject line and "update the fields atomically" describe this
correctly.

Could the first sentence say that flags and seqs were not updated
together under the lock, instead of saying the flags were modified
without it? The code change itself looks correct.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113011447.2357709-2-pablo%40netfilter.org