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

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

[Dan Carpenter via Ocfs2-devel]

Hello OCFS Devs,

There is a big push to re-write list_for_each_entry() so that you
can't use the list iterator outside the loop.  I wrote a check for that
but this code has me puzzled.

The patch b0d4f817ba5d: "ocfs2/dlm: Fix race in adding/removing
lockres' to/from the tracking list" from Dec 16, 2008, leads to the
following Smatch static checker warning:

	fs/ocfs2/dlm/dlmdebug.c:573 lockres_seq_start()
	warn: iterator used outside loop: 'res'

fs/ocfs2/dlm/dlmdebug.c
    539 static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
    540 {
    541         struct debug_lockres *dl = m->private;
    542         struct dlm_ctxt *dlm = dl->dl_ctxt;
    543         struct dlm_lock_resource *oldres = dl->dl_res;
    544         struct dlm_lock_resource *res = NULL;
    545         struct list_head *track_list;
    546 
    547         spin_lock(&dlm->track_lock);
    548         if (oldres)
    549                 track_list = &oldres->tracking;
    550         else {
    551                 track_list = &dlm->tracking_list;
    552                 if (list_empty(track_list)) {
    553                         dl = NULL;
    554                         spin_unlock(&dlm->track_lock);
    555                         goto bail;
    556                 }

Why not do the list_empty() check after the else statement.  In the
current code if "&oldres->tracking" is empty it will lead to a crash.

    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?

    561                         res = NULL;
    562                 else
    563                         dlm_lockres_get(res);
    564                 break;

If we using list_first_entry() instead of open coding it then this
function becomes a lot simpler.  See patch below:

    565         }
    566         spin_unlock(&dlm->track_lock);
    567 
    568         if (oldres)
    569                 dlm_lockres_put(oldres);
    570 
    571         dl->dl_res = res;
    572 
--> 573         if (res) {
    574                 spin_lock(&res->spinlock);

Here is where the crash would happen if the &oldres->tracking list is
empty.

    575                 dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
    576                 spin_unlock(&res->spinlock);
    577         } else
    578                 dl = NULL;
    579 
    580 bail:
    581         /* passed to seq_show */
    582         return dl;
    583 }

Here is a patch which removes the "if (&res->tracking == &dlm->tracking_list)"
and uses list_first_entry_or_null().  It removes 13 lines of code.

regards,
dan carpenter

---
 fs/ocfs2/dlm/dlmdebug.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)

diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
index d442cf5dda8a..fb61cdfe0d06 100644
--- a/fs/ocfs2/dlm/dlmdebug.c
+++ b/fs/ocfs2/dlm/dlmdebug.c
@@ -547,37 +547,24 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
 	spin_lock(&dlm->track_lock);
 	if (oldres)
 		track_list = &oldres->tracking;
-	else {
+	else
 		track_list = &dlm->tracking_list;
-		if (list_empty(track_list)) {
-			dl = NULL;
-			spin_unlock(&dlm->track_lock);
-			goto bail;
-		}
-	}
 
-	list_for_each_entry(res, track_list, tracking) {
-		if (&res->tracking == &dlm->tracking_list)
-			res = NULL;
-		else
-			dlm_lockres_get(res);
-		break;
-	}
+	res = list_first_entry_or_null(track_list, struct dlm_lock_resource,
+				       tracking);
 	spin_unlock(&dlm->track_lock);
+	if (!res)
+		return NULL;
 
 	if (oldres)
 		dlm_lockres_put(oldres);
 
 	dl->dl_res = res;
 
-	if (res) {
-		spin_lock(&res->spinlock);
-		dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
-		spin_unlock(&res->spinlock);
-	} else
-		dl = NULL;
+	spin_lock(&res->spinlock);
+	dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
+	spin_unlock(&res->spinlock);
 
-bail:
 	/* passed to seq_show */
 	return dl;
 }
-- 
2.20.1


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

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

[Joseph Qi via Ocfs2-devel]

On 3/7/22 10:51 PM, Dan Carpenter via Ocfs2-devel wrote:
> Hello OCFS Devs,
> 
> There is a big push to re-write list_for_each_entry() so that you
> can't use the list iterator outside the loop.  I wrote a check for that
> but this code has me puzzled.
> 
> The patch b0d4f817ba5d: "ocfs2/dlm: Fix race in adding/removing
> lockres' to/from the tracking list" from Dec 16, 2008, leads to the
> following Smatch static checker warning:
> 
> 	fs/ocfs2/dlm/dlmdebug.c:573 lockres_seq_start()
> 	warn: iterator used outside loop: 'res'
> 
> fs/ocfs2/dlm/dlmdebug.c
>     539 static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
>     540 {
>     541         struct debug_lockres *dl = m->private;
>     542         struct dlm_ctxt *dlm = dl->dl_ctxt;
>     543         struct dlm_lock_resource *oldres = dl->dl_res;
>     544         struct dlm_lock_resource *res = NULL;
>     545         struct list_head *track_list;
>     546 
>     547         spin_lock(&dlm->track_lock);
>     548         if (oldres)
>     549                 track_list = &oldres->tracking;
>     550         else {
>     551                 track_list = &dlm->tracking_list;
>     552                 if (list_empty(track_list)) {
>     553                         dl = NULL;
>     554                         spin_unlock(&dlm->track_lock);
>     555                         goto bail;
>     556                 }
> 
> Why not do the list_empty() check after the else statement.  In the
> current code if "&oldres->tracking" is empty it will lead to a crash.
> 
lockres will be added into tracking_list during initialize.


>     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.
 
>     561                         res = NULL;
>     562                 else
>     563                         dlm_lockres_get(res);
>     564                 break;
> 
> If we using list_first_entry() instead of open coding it then this
> function becomes a lot simpler.  See patch below:
> 
>     565         }
>     566         spin_unlock(&dlm->track_lock);
>     567 
>     568         if (oldres)
>     569                 dlm_lockres_put(oldres);
>     570 
>     571         dl->dl_res = res;
>     572 
> --> 573         if (res) {
>     574                 spin_lock(&res->spinlock);
> 
> Here is where the crash would happen if the &oldres->tracking list is
> empty.
> 
>     575                 dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
>     576                 spin_unlock(&res->spinlock);
>     577         } else
>     578                 dl = NULL;
>     579 
>     580 bail:
>     581         /* passed to seq_show */
>     582         return dl;
>     583 }
> 
> Here is a patch which removes the "if (&res->tracking == &dlm->tracking_list)"
> and uses list_first_entry_or_null().  It removes 13 lines of code.
> 
> regards,
> dan carpenter
> 
> ---
>  fs/ocfs2/dlm/dlmdebug.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
> index d442cf5dda8a..fb61cdfe0d06 100644
> --- a/fs/ocfs2/dlm/dlmdebug.c
> +++ b/fs/ocfs2/dlm/dlmdebug.c
> @@ -547,37 +547,24 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
>  	spin_lock(&dlm->track_lock);
>  	if (oldres)
>  		track_list = &oldres->tracking;
> -	else {
> +	else
>  		track_list = &dlm->tracking_list;
> -		if (list_empty(track_list)) {
> -			dl = NULL;
> -			spin_unlock(&dlm->track_lock);
> -			goto bail;
> -		}
> -	}
>  
> -	list_for_each_entry(res, track_list, tracking) {
> -		if (&res->tracking == &dlm->tracking_list)
> -			res = NULL;
> -		else
> -			dlm_lockres_get(res);
> -		break;
> -	}
> +	res = list_first_entry_or_null(track_list, struct dlm_lock_resource,
> +				       tracking);
>  	spin_unlock(&dlm->track_lock);
> +	if (!res)
> +		return NULL;
>  
>  	if (oldres)
>  		dlm_lockres_put(oldres);
>  
>  	dl->dl_res = res;
>  
> -	if (res) {
> -		spin_lock(&res->spinlock);
> -		dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
> -		spin_unlock(&res->spinlock);
> -	} else
> -		dl = NULL;
> +	spin_lock(&res->spinlock);
> +	dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
> +	spin_unlock(&res->spinlock);
>  
> -bail:
>  	/* passed to seq_show */
>  	return dl;
>  }

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

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

[Dan Carpenter via Ocfs2-devel]

On Wed, Mar 09, 2022 at 03:46:11PM +0800, Joseph Qi wrote:
> 
> 
> On 3/7/22 10:51 PM, Dan Carpenter via Ocfs2-devel wrote:
> > Hello OCFS Devs,
> > 
> > There is a big push to re-write list_for_each_entry() so that you
> > can't use the list iterator outside the loop.  I wrote a check for that
> > but this code has me puzzled.
> > 
> > The patch b0d4f817ba5d: "ocfs2/dlm: Fix race in adding/removing
> > lockres' to/from the tracking list" from Dec 16, 2008, leads to the
> > following Smatch static checker warning:
> > 
> > 	fs/ocfs2/dlm/dlmdebug.c:573 lockres_seq_start()
> > 	warn: iterator used outside loop: 'res'
> > 
> > fs/ocfs2/dlm/dlmdebug.c
> >     539 static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
> >     540 {
> >     541         struct debug_lockres *dl = m->private;
> >     542         struct dlm_ctxt *dlm = dl->dl_ctxt;
> >     543         struct dlm_lock_resource *oldres = dl->dl_res;
> >     544         struct dlm_lock_resource *res = NULL;
> >     545         struct list_head *track_list;
> >     546 
> >     547         spin_lock(&dlm->track_lock);
> >     548         if (oldres)
> >     549                 track_list = &oldres->tracking;
> >     550         else {
> >     551                 track_list = &dlm->tracking_list;
> >     552                 if (list_empty(track_list)) {
> >     553                         dl = NULL;
> >     554                         spin_unlock(&dlm->track_lock);
> >     555                         goto bail;
> >     556                 }
> > 
> > Why not do the list_empty() check after the else statement.  In the
> > current code if "&oldres->tracking" is empty it will lead to a crash.
> > 
> lockres will be added into tracking_list during initialize.
> 

I am not sure we understand each other.

This function is iterating over the list.  Nothing should be modifying
the list until after we have released the spinlock.  Any modifications
is a race condition.  Which initialize function are you talking about?

In the currect code if oldres is non-NULL and list_empty(&oldres->tracking)
is true then it leads to a crash.  There is no reason that we cannot do:

	if (oldres)
		track_list = &oldres->tracking;
	else
		track_list = &dlm->tracking_list;

	if (list_empty(track_list)) {
		dl = NULL;
		spin_unlock(&dlm->track_lock);
		goto bail;
	}

This is cleaner and now we do not have know from outside context
whether the &oldres->tracking list can be empty.

> 
> >     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.

	if ((void *)res + 136 == (void *)dlm_ctxt + 88)

Use algebra to subtract 88 from both sides.

	if ((void *)res + 48 == (void *)dlm_ctxt)

So dlm points to somewhere in the middle of "res" struct.

>  
> >     561                         res = NULL;
> >     562                 else
> >     563                         dlm_lockres_get(res);
> >     564                 break;
> > 

regards,
dan carpenter


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

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

[Joseph Qi via Ocfs2-devel]

On 3/9/22 10:57 PM, Dan Carpenter via Ocfs2-devel wrote:
> On Wed, Mar 09, 2022 at 03:46:11PM +0800, Joseph Qi wrote:
>>
>>
>> On 3/7/22 10:51 PM, Dan Carpenter via Ocfs2-devel wrote:
>>> Hello OCFS Devs,
>>>
>>> There is a big push to re-write list_for_each_entry() so that you
>>> can't use the list iterator outside the loop.  I wrote a check for that
>>> but this code has me puzzled.
>>>
>>> The patch b0d4f817ba5d: "ocfs2/dlm: Fix race in adding/removing
>>> lockres' to/from the tracking list" from Dec 16, 2008, leads to the
>>> following Smatch static checker warning:
>>>
>>> 	fs/ocfs2/dlm/dlmdebug.c:573 lockres_seq_start()
>>> 	warn: iterator used outside loop: 'res'
>>>
>>> fs/ocfs2/dlm/dlmdebug.c
>>>     539 static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
>>>     540 {
>>>     541         struct debug_lockres *dl = m->private;
>>>     542         struct dlm_ctxt *dlm = dl->dl_ctxt;
>>>     543         struct dlm_lock_resource *oldres = dl->dl_res;
>>>     544         struct dlm_lock_resource *res = NULL;
>>>     545         struct list_head *track_list;
>>>     546 
>>>     547         spin_lock(&dlm->track_lock);
>>>     548         if (oldres)
>>>     549                 track_list = &oldres->tracking;
>>>     550         else {
>>>     551                 track_list = &dlm->tracking_list;
>>>     552                 if (list_empty(track_list)) {
>>>     553                         dl = NULL;
>>>     554                         spin_unlock(&dlm->track_lock);
>>>     555                         goto bail;
>>>     556                 }
>>>
>>> Why not do the list_empty() check after the else statement.  In the
>>> current code if "&oldres->tracking" is empty it will lead to a crash.
>>>
>> lockres will be added into tracking_list during initialize.
>>
> 
> I am not sure we understand each other.
> 
> This function is iterating over the list.  Nothing should be modifying
> the list until after we have released the spinlock.  Any modifications
> is a race condition.  Which initialize function are you talking about?
> 
dlm_init_lockres(), I mean if res is there, it seems won't be empty?
That's why we haven't encountered any real issue here, I guess.
But for code itself, I agree it makes a little puzzle.

> In the currect code if oldres is non-NULL and list_empty(&oldres->tracking)
> is true then it leads to a crash.  There is no reason that we cannot do:
> 
> 	if (oldres)
> 		track_list = &oldres->tracking;
> 	else
> 		track_list = &dlm->tracking_list;
> 
> 	if (list_empty(track_list)) {
> 		dl = NULL;
> 		spin_unlock(&dlm->track_lock);
> 		goto bail;
> 	}
> 
> This is cleaner and now we do not have know from outside context
> whether the &oldres->tracking list can be empty.
> 
>>
>>>     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?

Thanks,
Joseph 

> 	if ((void *)res + 136 == (void *)dlm_ctxt + 88)
> 
> Use algebra to subtract 88 from both sides.
> 
> 	if ((void *)res + 48 == (void *)dlm_ctxt)
> 
> So dlm points to somewhere in the middle of "res" struct.
> 
>>  
>>>     561                         res = NULL;
>>>     562                 else
>>>     563                         dlm_lockres_get(res);
>>>     564                 break;
>>>
> 
> regards,
> dan carpenter
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel

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

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

[Dan Carpenter via Ocfs2-devel]

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

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.

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

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

[Joseph Qi via Ocfs2-devel]

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.

> 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

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

[Joseph Qi via Ocfs2-devel]

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