Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 0 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] The failure path in ip_vs_conn_unlink() saturates the refcount
instead of restoring it.
- [High] The controller walk in ip_vs_conn_expire() relies on RCU to keep
ct alive.
- [Low] An undeclared stable dependency.
--
ipvs: avoid stack overflow from recursive connection expiration
This commit removes the recursive ip_vs_conn_del(ct) call in
ip_vs_conn_expire(). Instead, once the current connection is cleaned up,
the function carries on expiring its controller in a loop. The whole walk
runs under one rcu_read_lock(), and every later iteration runs with my_cb
set to false (deletion mode).
> This keeps controlled-connection cleanup synchronous while using one stack
> frame for the whole chain. The timer callback race during connection
> deletion is handled by the preceding refcount fix.
>
> Fixes: f9200a52eedf ("ipvs: avoid expiring many connections from timer")
> Cc: [email protected]
[Severity: Low]
This patch is tagged for stable, but it only works on top of the
preceding patch, "ipvs: wait the running timer cb on conn deletion".
That patch adds:
- ip_vs_conn_unlink(cp, my_cb)
- the tagged-pointer my_cb decoding in ip_vs_conn_expire()
- the new ip_vs_conn_del()
- the timer callback revalidation
The prerequisite has a Fixes: tag but no Cc: stable. This commit message
only calls it "the preceding refcount fix".
Could the prerequisite also be tagged for stable, and named here by title
or as a stable dependency?
If this loop were backported alone, or adapted onto the old
ip_vs_conn_del()/ip_vs_conn_del_put() layout, ct would be expired
synchronously with my_cb set to false. Nothing would then guard against
ct's own timer callback running at the same time.
> diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> index 32cfc02aa2912..f85752e79ed92 100644
> --- a/net/netfilter/ipvs/ip_vs_conn.c
> +++ b/net/netfilter/ipvs/ip_vs_conn.c
> @@ -1357,6 +1357,9 @@ static void ip_vs_conn_expire(struct timer_list *t)
> struct ip_vs_conn *cp = timer_container_of(cp, t2, timer);
> struct netns_ipvs *ipvs = cp->ipvs;
>
> + rcu_read_lock();
> +
> +repeat:
> /*
> * do I control anybody?
> */
> @@ -1366,19 +1369,20 @@ static void ip_vs_conn_expire(struct timer_list *t)
> /* Unlink conn if not referenced anymore */
> if (likely(ip_vs_conn_unlink(cp, my_cb))) {
> struct ip_vs_conn *ct = cp->control;
> + bool next = false;
>
> /* does anybody control me? */
> if (ct) {
> - rcu_read_lock();
> ip_vs_control_del(cp);
> /* Drop CTL or non-assured TPL if not used anymore */
> if (!cp->timeout && !atomic_read(&ct->n_control) &&
> (!(ct->flags & IP_VS_CONN_F_TEMPLATE) ||
> !(ct->state & IP_VS_CTPL_S_ASSURED))) {
> IP_VS_DBG(4, "drop controlling connection\n");
> - ip_vs_conn_del(ct);
> + if (ct->control)
> + ct->timeout = 0;
> + next = true;
[Severity: High]
Is RCU enough to keep ct alive here when the controller has
IP_VS_CONN_F_ONE_PACKET set?
After ip_vs_control_del() drops ct->n_control to zero, the walk keeps
using ct without holding a reference. It touches n_control, flags, state
and timeout, and later refcnt via ip_vs_conn_unlink(). A ONE_PACKET
connection is freed right away, not after a grace period:
if (cp->flags & IP_VS_CONN_F_ONE_PACKET)
ip_vs_conn_rcu_free(&cp->rcu_head);
else
call_rcu(&cp->rcu_head, ip_vs_conn_rcu_free);
Such a controller can be built. IP_VS_CONN_F_DEST_MASK includes
IP_VS_CONN_F_ONE_PACKET, and the legacy sockopt path copies the flags
unfiltered in ip_vs_copy_udest_compat():
udest->conn_flags = udest_compat->conn_flags;
ip_vs_bind_dest() then passes dest->conn_flags on to UDP persistence
templates.
Suppose a destination update clears the flag between the template's and
the child's ip_vs_conn_new() calls in ip_vs_sched_persist(). An unhashed
ONE_PACKET template then controls a hashed normal child. While n_control
is above zero, the template's timer is re-armed every jiffy, because
__ip_vs_conn_put_timer() uses a timeout of 0 for ONE_PACKET.
Now suppose the child is deleted through ip_vs_conn_del() by flush,
drop_entry or nodest. cp->timeout is 0, so next is set and the walk calls
ip_vs_conn_unlink(ct, false). For ONE_PACKET, that function returns true
right after refcount_dec_if_one() and never calls timer_delete():
if (cp->flags & IP_VS_CONN_F_ONE_PACKET)
return true;
Would the walk then free ct at once, while ct->timer is still pending in
the timer wheel?
There is also the other order. If ct's own timer callback on another CPU
frees it first, wouldn't the walk read and write ct->n_control, flags,
state, timeout and refcnt on freed memory?
Before the series, the code took a reference on ct (has_ref) and gated
deletion on timer_delete() in ip_vs_conn_del_put(). The preceding patch,
"ipvs: wait the running timer cb on conn deletion", replaced that with
rcu_read_lock(). This patch keeps that approach and widens the window to
cover the whole cp cleanup before goto repeat.
> }
> - rcu_read_unlock();
> }
>
> if ((cp->flags & IP_VS_CONN_F_NFCT) &&
> @@ -1405,7 +1409,12 @@ static void ip_vs_conn_expire(struct timer_list *t)
> else
> call_rcu(&cp->rcu_head, ip_vs_conn_rcu_free);
> atomic_dec(&ipvs->conn_count);
> - return;
> + if (next) {
> + cp = ct;
> + my_cb = false;
> + goto repeat;
> + }
[Severity: High]
With my_cb now false, every controller in the chain goes through
ip_vs_conn_unlink(ct, false). Can the failure path there leave ct with a
saturated refcount?
ip_vs_conn_unlink() drops the count to zero before it revalidates:
net/netfilter/ipvs/ip_vs_conn.c:ip_vs_conn_unlink() {
if (!refcount_dec_if_one(&cp->refcnt))
return false;
...
if (atomic_read(&cp->n_control) ||
(!timer_delete(&cp->timer) && !my_cb)) {
...
refcount_inc(&cp->refcnt);
return false;
}
...
}
Calling refcount_inc() on zero goes through __refcount_add(). That sees
old == 0 and calls refcount_warn_saturate(r, REFCOUNT_ADD_UAF), which
warns "addition on 0; use-after-free". The counter is left at
REFCOUNT_SATURATED instead of going back to 1.
The new loop seems to reach this in two ways:
- ct's own timer callback is already running on another CPU, so
timer_delete() returns 0 while my_cb is false.
- A packet takes ct with __ip_vs_conn_get(), then calls
ip_vs_control_add() and ip_vs_conn_put(), between the n_control
check at repeat: and refcount_dec_if_one(). This can also happen on
the plain timer path of a persistence template while a new client
connection arrives.
Once the counter is saturated, refcount_dec_if_one() can never succeed,
but __ip_vs_conn_get() still does. The connection keeps going through
expire_later and getting re-armed, and it is never freed.
Wouldn't ip_vs_conn_flush() then loop forever during netns cleanup?
net/netfilter/ipvs/ip_vs_conn.c:ip_vs_conn_flush() {
...
if (atomic_read(&ipvs->conn_count) != 0) {
schedule();
goto flush_again;
}
...
}
This restore path came from the preceding patch in the series, "ipvs:
wait the running timer cb on conn deletion", and this commit message
relies on it. The later patch, "ipvs: reject FTP control ports as data
ports", doesn't touch ip_vs_conn.c.
Every lookup uses refcount_inc_not_zero(). Would refcount_set(&cp->refcnt,
1) be a safe way to restore the count here?
> + goto out;
> }
>
> expire_later:
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1790146910.git.zihanx%40nebusec.ai
