Re: [PATCH net 06/12] ipvs: bound LBLCR and LBLC cache growth

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ipvs: bound LBLCR and LBLC cache growth

This adds a hard admission limit to ip_vs_lblc_new() and
ip_vs_lblcr_new(), rejecting new cache entries once atomic_read(&tbl->entries)
reaches max_size * 3 / 2.  The stated intent is to leave headroom so the
periodic collector can catch up while the scheduler fallback keeps serving
traffic.

> diff --git a/net/netfilter/ipvs/ip_vs_lblc.c b/net/netfilter/ipvs/ip_vs_lblc.c
> index 693bcc82ccb7..8180a7ba9f53 100644
> --- a/net/netfilter/ipvs/ip_vs_lblc.c
> +++ b/net/netfilter/ipvs/ip_vs_lblc.c
> @@ -204,6 +204,9 @@ ip_vs_lblc_new(struct ip_vs_lblc_table *tbl, const union nf_inet_addr *daddr,
>  			return en;
>  		ip_vs_lblc_del(en);
>  	}
> +	if (atomic_read(&tbl->entries) >= tbl->max_size * 3 / 2)
> +		return NULL;
> +
>  	en = kmalloc_obj(*en, GFP_ATOMIC);
>  	if (!en)
>  		return NULL;

[Severity: High]
Can tbl->entries be trusted as a bound on the number of live entries in
ip_vs_lblc_new()?  The replacement path just above the new test is the only
ip_vs_lblc_del() call site that does not decrement the counter:

net/netfilter/ipvs/ip_vs_lblc.c:ip_vs_lblc_new() {
	en = ip_vs_lblc_get(af, tbl, daddr);
	if (en) {
		if (en->dest == dest)
			return en;
		ip_vs_lblc_del(en);	/* no atomic_dec(&tbl->entries) */
	}
	...
}

The helper itself does not touch the counter:

static inline void ip_vs_lblc_del(struct ip_vs_lblc_entry *en)
{
	hlist_del_rcu(&en->list);
	call_rcu(&en->rcu_head, ip_vs_lblc_rcu_free);
}

while every other caller pairs it with an explicit decrement, e.g. in
ip_vs_lblc_flush(), ip_vs_lblc_full_check() and ip_vs_lblc_check_expire():

			ip_vs_lblc_del(en);
			atomic_dec(&tbl->entries);

Does that mean each dest change for an already cached daddr adds +1 to
tbl->entries while the live entry count stays the same, since
ip_vs_lblc_hash() is the only place that increments?

	hlist_add_head_rcu(&en->list, &tbl->bucket[hash]);
	atomic_inc(&tbl->entries);

If so, does the drift ever get corrected?  The collector decrements only when
it removes a real entry, so it lowers both the counter and the live count
equally, and tbl->entries otherwise seems to be reset only in
ip_vs_lblc_init_svc().

There also looks to be a second effect from the placement of the new test
after ip_vs_lblc_del(): when the limit fires, the old entry has already been
unlinked and NULL is returned without a decrement and without a replacement,
so the table shrinks while the counter stays where it is.

The replacement path appears to be driven by remote packets, since
ip_vs_lblc_schedule() calls __ip_vs_lblc_schedule() and then
ip_vs_lblc_new() with a different dest whenever the cached dest is not
available, has zero weight, or is_overloaded():

	dest = __ip_vs_lblc_schedule(svc);
	...
	spin_lock_bh(&svc->sched_lock);
	if (!tbl->dead)
		ip_vs_lblc_new(tbl, &iph->daddr, svc->af, dest);

Once the accumulated drift alone reaches max_size * 3 / 2 (24576 with the
default 16384), does ip_vs_lblc_new() return NULL permanently for that
service, even with an empty table?  That would leave LBLC locality caching
disabled for the lifetime of the service, with every packet taking the full
__ip_vs_lblc_schedule() dest walk and ip_vs_lblc_check_expire() still
scanning all 1024 buckets under svc->sched_lock looking for entries that are
no longer there.

That also seems to conflict with the commit message text:

    The extra headroom lets the periodic collector catch up while the
    existing scheduler fallback continues to use the selected destination
    when cache creation fails.

Would adding the missing atomic_dec(&tbl->entries) next to the
ip_vs_lblc_del(en) in ip_vs_lblc_new(), or moving the new test ahead of the
lookup and delete, make the limit match the live entry count?

Note that ip_vs_lblcr_new() below places the identical test inside
if (!en), before any removal, and has no unbalanced delete, so the two
schedulers' counters do not appear to satisfy the same invariant even though
the same expression is applied to both.

> diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c
> index f53f05ceea36..858393b1d2d1 100644
> --- a/net/netfilter/ipvs/ip_vs_lblcr.c
> +++ b/net/netfilter/ipvs/ip_vs_lblcr.c
> @@ -363,6 +363,9 @@ ip_vs_lblcr_new(struct ip_vs_lblcr_table *tbl, const union nf_inet_addr *daddr,
>  
>  	en = ip_vs_lblcr_get(af, tbl, daddr);
>  	if (!en) {
> +		if (atomic_read(&tbl->entries) >= tbl->max_size * 3 / 2)
> +			return NULL;
> +
>  		en = kmalloc_obj(*en, GFP_ATOMIC);
>  		if (!en)
>  			return NULL;

[Severity: Medium]
This isn't a bug introduced by this patch, but does the new limit still leave
the per-bucket chain length unbounded?  ip_vs_lblc_hashkey() and
ip_vs_lblcr_hashkey() hash the destination address with no per-table random
seed:

	return hash_32(ntohl(addr_fold), IP_VS_LBLC_TAB_BITS);

so bucket selection for a chosen address is predictable offline, and
ip_vs_lblc_get() / ip_vs_lblcr_get() walk the chain linearly on every
scheduled packet:

	hlist_for_each_entry_rcu(en, &tbl->bucket[hash], list)
		if (ip_vs_addr_equal(af, &en->addr, addr))
			return en;

Since the new test caps only the total entry count and not the chain length,
can all 24576 permitted entries be steered into a single one of the 1024
buckets, leaving the lookup in softirq context (and again under
spin_lock_bh(&svc->sched_lock) from ip_vs_lblc_new()) walking a ~24k element
list per packet?  The patch does reduce the worst case compared to the
unbounded chain before it, so this is a pre-existing exposure rather than
something the patch adds, but would seeding the hash or bounding chain length
be worth doing on top?