Re: [PATCH nf v5 2/3] ipvs: avoid stack overflow from recursive connection expiration

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 1 · High: 0 · 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).

- [Critical] ip_vs_conn_expire()'s chain walk (`cp = ct; my_cb = false;
  goto repeat`) can free a ONE_PACKET controller immediately while its…
- [Low] This patch carries Cc: stable and Fixes: f9200a52eedf, but it
  depends on the preceding series commit "ipvs: wait the running timer…

--

ipvs: avoid stack overflow from recursive connection expiration

This commit replaces the recursive ip_vs_conn_del(ct) call in
ip_vs_conn_expire() with an iterative walk. After the current connection
is freed, the walk continues with its controller. The whole walk runs
under one rcu_read_lock(), and each following controller is handled in
deletion mode (my_cb = false).

> diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> index eac185496a8a6..ec5c0c8ecde74 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))) {

[Severity: Low]
This patch has Cc: stable and Fixes: f9200a52eedf, but it depends on the
previous commit, "ipvs: wait the running timer cb on conn deletion". That
commit adds my_cb, the tagged timer pointer in ip_vs_conn_del(),
ip_vs_conn_unlink(cp, my_cb) and the guarded expire_later path.

The prerequisite has a Fixes: tag but no Cc: stable. This commit message
only calls it "the preceding refcount fix", without its subject or a
dependency note.

If stable picks up this patch alone, will it fail to apply or build?
Could a manual backport also lose the deletion-mode revalidation that this
walk relies on?

Would it make sense to add Cc: stable to the prerequisite and name it
here, for example with a "# depends on" note?

>  		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;
>  			}
> -			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: Critical]
Can this walk free a ONE_PACKET controller while its timer is still
queued?

The commit message says the walk stays under RCU and that the preceding
refcount fix handles the timer callback race. Neither seems to cover
IP_VS_CONN_F_ONE_PACKET conns.

For those conns, ip_vs_conn_unlink() returns before the
timer_delete()/my_cb revalidation:

net/netfilter/ipvs/ip_vs_conn.c:ip_vs_conn_unlink() {
    if (!refcount_dec_if_one(&cp->refcnt))
        return false;

    if (cp->flags & IP_VS_CONN_F_ONE_PACKET)
        return true;
    ...
}

ip_vs_conn_expire() then frees them without waiting for an RCU grace
period:

    if (cp->flags & IP_VS_CONN_F_ONE_PACKET)
        ip_vs_conn_rcu_free(&cp->rcu_head);

A ONE_PACKET template with an ordinary child looks reachable:

- ONE_PACKET is part of IP_VS_CONN_F_DEST_MASK, so __ip_vs_update_dest()
  accepts it from userspace.
- ip_vs_bind_dest() only strips it for non-UDP conns, so UDP persistence
  templates keep it.
- ip_vs_sched_persist() reads dest->conn_flags separately for the
  template and for the child. A concurrent dest edit can leave a hashed
  ordinary child cp controlled by a ONE_PACKET template ct.

While cp exists, ct's expire_later path keeps re-arming ct->timer through
__ip_vs_conn_put_timer(). For ONE_PACKET conns that function uses a
timeout of 0.

Suppose cp is then deleted, for example by ip_vs_conn_flush() during
netns teardown:

ip_vs_conn_del(cp)
  ip_vs_conn_expire(cp, my_cb = false)
    ip_vs_control_del(cp)          /* ct->n_control drops to 0 */
    next = true                    /* new template is not ASSURED */
    cp = ct; my_cb = false; goto repeat
    ip_vs_conn_unlink(ct, false)   /* returns true, no timer_delete() */
    ip_vs_conn_rcu_free(ct)        /* ct freed, ct->timer still queued */

Would the timer base then touch the freed ct, and later call
ip_vs_conn_expire() on it?

There also seems to be a second variant. Suppose ct's own timer callback
on another CPU sees n_control == 0 first. It frees ct synchronously while
this walker still reads ct->n_control, ct->flags, ct->state and
ct->control. The walker then goes back to repeat with the freed ct.

Before "ipvs: wait the running timer cb on conn deletion", the old code
had two protections:

    bool has_ref = !cp->timeout && __ip_vs_conn_get(ct);
    ...
    ip_vs_conn_del_put(ct);

ct was pinned before ip_vs_control_del(), and ip_vs_conn_del_put() only
went ahead if timer_delete() on ct's timer succeeded. That commit removed
both, and this patch keeps that behaviour in the iterative walk. The
remaining patch in the series only touches ip_vs_ftp.c.

A related point: ip_vs_conn_flush() and ip_vs_expire_nodest_conn_flush()
read cp_c->n_control after ip_vs_conn_del(cp), and rely on RCU for that:

    /* Rely on RCU grace period while accessing cp after ip_vs_conn_del */

Could that read also hit freed memory when cp_c is a ONE_PACKET
controller that this walk freed synchronously?

> +		}
> +		goto out;
>  	}
>  
>    expire_later:

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1790266803.git.zihanx%40nebusec.ai