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

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

On 10/14/26 11:18 PM, Heming Zhao wrote:
> 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@vger.kernel.org>
>> Signed-off-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> 
> The code looks good to me.
> Reviewed-by: Heming Zhao <heming.zhao@suse.com>
> 
> btw, how do you find the issue?
> 

This is found when I review patch:
  ocfs2/dlm: hold a lock reference across grant AST enqueue

Since they address difference flow, so I send it as a separate patch.

Thanks,
Joseph