Re: [PATCH v2 1/2] tipc: skip socketless connections in topsrv teardown

[netdev-bot+sashiko@kernel.org]

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

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

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] This patch is not complete on its own.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the same
  livelock already exists for socket-backed conns.
- [High] This problem wasn't introduced by this patch, but
  tipc_topsrv_stop() calls conn_get() (plain kref_get()) on any conn…

--

tipc: skip socketless connections in topsrv teardown

This patch makes tipc_topsrv_stop() skip tipc_conn_close() for kernel
subscription connections, which have no socket. These connections can
stay in conn_idr after unsubscribe while send work is still pending.
Calling tipc_conn_close() on them dereferences a NULL con->sock during
netns teardown.

> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index af530c9ed8402..01dd34b758f4b 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -706,7 +706,8 @@ static void tipc_topsrv_stop(struct net *net)
>  		if (con) {
>  			conn_get(con);
>  			spin_unlock_bh(&srv->idr_lock);
> -			tipc_conn_close(con);
> +			if (con->sock)
> +				tipc_conn_close(con);
>  			conn_put(con);
>  			spin_lock_bh(&srv->idr_lock);
>  		}

[Severity: High]
This avoids the NULL dereference. In the scenario from the commit
message, though, can netns teardown still stall and then hit a
use-after-free on con?

In that scenario, tipc_topsrv_queue_evt() passes its lookup reference
to con->swork:

	if (queue_work(srv->send_wq, &con->swork))
		return;

tipc_topsrv_kern_unsubscr() then drops its two references:

	test_and_clear_bit(CF_CONNECTED, &con->flags);
	tipc_conn_delete_sub(con, NULL);
	conn_put(con);
	conn_put(con);

After that, the pending swork holds the last reference.

At this commit the loop in tipc_topsrv_stop() is:

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

With the new check, the socketless conn gets conn_get() and conn_put(),
which leaves the refcount at 1. The loop then retakes idr_lock and moves
on.

idr_find() returns NULL for every higher id. So idr_lock stays held with
BH disabled and no cond_resched().

Meanwhile the send_wq worker runs:

tipc_conn_send_work()
    conn_put(con)
        tipc_conn_kref_release()
            spin_lock_bh(&s->idr_lock);   <- blocks on the stop loop

tipc_conn_kref_release() is the only place that calls idr_remove() and
decrements s->idr_in_use. So the loop only ends after int id wraps
around, which takes about 2^32 lookups under the spinlock. Won't that
cause soft lockup and RCU stall warnings?

When id gets back to the conn's slot, conn_get() raises a zero refcount.
After spin_unlock_bh(), the release path calls kfree(con). The stop path
then reads con->sock and calls conn_put() on freed memory.

The path is reachable from an unprivileged user in a user+net namespace:

  1. TIPC_GROUP_JOIN (tipc_group_create()->tipc_topsrv_kern_subscr()).
  2. Leave the group or close the socket while topology events are
     queued (tipc_group_delete()->tipc_topsrv_kern_unsubscr()).
  3. Tear down the netns.

The later patch in this series, "tipc: fix use-after-free in netns
teardown", fixes this. Its loop uses idr_get_next_ul() with
kref_get_unless_zero() and drops idr_lock with cond_resched() on every
iteration. Remaining work references are left to
tipc_topsrv_work_stop(). So the problem only exists at this commit.

The two patches carry different Fixes: tags. Could that cause trouble
for bisection, or for stable trees that take this patch without the
follow-up?

[Severity: High]
This isn't a bug introduced by this patch, but the same livelock looks
possible for socket-backed conns too.

Suppose recv or send work already cleared CF_CONNECTED through
tipc_conn_close(), while rwork or swork still holds a reference. The
call to tipc_conn_close() from the stop loop then returns early without
a conn_put():

	disconnect = test_and_clear_bit(CF_CONNECTED, &con->flags);
	[ ... ]
	if (!disconnect)
		return;

The loop's own conn_get()/conn_put() pair leaves the work's reference
in place. The loop then scans empty ids under idr_lock, waiting for
idr_in_use to reach 0.

The worker's final conn_put()->tipc_conn_kref_release() blocks in
spin_lock_bh(&s->idr_lock). Can idr_in_use ever drop while the stop
loop spins?

The later patch in this series, "tipc: fix use-after-free in netns
teardown", also fixes this. With it, the loop ends when
idr_get_next_ul() returns NULL and drops idr_lock on every iteration.

[Severity: High]
This is a pre-existing issue, but can the conn_get() here bring back a
zero refcount? conn_get() is a plain kref_get():

static void conn_get(struct tipc_conn *con)
{
	kref_get(&con->kref);
}

tipc_conn_lookup() uses kref_get_unless_zero() instead.

Say a final conn_put() from rwork, swork or tipc_conn_close() runs on
another CPU while the stop loop holds idr_lock. The conn is then still
visible in conn_idr with a refcount of 0:

CPU1                              CPU2 tipc_topsrv_stop()
                                  spin_lock_bh(&srv->idr_lock)
conn_put(con)
  tipc_conn_kref_release()
    spin_lock_bh(&s->idr_lock)
      (blocks)
                                  con = idr_find(&srv->conn_idr, id)
                                  conn_get(con)    <- refcount from 0
                                  spin_unlock_bh(&srv->idr_lock)
    idr_remove(&s->conn_idr, ...)
    [ ... ]
    kfree(con)
                                  if (con->sock)   <- freed memory
                                      tipc_conn_close(con)
                                  conn_put(con)

Before this patch, tipc_conn_close() already dereferenced con->sock->sk
on the same freed object. The new con->sock read touches the same
memory.

This seems to match the KASAN slab-use-after-free described in the next
patch in the series, "tipc: fix use-after-free in netns teardown". That
patch replaces conn_get() with kref_get_unless_zero() under idr_lock
and sets con = NULL when it fails.

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