Re: [PATCH v2 2/2] tipc: fix use-after-free in netns teardown

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] After this patch, `struct tipc_topsrv::idr_in_use` is dead state.

--

tipc: fix use-after-free in netns teardown

This patch stops the listener and cancels the accept work before
tipc_topsrv_stop() walks conn_idr. The walk now goes forward with
idr_get_next_ul() and uses kref_get_unless_zero(), so it no longer takes
a reference on a connection whose count has already reached zero.
Connections are disconnected before the workqueues are drained.

> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 01dd34b758f4b..f3f32fd8134ac 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c

[ ... ]

>  	spin_lock_bh(&srv->idr_lock);
> -	for (id = 0; srv->idr_in_use; id++) {
> -		con = idr_find(&srv->conn_idr, id);
> +	for (id = 0; (con = idr_get_next_ul(&srv->conn_idr, &id)); id++) {

[Severity: Low]
This isn't a bug, but this loop condition was the only place that read
srv->idr_in_use. Is the field still needed after this change?

It is still declared and documented in struct tipc_topsrv:

 * @idr_in_use: amount of allocated identifier entry
 ...
	int idr_in_use;

It is also still written under idr_lock. It is set to 0 in
tipc_topsrv_start(), incremented in tipc_conn_alloc() and decremented in
tipc_conn_kref_release():

	s->idr_in_use++;
	...
	s->idr_in_use--;

Nothing reads it now.

I checked whether dropping the old termination condition removes a
guarantee that all connections are gone before kfree(srv), and it does
not appear to.

tipc_conn_close() unsubscribes synchronously via tipc_conn_delete_sub()
->tipc_sub_unsubscribe() before it drops its reference. That
unsubscribe serializes on sc->lock and calls timer_delete_sync(). The
get/put pairs in tipc_conn_data_ready() and tipc_conn_write_space() are
bounded by sk_callback_lock. The remaining final puts run from
rcv_wq/send_wq, and tipc_topsrv_work_stop() drains those.

The old loop also held idr_lock while stepping over empty ids, so it
could not act as a wait-for-empty barrier anyway.

Could the field and its kernel-doc entry be removed in this patch? If
it is kept, could the commit message say that the old idr_in_use
termination condition was dropped on purpose?

> +		if (!kref_get_unless_zero(&con->kref))
> +			con = NULL;
> +		spin_unlock_bh(&srv->idr_lock);
>  		if (con) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004210016.3051310-1-Jeremy.Jean%40oss.cyber.gouv.fr