Hello,
On Sun, 27 Sep 2026, [email protected] wrote:
> 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.
With the pending patch "ipvs: do not create invisible templates"
we should disallow ONE_PACKET for templates. Other patches do the same
for the SYNC conns. In short, we should not see ONE_PACKET for
templates and in the cp->control chain. Hopefully, the mentioned patches
should go before this patchset.
> > }
> > - 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?
This is solved in patch 1 from v5
> > + goto out;
> > }
> >
> > expire_later:
>
> [ ... ]
>
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1790146910.git.zihanx%40nebusec.ai
>
Regards
--
Julian Anastasov <[email protected]>
