Re: [PATCH net 04/15] ipvs: avoid stack overflow from recursive connection expiration

[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 says the recursive chain walk exhausts the
  kernel stack "during namespace cleanup".

--

ipvs: avoid stack overflow from recursive connection expiration

This commit replaces the recursive controller expiration in
ip_vs_conn_expire() with an iterative walk. It no longer calls
ip_vs_conn_del() on the controller. Instead, the new
ip_vs_conn_del_reference() helper steals the controller's timer
reference, and the function loops back with cp = ct inside a single
rcu_read_lock() section.

> When a controlled IPVS connection expires, its controller may be expired
> synchronously if it has no remaining controlled connections. A chain of
> controlled connections can then recurse through ip_vs_conn_expire() and
> exhaust the kernel stack during namespace cleanup.

[Severity: Low]
This isn't a bug, but is "during namespace cleanup" the only place the
recursion can happen?

ip_vs_conn_del_reference() is the only code that sets cp->timeout = 0,
which arms the drop-controller chain. It is reached from runtime callers
as well as from the netns flush.

ip_vs_random_dropentry() runs from defense_work when drop_entry defense
is active:

net/netfilter/ipvs/ip_vs_conn.c:ip_vs_random_dropentry() {
    ...
drop:
			IP_VS_DBG(4, "drop connection\n");
			ip_vs_conn_del(cp);
    ...
}

ip_vs_expire_nodest_conn_flush() runs when a real server becomes
unavailable and expire_nodest_conn is set:

net/netfilter/ipvs/ip_vs_conn.c:ip_vs_expire_nodest_conn_flush() {
    ...
			ip_vs_conn_del(cp);
			if (cp_c && !atomic_read(&cp_c->n_control)) {
				IP_VS_DBG(4, "del controlling connection\n");
				ip_vs_conn_del(cp_c);
			}
    ...
}

The chain can also run from the timer softirq. If ip_vs_conn_unlink()
fails because refcnt > 1, the expire_later path keeps timeout == 0 when
n_control == 0:

net/netfilter/ipvs/ip_vs_conn.c:ip_vs_conn_expire() {
    ...
  expire_later:
    ...
	if (cp->timeout || atomic_read(&cp->n_control))
		cp->timeout = 60 * HZ;
    ...
	__ip_vs_conn_put_timer(cp);
}

The timer is then re-armed at jiffies + 0. The next callback walks the
controller chain, and before this patch that walk recursed on the
softirq stack.

The loop in ip_vs_conn_expire() covers all of these entry points, so the
code change itself looks fine. Could the commit message also mention the
runtime paths, since stable and CVE triage will rely on the stated
impact?

The commit message also doesn't explain how a deep chain is built. The
ip_vs_conn_expire_now() comment still says chains are at most
DATA->CTL->TPL. Both points are handled by the follow-up patch in the
series, "ipvs: reject FTP control ports as data ports". It rejects
control ports as FTP data ports in ip_vs_ftp_out() and ip_vs_ftp_in(),
which bounds the chain again.

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