* [Ocfs2-devel] [PATCH 1/1] Ocfs2: Handle invalid lock_res in lockres_seq_start() for dlmdebug.c
@ 2010-09-09 9:16 Tristan Ye
2010-09-09 16:59 ` Sunil Mushran
0 siblings, 1 reply; 5+ messages in thread
From: Tristan Ye @ 2010-09-09 9:16 UTC (permalink / raw)
To: ocfs2-devel
In lockres_seq_start() of dlmdebug.c, when you looking at following
piece of codes:
list_for_each_entry(res, track_list, tracking) {
if (&res->tracking == &dlm->tracking_list)
res = NULL;
else
dlm_lockres_get(res);
break;
}
...
if (res) {
spin_lock(&res->spinlock);
dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
spin_unlock(&res->spinlock);
} else
dl = NULL;
One thought can come to you that, in the case of 'an-empty-list', cursor 'res'
here is not an INVALID pointer for real dlm_lock_resource object, it is nothing
than a fake address figured out by arbitary 'container_of()' way, the patch tries
to check track_list, and avoid accessing an invalid pointer if the list is empty,
it fixes following oops:
http://oss.oracle.com/bugzilla/show_bug.cgi?id=1287
Signed-off-by: Tristan Ye <tristan.ye@oracle.com>
---
fs/ocfs2/dlm/dlmdebug.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
index 5efdd37..06d668a 100644
--- a/fs/ocfs2/dlm/dlmdebug.c
+++ b/fs/ocfs2/dlm/dlmdebug.c
@@ -639,6 +639,12 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
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;
@@ -660,6 +666,7 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
} else
dl = NULL;
+bail:
/* passed to seq_show */
return dl;
}
--
1.5.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Ocfs2-devel] [PATCH 1/1] Ocfs2: Handle invalid lock_res in lockres_seq_start() for dlmdebug.c
2010-09-09 9:16 [Ocfs2-devel] [PATCH 1/1] Ocfs2: Handle invalid lock_res in lockres_seq_start() for dlmdebug.c Tristan Ye
@ 2010-09-09 16:59 ` Sunil Mushran
2010-09-10 1:20 ` tristan
0 siblings, 1 reply; 5+ messages in thread
From: Sunil Mushran @ 2010-09-09 16:59 UTC (permalink / raw)
To: ocfs2-devel
On 09/09/2010 02:16 AM, Tristan Ye wrote:
> In lockres_seq_start() of dlmdebug.c, when you looking at following
> piece of codes:
>
> list_for_each_entry(res, track_list, tracking) {
> if (&res->tracking ==&dlm->tracking_list)
> res = NULL;
> else
> dlm_lockres_get(res);
> break;
> }
>
> ...
>
> if (res) {
> spin_lock(&res->spinlock);
> dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
> spin_unlock(&res->spinlock);
> } else
> dl = NULL;
>
> One thought can come to you that, in the case of 'an-empty-list', cursor 'res'
> here is not an INVALID pointer for real dlm_lock_resource object, it is nothing
> than a fake address figured out by arbitary 'container_of()' way, the patch tries
> to check track_list, and avoid accessing an invalid pointer if the list is empty,
> it fixes following oops:
>
> http://oss.oracle.com/bugzilla/show_bug.cgi?id=1287
>
> Signed-off-by: Tristan Ye<tristan.ye@oracle.com>
> ---
>
Such a detailed description is only required if the bug fix
is complicated. This is a simple fix. Just say that this handles
the case in which the dlm->tracking_list is empty and mention
the bz.
> fs/ocfs2/dlm/dlmdebug.c | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
> index 5efdd37..06d668a 100644
> --- a/fs/ocfs2/dlm/dlmdebug.c
> +++ b/fs/ocfs2/dlm/dlmdebug.c
> @@ -639,6 +639,12 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
> else
> track_list =&dlm->tracking_list;
>
> + if (list_empty(track_list)) {
> + dl = NULL;
> + spin_unlock(&dlm->track_lock);
> + goto bail;
> + }
> +
>
You should add this check as part of the else block above so
that we check for empty list only at the start.
> list_for_each_entry(res, track_list, tracking) {
> if (&res->tracking ==&dlm->tracking_list)
> res = NULL;
> @@ -660,6 +666,7 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
> } else
> dl = NULL;
>
> +bail:
> /* passed to seq_show */
> return dl;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Ocfs2-devel] [PATCH 1/1] Ocfs2: Handle invalid lock_res in lockres_seq_start() for dlmdebug.c
2010-09-09 16:59 ` Sunil Mushran
@ 2010-09-10 1:20 ` tristan
2010-09-10 1:26 ` Sunil Mushran
0 siblings, 1 reply; 5+ messages in thread
From: tristan @ 2010-09-10 1:20 UTC (permalink / raw)
To: ocfs2-devel
Sunil Mushran wrote:
> On 09/09/2010 02:16 AM, Tristan Ye wrote:
>> In lockres_seq_start() of dlmdebug.c, when you looking at following
>> piece of codes:
>>
>> list_for_each_entry(res, track_list, tracking) {
>> if (&res->tracking ==&dlm->tracking_list)
>> res = NULL;
>> else
>> dlm_lockres_get(res);
>> break;
>> }
>>
>> ...
>>
>> if (res) {
>> spin_lock(&res->spinlock);
>> dump_lockres(res, dl->dl_buf, dl->dl_len - 1);
>> spin_unlock(&res->spinlock);
>> } else
>> dl = NULL;
>>
>> One thought can come to you that, in the case of 'an-empty-list',
>> cursor 'res'
>> here is not an INVALID pointer for real dlm_lock_resource object, it
>> is nothing
>> than a fake address figured out by arbitary 'container_of()' way, the
>> patch tries
>> to check track_list, and avoid accessing an invalid pointer if the
>> list is empty,
>> it fixes following oops:
>>
>> http://oss.oracle.com/bugzilla/show_bug.cgi?id=1287
>>
>> Signed-off-by: Tristan Ye<tristan.ye@oracle.com>
>> ---
>>
>
> Such a detailed description is only required if the bug fix
> is complicated. This is a simple fix. Just say that this handles
> the case in which the dlm->tracking_list is empty and mention
> the bz.
>
Yeah, kind of...
>> fs/ocfs2/dlm/dlmdebug.c | 7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
>> index 5efdd37..06d668a 100644
>> --- a/fs/ocfs2/dlm/dlmdebug.c
>> +++ b/fs/ocfs2/dlm/dlmdebug.c
>> @@ -639,6 +639,12 @@ static void *lockres_seq_start(struct seq_file
>> *m, loff_t *pos)
>> else
>> track_list =&dlm->tracking_list;
>>
>> + if (list_empty(track_list)) {
>> + dl = NULL;
>> + spin_unlock(&dlm->track_lock);
>> + goto bail;
>> + }
>> +
>>
>
> You should add this check as part of the else block above so
> that we check for empty list only at the start.
So we just don't need to check list 'oldres->tracking'?
>
>> list_for_each_entry(res, track_list, tracking) {
>> if (&res->tracking ==&dlm->tracking_list)
>> res = NULL;
>> @@ -660,6 +666,7 @@ static void *lockres_seq_start(struct seq_file
>> *m, loff_t *pos)
>> } else
>> dl = NULL;
>>
>> +bail:
>> /* passed to seq_show */
>> return dl;
>> }
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Ocfs2-devel] [PATCH 1/1] Ocfs2: Handle invalid lock_res in lockres_seq_start() for dlmdebug.c
2010-09-10 1:20 ` tristan
@ 2010-09-10 1:26 ` Sunil Mushran
2010-09-10 1:42 ` tristan
0 siblings, 1 reply; 5+ messages in thread
From: Sunil Mushran @ 2010-09-10 1:26 UTC (permalink / raw)
To: ocfs2-devel
On 09/09/2010 06:20 PM, tristan wrote:
>>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
>>> index 5efdd37..06d668a 100644
>>> --- a/fs/ocfs2/dlm/dlmdebug.c
>>> +++ b/fs/ocfs2/dlm/dlmdebug.c
>>> @@ -639,6 +639,12 @@ static void *lockres_seq_start(struct seq_file
>>> *m, loff_t *pos)
>>> else
>>> track_list =&dlm->tracking_list;
>>>
>>> + if (list_empty(track_list)) {
>>> + dl = NULL;
>>> + spin_unlock(&dlm->track_lock);
>>> + goto bail;
>>> + }
>>> +
>>
>> You should add this check as part of the else block above so
>> that we check for empty list only at the start.
> fs/ocfs2/dlm/dlmdebug.c | 7 +++++++
>
> So we just don't need to check list 'oldres->tracking'?
If you see how it works, it prints one lock resource at a time. As in,
each time _start is called, it fetches the next lock resource in the
tracking
list. So the empty list should be checked only at the very beginning.
To be fair, your code should also work. If the list has even one lockres,
it cannot, by definition, be empty. But adding it in the if block makes the
check explicit and clean. IMHO.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Ocfs2-devel] [PATCH 1/1] Ocfs2: Handle invalid lock_res in lockres_seq_start() for dlmdebug.c
2010-09-10 1:26 ` Sunil Mushran
@ 2010-09-10 1:42 ` tristan
0 siblings, 0 replies; 5+ messages in thread
From: tristan @ 2010-09-10 1:42 UTC (permalink / raw)
To: ocfs2-devel
Sunil Mushran wrote:
> On 09/09/2010 06:20 PM, tristan wrote:
>>>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
>>>> index 5efdd37..06d668a 100644
>>>> --- a/fs/ocfs2/dlm/dlmdebug.c
>>>> +++ b/fs/ocfs2/dlm/dlmdebug.c
>>>> @@ -639,6 +639,12 @@ static void *lockres_seq_start(struct seq_file
>>>> *m, loff_t *pos)
>>>> else
>>>> track_list =&dlm->tracking_list;
>>>>
>>>> + if (list_empty(track_list)) {
>>>> + dl = NULL;
>>>> + spin_unlock(&dlm->track_lock);
>>>> + goto bail;
>>>> + }
>>>> +
>>>
>>> You should add this check as part of the else block above so
>>> that we check for empty list only at the start.
>> fs/ocfs2/dlm/dlmdebug.c | 7 +++++++
>>
>> So we just don't need to check list 'oldres->tracking'?
>
> If you see how it works, it prints one lock resource at a time. As in,
> each time _start is called, it fetches the next lock resource in the
> tracking
> list. So the empty list should be checked only at the very beginning.
>
> To be fair, your code should also work. If the list has even one lockres,
> it cannot, by definition, be empty. But adding it in the if block
> makes the
> check explicit and clean. IMHO.
That makes pretty sense, we just need to check the list at the time of
getting its head:)
My check was making the logic clumsy, thanks for correction.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-09-10 1:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-09 9:16 [Ocfs2-devel] [PATCH 1/1] Ocfs2: Handle invalid lock_res in lockres_seq_start() for dlmdebug.c Tristan Ye
2010-09-09 16:59 ` Sunil Mushran
2010-09-10 1:20 ` tristan
2010-09-10 1:26 ` Sunil Mushran
2010-09-10 1:42 ` tristan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.