On 3/20/22 4:31 AM, Jakob Koschel wrote:
> Instead of setting 'res' to NULL, it should only be set if
> the suitable element was found.
>
> In the original code 'res' would have been set to an incorrect pointer
> if the list is empty.
>
The logic before iteration can make sure track_list won't be empty.
Please refer the discussion via:
https://lore.kernel.org/ocfs2-devel/[email protected]/T/#m96d4397930201d83d68677c33a9721ae8dbd8f15
Thanks,
Joseph
> In preparation to limit the scope of the list iterator to the list
> traversal loop, use a dedicated pointer pointing to the found element [1].
>
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Jakob Koschel <[email protected]>
> ---
> fs/ocfs2/dlm/dlmdebug.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
> index d442cf5dda8a..be5e9ed7da8d 100644
> --- a/fs/ocfs2/dlm/dlmdebug.c
> +++ b/fs/ocfs2/dlm/dlmdebug.c
> @@ -541,7 +541,7 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
> struct debug_lockres *dl = m->private;
> struct dlm_ctxt *dlm = dl->dl_ctxt;
> struct dlm_lock_resource *oldres = dl->dl_res;
> - struct dlm_lock_resource *res = NULL;
> + struct dlm_lock_resource *res = NULL, *iter;
> struct list_head *track_list;
>
> spin_lock(&dlm->track_lock);
> @@ -556,11 +556,11 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
> }
> }
>
> - list_for_each_entry(res, track_list, tracking) {
> - if (&res->tracking == &dlm->tracking_list)
> - res = NULL;
> - else
> - dlm_lockres_get(res);
> + list_for_each_entry(iter, track_list, tracking) {
> + if (&iter->tracking != &dlm->tracking_list) {
> + dlm_lockres_get(iter);
> + res = iter;
> + }
> break;
> }
> spin_unlock(&dlm->track_lock);
>
> base-commit: 34e047aa16c0123bbae8e2f6df33e5ecc1f56601
> --
> 2.25.1
> On 21. Mar 2022, at 02:50, Joseph Qi <[email protected]> wrote:
>
>
>
> On 3/20/22 4:31 AM, Jakob Koschel wrote:
>> Instead of setting 'res' to NULL, it should only be set if
>> the suitable element was found.
>>
>> In the original code 'res' would have been set to an incorrect pointer
>> if the list is empty.
>>
> The logic before iteration can make sure track_list won't be empty.
> Please refer the discussion via:
> https://lore.kernel.org/ocfs2-devel/[email protected]/T/#m96d4397930201d83d68677c33a9721ae8dbd8f15
ah yes, I just read up on the discussion there, sorry for having duplicated it
here.
Was any conclusion reached there which fixes can/should be merged?
This code obviously can always be safe if the list cannot be empty.
That's also not necessarily the reason I'm fixing this. The reason is that
we want to get rid of any use of the list iterator variable after the loop
('res' in this case). This will allow moving the list iterator variable
into the scope of the list iterator macro to forbid any invalid use of it
at compile time. Like this you don't have to rely on assumptions that are
hard to validate (e.g. that a certain list is never empty).
The patch here is the minimal change to simply do that but looking at
Dan Carpenter patch there might be more things in this code that can
be simplified.
[CC'd Dan Carpenter]
See [1] for changes that have already been merged:
[1] https://lore.kernel.org/linux-kernel/[email protected]/
>
> Thanks,
> Joseph
>
>> In preparation to limit the scope of the list iterator to the list
>> traversal loop, use a dedicated pointer pointing to the found element [1].
>>
>> Link: https://lore.kernel.org/all/[email protected]/
>> Signed-off-by: Jakob Koschel <[email protected]>
>> ---
>> fs/ocfs2/dlm/dlmdebug.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
>> index d442cf5dda8a..be5e9ed7da8d 100644
>> --- a/fs/ocfs2/dlm/dlmdebug.c
>> +++ b/fs/ocfs2/dlm/dlmdebug.c
>> @@ -541,7 +541,7 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
>> struct debug_lockres *dl = m->private;
>> struct dlm_ctxt *dlm = dl->dl_ctxt;
>> struct dlm_lock_resource *oldres = dl->dl_res;
>> - struct dlm_lock_resource *res = NULL;
>> + struct dlm_lock_resource *res = NULL, *iter;
>> struct list_head *track_list;
>>
>> spin_lock(&dlm->track_lock);
>> @@ -556,11 +556,11 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
>> }
>> }
>>
>> - list_for_each_entry(res, track_list, tracking) {
>> - if (&res->tracking == &dlm->tracking_list)
>> - res = NULL;
>> - else
>> - dlm_lockres_get(res);
>> + list_for_each_entry(iter, track_list, tracking) {
>> + if (&iter->tracking != &dlm->tracking_list) {
>> + dlm_lockres_get(iter);
>> + res = iter;
>> + }
>> break;
>> }
>> spin_unlock(&dlm->track_lock);
>>
>> base-commit: 34e047aa16c0123bbae8e2f6df33e5ecc1f56601
>> --
>> 2.25.1
Jakob
On 3/21/22 9:34 PM, Jakob Koschel wrote:
>
>> On 21. Mar 2022, at 02:50, Joseph Qi <[email protected]> wrote:
>>
>>
>>
>> On 3/20/22 4:31 AM, Jakob Koschel wrote:
>>> Instead of setting 'res' to NULL, it should only be set if
>>> the suitable element was found.
>>>
>>> In the original code 'res' would have been set to an incorrect pointer
>>> if the list is empty.
>>>
>> The logic before iteration can make sure track_list won't be empty.
>> Please refer the discussion via:
>> https://lore.kernel.org/ocfs2-devel/[email protected]/T/#m96d4397930201d83d68677c33a9721ae8dbd8f15
>
> ah yes, I just read up on the discussion there, sorry for having duplicated it
> here.
>
> Was any conclusion reached there which fixes can/should be merged?
>
> This code obviously can always be safe if the list cannot be empty.
> That's also not necessarily the reason I'm fixing this. The reason is that
> we want to get rid of any use of the list iterator variable after the loop
> ('res' in this case). This will allow moving the list iterator variable
> into the scope of the list iterator macro to forbid any invalid use of it
> at compile time. Like this you don't have to rely on assumptions that are
> hard to validate (e.g. that a certain list is never empty).
>
> The patch here is the minimal change to simply do that but looking at
> Dan Carpenter patch there might be more things in this code that can
> be simplified.
>
Agree, so I'm fine with this change.
So could you please update the description and send v2?
Thanks,
Joseph
> [CC'd Dan Carpenter]
>
> See [1] for changes that have already been merged:
>
> [1] https://lore.kernel.org/linux-kernel/[email protected]/
>
>>
>> Thanks,
>> Joseph
>>
>>> In preparation to limit the scope of the list iterator to the list
>>> traversal loop, use a dedicated pointer pointing to the found element [1].
>>>
>>> Link: https://lore.kernel.org/all/[email protected]/
>>> Signed-off-by: Jakob Koschel <[email protected]>
>>> ---
>>> fs/ocfs2/dlm/dlmdebug.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/dlm/dlmdebug.c b/fs/ocfs2/dlm/dlmdebug.c
>>> index d442cf5dda8a..be5e9ed7da8d 100644
>>> --- a/fs/ocfs2/dlm/dlmdebug.c
>>> +++ b/fs/ocfs2/dlm/dlmdebug.c
>>> @@ -541,7 +541,7 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
>>> struct debug_lockres *dl = m->private;
>>> struct dlm_ctxt *dlm = dl->dl_ctxt;
>>> struct dlm_lock_resource *oldres = dl->dl_res;
>>> - struct dlm_lock_resource *res = NULL;
>>> + struct dlm_lock_resource *res = NULL, *iter;
>>> struct list_head *track_list;
>>>
>>> spin_lock(&dlm->track_lock);
>>> @@ -556,11 +556,11 @@ static void *lockres_seq_start(struct seq_file *m, loff_t *pos)
>>> }
>>> }
>>>
>>> - list_for_each_entry(res, track_list, tracking) {
>>> - if (&res->tracking == &dlm->tracking_list)
>>> - res = NULL;
>>> - else
>>> - dlm_lockres_get(res);
>>> + list_for_each_entry(iter, track_list, tracking) {
>>> + if (&iter->tracking != &dlm->tracking_list) {
>>> + dlm_lockres_get(iter);
>>> + res = iter;
>>> + }
>>> break;
>>> }
>>> spin_unlock(&dlm->track_lock);
>>>
>>> base-commit: 34e047aa16c0123bbae8e2f6df33e5ecc1f56601
>>> --
>>> 2.25.1
>
> Jakob