Re: [PATCH] ocfs2/dlm: hold the lookup reference across convert AST enqueue

[Heming Zhao]

On Wed, Oct 14, 2026 at 07:23:21PM +0800, Joseph Qi wrote:
> dlm_convert_lock_handler() drops the reference it took when it found the
> lock on res->granted at its leave label, one statement before it calls
> dlm_queue_ast().  dlm_free_dead_locks() takes res->spinlock and drops the
> list and base references, so a peer death landing in that gap leaves the
> handler's own put as the one that frees the lock, and __dlm_queue_ast()
> then reads lock->lockres from freed memory.
> 
> The handler is registered next to dlm_create_lock_handler(), so a convert
> message buffered on a dying peer's socket arrives on the same o2net
> receive work that o2net_disconnect_node() does not wait for.  The two can
> interleave as follows, with the reference counts as they are before this
> fix:
> 
>   Socket data-ready       Node-down thread      Receive worker
>   -----------------       ----------------      --------------
>   o2net_data_ready()
>     select sc under
>     callback read lock
>                           o2net_disconnect_node()
>                             queue shutdown
>                             begin flush; new color
>     queue sc_rx_work
>     in the new color
>     release callback lock
>                             shutdown socket
>                             flush returns
>                                                 read buffered convert_lock
>                                                 dlm_convert_lock_handler()
>                                                   find lock on res->granted
>                                                   dlm_lock_get()  refs = 3
>                                                   lock res->spinlock
>                                                   __dlmconvert_master()
>                                                     call_ast = 1
>                                                   unlock res->spinlock
>                           DLM node-down cleanup
>                             lock res->spinlock
>                             dlm_free_dead_locks()
>                               unlink lock
>                               put list ref  refs = 2
>                               put base ref  refs = 1
>                             unlock res->spinlock
>                                                 dlm_lock_put()  refs = 0
>                                                   free lock
>                                                 dlm_queue_ast()
>                                                   __dlm_queue_ast()
>                                                   read lock->lockres
>                                                   (use-after-free)
> 
> __dlmconvert_master() only moves the lock between lists, so the count
> entering the gap is base, list and lookup.  Drop the lookup reference
> after the AST is queued instead of before it and the race ends with the
> one __dlm_queue_ast() took, which dlm_flush_asts() releases later with
> ast_pending cleared and ast_list unlinked.  No extra get is needed, and
> the normal case ends with the same count as before.
> 
> dlmconvert_master() has the same shape but is only reached from dlmlock()
> for a lock this node owns, which dlm_free_dead_locks() never matches.
> 
> Fixes: 2c5c54aca9d0 ("ocfs2/dlm: Add missing dlm_lock_put()s")
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Joseph Qi <joseph.qi@xxxxxxxxxxxxxxxxx>

The code looks good to me.
Reviewed-by: Heming Zhao <heming.zhao@xxxxxxxx>

btw, how do you find the issue?

> ---
>  fs/ocfs2/dlm/dlmconvert.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ocfs2/dlm/dlmconvert.c b/fs/ocfs2/dlm/dlmconvert.c
> index 450d46eefab32..8a828448ba958 100644
> --- a/fs/ocfs2/dlm/dlmconvert.c
> +++ b/fs/ocfs2/dlm/dlmconvert.c
> @@ -534,15 +534,19 @@ int dlm_convert_lock_handler(struct o2net_msg *msg, u32 len, void *data,
>  	}
>  
>  leave:
> -	if (lock)
> -		dlm_lock_put(lock);
> -
>  	/* either queue the ast or release it, if reserved */
>  	if (call_ast)
>  		dlm_queue_ast(dlm, lock);
>  	else if (ast_reserved)
>  		dlm_lockres_release_ast(dlm, res);
>  
> +	/*
> +	 * Keep the lock alive through AST enqueue: a concurrent
> +	 * dlm_free_dead_locks() drops the list and base references.
> +	 */
> +	if (lock)
> +		dlm_lock_put(lock);
> +
>  	if (kick_thread)
>  		dlm_kick_thread(dlm, res);
>  
> -- 
> 2.39.3
>