Re: [Ocfs2-devel] [bug report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list

[Joseph Qi via Ocfs2-devel <ocfs2-devel@oss.oracle.com>]

On 3/11/22 9:50 AM, Joseph Qi wrote:
> 
> 
> On 3/10/22 9:39 PM, Dan Carpenter wrote:
>> On Thu, Mar 10, 2022 at 11:13:05AM +0800, Joseph Qi wrote:
>>>>>>     557         }
>>>>>>     558 
>>>>>>     559         list_for_each_entry(res, track_list, tracking) {
>>>>>>     560                 if (&res->tracking == &dlm->tracking_list)
>>>>>>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>>> This should never be possible.  How is it possible?  If
>>>>>> &dlm->tracking_list is the list head the it's not possible without
>>>>>> memory corruption.  If &oldres->tracking is the list head then I do not
>>>>>> see how it is possible without memory corruption.  We can't mix different
>>>>>> types of list entries on the same list head?>
>>>>> In case of oldres, and the iterator points to dlm_ctxt.
>>>>> In this case, the lockres is not a valid one.
>>>>
>>>> That doesn't make sense.  :/  This condition is doing pointer math.
>>>> The offset of ->tracking is 136 bytes and ->tracking list is 88 bytes
>>>> into the dlm struct.
>>>>
>>> Now track_list is oldres->tracking, which is already linked to
>>> dlm->tracking_list?
>>
>> Are you saying or are you guessing?  :P
>>
> Honestly speaking, since these are debug code and not used often,
> I'm not quite sure it's the case described above.
> I'll dig it more later.
> 
Say there are totally 3 lockres now, and they are linked to dlm->tracking_list
when initializing:
head -> lockresA -> lockresB -> lockresC

now dump lockres:
cat /sys/kernel/debug/o2dlm/<domain>/locking_state

Round1:
oldres is NULL, track_list is dlm->tracking_list
dump lockresA

Round2:
oldres is lockresA, track_list is lockresA->tracking
put ref of lockresA
dump lockresB

Round3:
oldres is lockresB, track_list is lockresB->tracking
put ref of lockresB
dump lockresC

Round4:
oldres is lockresC, track_list is lockresC->tracking
now it is the end of tracking list
put ref of lockresC

Thanks,
Joseph

>> It's not impossible to set this condition up so that it's true.  But
>> it's bug if someone does that.
>>
>> I really think that condition can be deleted.  If you look at the commit
>> which added it b0d4f817ba5d ("ocfs2/dlm: Fix race in adding/removing
>> lockres' to/from the tracking list") the it's easy to imagine that it
>> was a copy and pasted pasted by mistake.
>> You may test your changes simply by:
> mkfs.ocfs2 -b 4k -C 1M -T datafiles /dev/vdc
> mount /dev/vdc /mnt/ocfs2
> cat /sys/kernel/debug/o2dlm/<domain>/locking_state
> 
> Thanks,
> Joseph
> 
>> Or another possibility is that it was debug code that was committed
>> accidentally.  After all if you remove the locking a delete the last
>> entry at the right time then it would be easy enough for the condition
>> to be true.  Hopefully, these days the locking prevents that condition
>> from being possible.
>>
>> regards,
>> dan carpenter

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel