Re: [PATCH v2] ocfs2/cluster: quiesce peer slots before destroying the o2net workqueue

[Joseph Qi <joseph.qi@linux.alibaba.com>]

On 10/27/26 12:32 PM, Cen Zhang wrote:
> Listener shutdown must leave no peer work behind before it destroys the
> o2net workqueue. destroy_workqueue() drains the work that is queued, but
> delayed work whose timer has not expired is invisible to it and is
> queued onto the freed workqueue when the timer fires.
> 
> o2net_stop_listening() only disconnects peers that are still in the node
> table, and o2net_disconnect_node() cancels the delayed work without
> waiting for a running instance. Several paths rearm it afterwards.
> 
> Node removal disconnects a peer before taking it out of the table. A
> heartbeat-up callback in between sees the persistent error and queues
> the connect and expiry work again, and the listener shutdown then skips
> the peer because it is no longer in the table:
> 
>   Peer removal                       Heartbeat / listener shutdown
>   ------------                       -----------------------------
>   disconnect peer; cancel work
>                                      heartbeat-up: rearm delayed work
>   remove peer from node table
>                                      stop listener
>                                        synchronize heartbeat callbacks
>                                        skip peer absent from node table
>                                        destroy o2net workqueue
>                                      delayed-work timer expires
>                                        queue on freed workqueue
> 
> o2net_connect_expired() may be running while the disconnect cancels;
> once the disconnect drops nn_lock it queues itself and the connect work
> again, and the flush does not wait for those timers.
> 
> o2net_start_connect() decides under nn_lock whether to connect, drops
> the lock to create the socket, and attaches the socket without looking
> again. A disconnect in between is undone: the socket stays attached
> with its callbacks armed, and o2net_set_nn_state() rearms the expiry
> work after it has been cancelled.
> 
> Set o2net_shutting_down for the duration of the shutdown. While it is
> set, o2net_set_nn_state() rearms no work, the connect worker repeats its
> check under nn_lock before attaching the socket and backs off, and the
> accept path refuses a new socket. With no rearm left, cancel the
> connect-expiry, connect and still-up work of every peer slot
> synchronously after the disconnect; expiry queues connect work, so it
> is cancelled first.
> 
> Those cancellations can remove the only pending connection attempt of a
> slot whose persistent error a rearm has already cleared. Leave every
> slot the way a disconnect does, with nn_timeout reset and -ENOTCONN as
> the persistent error, so that the next heartbeat-up after a restart
> reconnects and senders fail instead of waiting for a connection that
> nothing will establish.
> 
> KASAN report, only the relevant frames are kept:
> 
> BUG: KASAN: slab-use-after-free in __queue_work+0x1189/0x1600
> Read of size 4 at addr ffff8881169069c0 by task swapper/2/0
> 
> Call Trace:
>  <IRQ>
>  __queue_work+0x1189/0x1600
>  call_timer_fn+0x176/0x540
>  </IRQ>
> 
> Allocated by task 486:
>  __kmalloc_noprof+0x2de/0x780
>  __alloc_workqueue+0x61/0x17a0
>  alloc_workqueue_noprof+0xcc/0x130
>  o2net_start_listening+0x139/0x7c0
>  o2nm_node_local_store+0x4c0/0x810
>  configfs_write_iter+0x2f9/0x4f0
>  vfs_write+0x639/0x1010
>  ksys_write+0x111/0x200
> 
> Freed by task 485:
>  kfree+0x308/0x580
>  rcu_core+0x6be/0x1ed0
> 
> Last potentially related work creation:
>  __call_rcu_common.constprop.0+0x80/0x280
>  pwq_release_workfn+0x740/0xe10
> 
> The buggy address belongs to the object at ffff888116906800
>  which belongs to the cache kmalloc-1k of size 1024
> The buggy address is located 448 bytes inside of
>  freed 1024-byte region [ffff888116906800, ffff888116906c00)
> 
> Fixes: 98211489d414 ("[PATCH] OCFS2: The Second Oracle Cluster Filesystem")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Cen Zhang <zzzccc427@gmail.com>
> ---
> Changes in v2:
> - Add o2net_shutting_down: o2net_set_nn_state() rearms nothing while it is
>   set, the connect worker rechecks under nn_lock before attaching its socket
>   and the accept path refuses a new socket, so an in-flight connect or
>   expiry can no longer rearm the timers after the synchronous cancellations
>   (Sashiko review).
> - Leave every slot with nn_timeout reset and -ENOTCONN after the
>   cancellations, so a later heartbeat-up reconnects and senders fail instead
>   of waiting on a slot whose persistent error a rearm had cleared (Sashiko
>   review).
> - Subject changed accordingly.
> 
> Link to v1: https://lore.kernel.org/r/pm-ocfs2-objects-candidate-0090-v6-127847d463212a41b773@gmail.com
> 
>  fs/ocfs2/cluster/tcp.c | 86 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 69 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ocfs2/cluster/tcp.c b/fs/ocfs2/cluster/tcp.c
> index 8b3830b601ba..eb4628839eef 100644
> --- a/fs/ocfs2/cluster/tcp.c
> +++ b/fs/ocfs2/cluster/tcp.c
> @@ -114,6 +114,8 @@ static DEFINE_MUTEX(o2net_wq_mutex);
>  static DECLARE_COMPLETION(o2net_wq_destroyed);
>  /* Heartbeat callbacks stay registered across local-node off/on. */
>  static bool o2net_listening;
> +/* set while o2net_stop_listening() quiesces the peer slots */
> +static bool o2net_shutting_down;
>  static struct work_struct o2net_listen_work;
>  
>  static struct o2hb_callback_func o2net_hb_up, o2net_hb_down;
> @@ -553,11 +555,14 @@ static void o2net_set_nn_state(struct o2net_node *nn,
>  		       SC_NODEF_ARGS(sc));
>  	}
>  
> -	/* trigger the connecting worker func as long as we're not valid,
> +	/*
> +	 * trigger the connecting worker func as long as we're not valid,
>  	 * it will back off if it shouldn't connect.  This can be called
>  	 * from node config teardown and so needs to be careful about
> -	 * the work queue actually being up. */
> -	if (!valid && o2net_wq) {
> +	 * the work queue actually being up.  Once the listener shutdown
> +	 * has started cancelling, nothing may be rearmed.
> +	 */
> +	if (!valid && o2net_wq && !READ_ONCE(o2net_shutting_down)) {
>  		unsigned long delay;
>  		/* delay if we're within a RECONNECT_DELAY of the
>  		 * last attempt */
> @@ -1578,6 +1583,23 @@ static void o2net_sc_postpone_idle(struct o2net_sock_container *sc)
>   * having a connect attempt fail, etc. This centralizes the logic which decides
>   * if a connect attempt should be made or if we should give up and all future
>   * transmit attempts should fail */
> +/*
> + * Called with nn->nn_lock held.  See if we already have one pending or
> + * have given up.  For nn_timeout, it is set when we close the connection
> + * because of the idle time out.  So it means that we have at least
> + * connected to that node successfully once, now try to connect to it
> + * again.  Nothing may connect once the listener shutdown has started.
> + */
> +static bool o2net_nn_should_connect(struct o2net_node *nn)
> +{
> +	unsigned int timeout = atomic_read(&nn->nn_timeout);
> +
> +	if (READ_ONCE(o2net_shutting_down) || nn->nn_sc)
> +		return false;
> +	return !nn->nn_persistent_error ||
> +	       (nn->nn_persistent_error == -ENOTCONN && timeout != 0);
> +}
> +
>  static void o2net_start_connect(struct work_struct *work)
>  {
>  	struct o2net_node *nn =
> @@ -1587,7 +1609,6 @@ static void o2net_start_connect(struct work_struct *work)
>  	struct socket *sock = NULL;
>  	struct sockaddr_in myaddr = {0, }, remoteaddr = {0, };
>  	int ret = 0, stop;
> -	unsigned int timeout;
>  	unsigned int nofs_flag;
>  
>  	/*
> @@ -1609,17 +1630,7 @@ static void o2net_start_connect(struct work_struct *work)
>  		goto out;
>  
>  	spin_lock(&nn->nn_lock);
> -	/*
> -	 * see if we already have one pending or have given up.
> -	 * For nn_timeout, it is set when we close the connection
> -	 * because of the idle time out. So it means that we have
> -	 * at least connected to that node successfully once,
> -	 * now try to connect to it again.
> -	 */
> -	timeout = atomic_read(&nn->nn_timeout);
> -	stop = (nn->nn_sc ||
> -		(nn->nn_persistent_error &&
> -		(nn->nn_persistent_error != -ENOTCONN || timeout == 0)));
> +	stop = !o2net_nn_should_connect(nn);
>  	spin_unlock(&nn->nn_lock);
>  	if (stop)
>  		goto out;
> @@ -1658,9 +1669,16 @@ static void o2net_start_connect(struct work_struct *work)
>  	tcp_sock_set_nodelay(sc->sc_sock->sk);
>  	tcp_sock_set_user_timeout(sock->sk, O2NET_TCP_USER_TIMEOUT);
>  
> -	o2net_register_callbacks(sc->sc_sock->sk, sc);
> -
>  	spin_lock(&nn->nn_lock);
> +	/*
> +	 * nn_lock was dropped while the socket was created.  A disconnect
> +	 * or the listener shutdown in between must not gain this socket.
> +	 */
> +	if (!o2net_nn_should_connect(nn)) {
> +		spin_unlock(&nn->nn_lock);
> +		goto out;
> +	}
> +	o2net_register_callbacks(sc->sc_sock->sk, sc);
>  	/* handshake completion will set nn->nn_sc_valid */
>  	o2net_set_nn_state(nn, sc, 0, 0);
>  	spin_unlock(&nn->nn_lock);
> @@ -1954,6 +1972,12 @@ static int o2net_accept_one(struct socket *sock, int *more)
>  	new_sock = NULL;
>  
>  	spin_lock(&nn->nn_lock);
> +	/* a slot that is being quiesced takes no new socket */
> +	if (READ_ONCE(o2net_shutting_down)) {
> +		spin_unlock(&nn->nn_lock);
> +		ret = -ESHUTDOWN;
> +		goto out;
> +	}
>  	atomic_set(&nn->nn_timeout, 0);
>  	o2net_set_nn_state(nn, sc, 0, 0);
>  	spin_unlock(&nn->nn_lock);
> @@ -2179,6 +2203,30 @@ int o2net_start_listening(struct o2nm_node *node)
>  	return ret;
>  }
>  
> +/*
> + * Called with o2net_shutting_down set.  Cancel the delayed work of a peer
> + * slot synchronously: work waiting on its timer is invisible to a flush
> + * and would be queued on the destroyed workqueue.  Then leave the slot
> + * the way a disconnect does, so that the next heartbeat-up after a
> + * restart reconnects and senders fail instead of waiting.
> + */
> +static void o2net_quiesce_nn(struct o2net_node *nn)
> +{
> +	/* whoever takes nn_lock from now on sees o2net_shutting_down */
> +	spin_lock(&nn->nn_lock);
> +	spin_unlock(&nn->nn_lock);
> +
> +	/* expiry queues connect work, so cancel it first */
> +	cancel_delayed_work_sync(&nn->nn_connect_expired);
> +	cancel_delayed_work_sync(&nn->nn_connect_work);
> +	cancel_delayed_work_sync(&nn->nn_still_up);
> +
> +	spin_lock(&nn->nn_lock);
> +	atomic_set(&nn->nn_timeout, 0);
> +	o2net_set_nn_state(nn, NULL, 0, -ENOTCONN);
> +	spin_unlock(&nn->nn_lock);
> +}

o2net_set_nn_state() still has a queue_delayed_work() that the new flag
doesn't gate. So o2net_quiesce_nn() cancels nn_still_up and afterwards
calls o2net_set_nn_state(), which queues it again.

> +
>  /* again, o2nm_this_node() doesn't work here as we're involved in
>   * tearing it down */
>  void o2net_stop_listening(struct o2nm_node *node)
> @@ -2190,6 +2238,7 @@ void o2net_stop_listening(struct o2nm_node *node)
>  	BUG_ON(o2net_listen_sock == NULL);
>  
>  	WRITE_ONCE(o2net_listening, false);
> +	WRITE_ONCE(o2net_shutting_down, true);
>  	o2hb_synchronize_callbacks();
>  
>  	/* stop the listening socket from generating work */
> @@ -2200,15 +2249,18 @@ void o2net_stop_listening(struct o2nm_node *node)
>  
>  	for (i = 0; i < ARRAY_SIZE(o2net_nodes); i++) {
>  		struct o2nm_node *node = o2nm_get_node_by_num(i);
> +
>  		if (node) {
>  			o2net_disconnect_node(node);
>  			o2nm_node_put(node);
>  		}
> +		o2net_quiesce_nn(o2net_nn_from_num(i));

o2net_idle_timer() queues the same work from softirq with no nn_lock and no
flag check, sets nn_timeout back to 1, and re-arms sc_idle_timeout — which
quiesce never deletes.

Thanks,
Joseph

>  	}
>  
>  	/* finish all work and tear down the work queue */
>  	mlog(ML_KTHREAD, "waiting for o2net thread to exit....\n");
>  	o2net_destroy_wq();
> +	WRITE_ONCE(o2net_shutting_down, false);
>  
>  	sock_release(o2net_listen_sock);
>  	o2net_listen_sock = NULL;