Re: [PATCH nf v4 1/3] ipvs: wait the running timer cb on conn deletion

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 2

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] In ip_vs_conn_unlink() (net/netfilter/ipvs/ip_vs_conn.c), the
  new abort path tries to put the refcount back to 1 with…
- [Medium] In ip_vs_conn_unlink(), the new check of
  atomic_read(&cp->n_control) after the decrement has no acquire…
- [Medium] ip_vs_conn_unlink() now does refcount_dec_if_one() before its
  revalidation, and on the restore path the connection stays alive.
- [Low] Commit message accuracy.
- [Low] The comment in ip_vs_conn_expire_now() still refers to "the final
  timer_delete in ip_vs_conn_expire".

--

ipvs: wait the running timer cb on conn deletion

This patch changes ip_vs_conn_unlink() to drop the refcount from 1 to 0
first. It then rechecks n_control and timer_delete(), and if either check
fails it restores the refcount and returns false. ip_vs_conn_del() now
always calls ip_vs_conn_expire() with a tagged timer pointer to mark the
deletion path, and ip_vs_conn_del_put() is replaced by RCU protection for
the cp->control chain.

Link: https://sashiko.dev/#/patchset/cover.1789435989.git.zihanx%40nebusec.ai

> ipvs: wait the running timer cb on conn deletion

[Severity: Low]

This isn't a bug, but the subject says the deletion path waits for the
running timer callback. ip_vs_conn_unlink() doesn't wait, since there is no
timer_delete_sync(). It fails and leaves the work to a later timer run, as
the body describes. Would a word like "defer" describe this better?

> As ip_vs_conn_del*() run under RCU lock
> there is no risk accessing a freed connection by
> concurrent timer callback as Sashiko warns, may
> be only if our timer expires and we try to delete
> the cp->control chain.

This seems to contradict the sentence before it, which says the callback
risked accessing the deleted connection after it was freed. Could it be
reworded?

The body also doesn't say that ip_vs_conn_del() now calls
ip_vs_conn_expire() without first needing timer_delete() to succeed. Could
that be mentioned?

> diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
> index 6fa3e1dc534c3..32cfc02aa2912 100644
> --- a/net/netfilter/ipvs/ip_vs_conn.c
> +++ b/net/netfilter/ipvs/ip_vs_conn.c
> @@ -313,17 +313,34 @@ static inline int ip_vs_conn_hash(struct ip_vs_conn *cp)
>  /* Try to unlink ip_vs_conn from conn_tab.
>   * returns bool success.
>   */
> -static inline bool ip_vs_conn_unlink(struct ip_vs_conn *cp)
> +static inline bool ip_vs_conn_unlink(struct ip_vs_conn *cp, bool my_cb)
>  {

[ ... ]

> +	if (!refcount_dec_if_one(&cp->refcnt))
> +		return false;
> +

[Severity: Medium]

The decrement now happens before the revalidation. That means a conn that
is still hashed, and may end up staying alive, briefly has refcnt == 0.
Can lookups miss it during that window?

__ip_vs_conn_in_get() and ip_vs_ct_in_get() skip entries where
__ip_vs_conn_get() fails:

	if (__ip_vs_conn_get(cp)) {
		/* HIT */

Take a template or FTP control conn whose timer fires while it is gaining
a controlled conn. The n_control check below restores the refcount and the
conn stays alive.

A packet arriving in that window could miss the template and create a
duplicate one, possibly for a different real server. A mid-stream packet
could also miss its FTP control conn.

Before this patch, a successful refcount_dec_if_one() always led straight
to the unlink under the bucket lock. A failed lookup could only affect a
conn that was really going away.

>  	if (cp->flags & IP_VS_CONN_F_ONE_PACKET)
> -		return refcount_dec_if_one(&cp->refcnt);
> +		return true;
> +
> +	/* Revalidate after conn is excluded from traffic:
> +	 * - not controlling other conns
> +	 * - no pending/running timer callback
> +	 *
> +	 * And the winner is ...
> +	 */
> +	if (atomic_read(&cp->n_control) ||
> +	    (!timer_delete(&cp->timer) && !my_cb)) {
> +		/* Not me? Give the timer callback another chance, even
> +		 * if one is concurrently running during the conn deletion.
> +		 */
> +		refcount_inc(&cp->refcnt);
> +		return false;
> +	}

[Severity: High]

Does this refcount_inc() actually bring refcnt back to 1?
refcount_dec_if_one() has already moved it from 1 to 0, and refcount_t
does not allow an increment from 0:

include/linux/refcount.h:__refcount_add() {
	int old = atomic_fetch_add_relaxed(i, &r->refs);
	...
	if (unlikely(!old))
		refcount_warn_saturate(r, REFCOUNT_ADD_UAF);
	...
}

This prints "refcount_t: addition on 0; use-after-free".
refcount_warn_saturate() then sets refcnt to REFCOUNT_SATURATED, not 1.

>From then on, refcount_dec_if_one() can never succeed for this conn, so
neither the timer path nor ip_vs_conn_del() can free it.

When the timer runs, the __ip_vs_conn_get() in expire_later succeeds on
the saturated value, and __ip_vs_conn_put_timer() re-arms the timer for
60*HZ. The conn never reaches call_rcu(), and ipvs->conn_count is never
decremented. The dest, conntrack and app references are never dropped
either.

Wouldn't ip_vs_conn_flush() then loop forever at netns teardown?

	if (atomic_read(&ipvs->conn_count) != 0) {
		schedule();
		goto flush_again;
	}

There seem to be two ways to get here. The first is the race this patch
is meant to fix:

CPU X                         CPU Y
cp->timer fires               ip_vs_conn_del(cp)
ip_vs_conn_expire(cp)           ip_vs_conn_expire()
                                  ip_vs_conn_unlink(cp, false)
                                    refcount_dec_if_one() 1 -> 0
                                    timer_delete() returns 0
                                    refcount_inc() on refcnt 0

CPU Y can be in ip_vs_conn_flush(), ip_vs_random_dropentry() or the
expire_nodest_conn flush.

The second is a template or FTP control conn ct:

- Its timer passes the early n_control == 0 check.
- Meanwhile another CPU runs ip_vs_ct_in_get()->ip_vs_control_add() and
  then ip_vs_conn_put(ct).
- The timer's refcount_dec_if_one() then succeeds, but n_control is now
  non-zero.
- refcount_inc() then runs on 0 again.

Both cases in the commit message ("One of two things can happen when we
detect the running callback") assume refcnt goes back to 1.

Was refcount_set(&cp->refcnt, 1) intended here? While refcnt is 0,
refcount_inc_not_zero() and refcount_dec_if_one() fail in every other
context.

[Severity: Medium]

Is the atomic_read(&cp->n_control) recheck ordered after
refcount_dec_if_one()? That function is only a release cmpxchg:

lib/refcount.c:refcount_dec_if_one() {
	...
	return atomic_try_cmpxchg_release(&r->refs, &val, 0);
}

A control dependency on its result orders later stores but not later
loads. Unlike refcount_dec_and_test(), there is no
smp_acquire__after_ctrl_dep() here.

Consider a template ct with refcnt 2:

CPU B                               CPU A (ct timer callback)
                                    ip_vs_conn_expire(ct)
                                      n_control == 0 check passes
ip_vs_control_add(cp2, ct)
  atomic_inc(&ct->n_control)
ip_vs_conn_put(ct)
  mod_timer()
  refcnt 2 -> 1 (release)
                                    ip_vs_conn_unlink(ct, true)
                                      refcount_dec_if_one() 1 -> 0
                                      atomic_read(&ct->n_control)
                                        may still observe 0

On arm64 or powerpc, can ct then be unlinked and passed to call_rcu()
while cp2->control still points at it? When cp2 later expires,
ip_vs_control_del(cp2) would read and decrement ct->n_control in freed
memory.

Is an smp_acquire__after_ctrl_dep() (or smp_rmb()) needed after a
successful refcount_dec_if_one() and before reading n_control?

[ ... ]

> @@ -1356,26 +1364,21 @@ static void ip_vs_conn_expire(struct timer_list *t)
>  		goto expire_later;
>  
>  	/* Unlink conn if not referenced anymore */
> -	if (likely(ip_vs_conn_unlink(cp))) {
> +	if (likely(ip_vs_conn_unlink(cp, my_cb))) {
>  		struct ip_vs_conn *ct = cp->control;
>  
> -		/* delete the timer if it is activated by other users */
> -		timer_delete(&cp->timer);
> -

[Severity: Low]

This isn't a bug, but a comment in ip_vs_conn_expire_now() still refers to
this call:

	/* Using mod_timer_pending will ensure the timer is not
	 * modified after the final timer_delete in ip_vs_conn_expire.
	 */

With this timer_delete() removed, the one that remains is the conditional
call in ip_vs_conn_unlink(). Could the comment be updated to point there?

[ ... ]

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