* [Ocfs2-devel] [bug report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list
@ 2022-03-07 14:51 Dan Carpenter via Ocfs2-devel
2022-03-09 7:46 ` Joseph Qi via Ocfs2-devel
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter via Ocfs2-devel @ 2022-03-07 14:51 UTC (permalink / raw)
To: ocfs2-devel; +Cc: Jakob Koschel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Ocfs2-devel] [bug report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list
2022-03-07 14:51 [Ocfs2-devel] [bug report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list Dan Carpenter via Ocfs2-devel
@ 2022-03-09 7:46 ` Joseph Qi via Ocfs2-devel
2022-03-09 14:57 ` Dan Carpenter via Ocfs2-devel
0 siblings, 1 reply; 7+ messages in thread
From: Joseph Qi via Ocfs2-devel @ 2022-03-09 7:46 UTC (permalink / raw)
To: Dan Carpenter, ocfs2-devel; +Cc: Jakob Koschel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Ocfs2-devel] [bug report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list
2022-03-09 7:46 ` Joseph Qi via Ocfs2-devel
@ 2022-03-09 14:57 ` Dan Carpenter via Ocfs2-devel
2022-03-10 3:13 ` Joseph Qi via Ocfs2-devel
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter via Ocfs2-devel @ 2022-03-09 14:57 UTC (permalink / raw)
To: Joseph Qi; +Cc: Jakob Koschel, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Ocfs2-devel] [bug report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list
2022-03-09 14:57 ` Dan Carpenter via Ocfs2-devel
@ 2022-03-10 3:13 ` Joseph Qi via Ocfs2-devel
2022-03-10 13:39 ` Dan Carpenter via Ocfs2-devel
0 siblings, 1 reply; 7+ messages in thread
From: Joseph Qi via Ocfs2-devel @ 2022-03-10 3:13 UTC (permalink / raw)
To: Dan Carpenter, Joseph Qi; +Cc: Jakob Koschel, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Ocfs2-devel] [bug report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list
2022-03-10 3:13 ` Joseph Qi via Ocfs2-devel
@ 2022-03-10 13:39 ` Dan Carpenter via Ocfs2-devel
2022-03-11 1:50 ` Joseph Qi via Ocfs2-devel
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter via Ocfs2-devel @ 2022-03-10 13:39 UTC (permalink / raw)
To: Joseph Qi; +Cc: Jakob Koschel, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Ocfs2-devel] [bug report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list
2022-03-10 13:39 ` Dan Carpenter via Ocfs2-devel
@ 2022-03-11 1:50 ` Joseph Qi via Ocfs2-devel
2022-03-11 3:08 ` Joseph Qi via Ocfs2-devel
0 siblings, 1 reply; 7+ messages in thread
From: Joseph Qi via Ocfs2-devel @ 2022-03-11 1:50 UTC (permalink / raw)
To: Dan Carpenter, Joseph Qi; +Cc: Jakob Koschel, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Ocfs2-devel] [bug report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list
2022-03-11 1:50 ` Joseph Qi via Ocfs2-devel
@ 2022-03-11 3:08 ` Joseph Qi via Ocfs2-devel
0 siblings, 0 replies; 7+ messages in thread
From: Joseph Qi via Ocfs2-devel @ 2022-03-11 3:08 UTC (permalink / raw)
To: Joseph Qi, Dan Carpenter; +Cc: Jakob Koschel, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-03-11 3:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-07 14:51 [Ocfs2-devel] [bug report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list Dan Carpenter via Ocfs2-devel
2022-03-09 7:46 ` Joseph Qi via Ocfs2-devel
2022-03-09 14:57 ` Dan Carpenter via Ocfs2-devel
2022-03-10 3:13 ` Joseph Qi via Ocfs2-devel
2022-03-10 13:39 ` Dan Carpenter via Ocfs2-devel
2022-03-11 1:50 ` Joseph Qi via Ocfs2-devel
2022-03-11 3:08 ` Joseph Qi via Ocfs2-devel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).