Re: [PATCH net 02/15] ipvs: wait grace period while reusing dest from trash

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · 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] ipvs: wait for RCU grace period before calling
  ip_vs_start_estimator()
- [Low] The comment on the new `rcu_state` field in `struct ip_vs_dest`
  (include/net/ip_vs.h:1024) reads `/* GP cookie when trashed */`, but…

--

ipvs: wait grace period while reusing dest from trash

This patch fixes looping readers of the destination list. The loop happens
when a dest is removed and re-added through the destination trash before
an RCU grace period has elapsed. A grace-period cookie is saved with
get_state_synchronize_rcu() when dests are unlinked, and
cond_synchronize_rcu() is called before the dest is relinked. Dest edits
that unhash and rehash an entry in rs_table use the same mechanism.

Link: https://patch.msgid.link/cover.1789117690.git.zzyy19904204639@163.com/

> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index 9a83bff32569d..50a2999beb01f 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -1021,7 +1021,9 @@ struct ip_vs_dest {
>  
>  	struct rcu_head		rcu_head;
>  	struct list_head	t_list;		/* in dest_trash */
> -	unsigned int		in_rs_table:1;	/* we are in rs_table */
> +	unsigned long		rcu_state;	/* GP cookie when trashed */
                                                   ^^^^

[Severity: Low]
This isn't a bug, but is this comment on struct ip_vs_dest::rcu_state
complete?

The patch also writes rcu_state in __ip_vs_update_dest() when a live dest
is edited and unhashed from rs_table. That dest stays on
svc->destinations and never goes to the trash:

net/netfilter/ipvs/ip_vs_ctl.c:__ip_vs_update_dest() {
    ...
		if (unlinked) {
			dest->rcu_state = get_state_synchronize_rcu();
			/* Need grace period in this or next dest update */
			dest->need_gp = 1;
		}
    ...
}

ip_vs_rs_hash() then reads that cookie through need_gp before
hlist_add_head_rcu().

So the field holds either the trash-time cookie read by ip_vs_add_dest(),
or the edit-time unhash cookie read by ip_vs_rs_hash().

Could the comment say something like "GP cookie when unlinked (trash or
rehash)" to cover both uses?

> +	unsigned int		in_rs_table:1,	/* we are in rs_table */
> +				need_gp:1;	/* Need GP on rehash */
>  };
>  
>  /* The scheduler object */

[ ... ]

> diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
> --- a/net/netfilter/ipvs/ip_vs_ctl.c
> +++ b/net/netfilter/ipvs/ip_vs_ctl.c
> @@ -1566,11 +1615,18 @@ ip_vs_add_dest(struct ip_vs_service *svc, struct ip_vs_dest_user_kern *udest)
>  
>  		ret = ip_vs_start_estimator(svc->ipvs, &dest->stats);
>  		/* On error put back dest into the trash */
> -		if (ret < 0)
> -			ip_vs_trash_put_dest(svc->ipvs, dest, dest->idle_start,
> -					     false);
> -		else
> +		if (ret < 0) {
> +			ip_vs_trash_put_back(svc->ipvs, dest);
> +		} else {
> +			/* Readers may still follow the n_list/d_list of the
> +			 * unlinked dest, wait before linking it again.
> +			 */
> +			cond_synchronize_rcu(dest->rcu_state);
> +			/* need_gp matters only on dest edit, reinit it */
> +			dest->need_gp = 0;
> +
>  			__ip_vs_update_dest(svc, dest, udest, 1);
> +		}
>  	} else {
>  		/*
>  		 * Allocate and initialize the dest structure

[Severity: High]
Does cond_synchronize_rcu() need to come before ip_vs_start_estimator()
here?

The comment covers n_list and d_list, but dest->stats.est.list is a
third RCU-linked node that gets reused on this path. When the dest was
deleted, __ip_vs_del_dest() called ip_vs_stop_estimator(). For an
estimator that kthread 0 had already moved to a tick chain
(est->ktid >= 0), that does hlist_del_rcu(&est->list) on
td->chains[cid] and does not wait for a grace period.

The estimator kthreads walk that chain without service_mutex. They do it
in ip_vs_chain_estimation() under rcu_read_lock() taken in
ip_vs_tick_estimation(). So a reader can still be positioned on this
est when the dest is taken back out of the trash.

ip_vs_add_dest() calls ip_vs_start_estimator() before the new grace
period wait, and that rewrites the node right away:

net/netfilter/ipvs/ip_vs_est.c:ip_vs_start_estimator() {
    ...
	if (ret >= 0)
		hlist_add_head(&est->list, &ipvs->est_temp_list);
	else
		INIT_HLIST_NODE(&est->list);
    ...
}

On success, a reader still on the old chain follows est->list.next onto
ipvs->est_temp_list. That list is not RCU protected, and
ip_vs_stop_estimator() uses plain hlist_del() for ktid < 0 entries. The
reader can therefore end up dereferencing LIST_POISON1.

On failure, next becomes NULL and the walk ends early, skipping the
remaining estimators on td->chains[cid]. The failure path also never
reaches cond_synchronize_rcu() at all.

This is the same unlink-then-relink-without-a-grace-period problem the
patch fixes for n_list and d_list. It is reachable with the same
delete/re-add sequence from netlink or sockopt, as long as estimator
kthreads are running.

Would it work to move cond_synchronize_rcu(dest->rcu_state) up to just
after ip_vs_trash_get_dest() returns a dest? The wait would then cover
est->list too, on both the success and error paths. The comment could
be updated to mention the estimator list as well.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103140308.591690-3-pablo%40netfilter.org