| Message ID | 20220307145138.GA22641@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [bug,report] ocfs2/dlm: Fix race in adding/removing lockres' to/from the tracking list | expand |
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; > }
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
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
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
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
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
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; }
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(-)