linux-kernel - Re: [PATCH] ocfs2: fix check if list iterator did find an element

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Mar 2022 14:34:34 +0100
From:   Jakob Koschel <jakobkoschel@...il.com>
To:     Joseph Qi <joseph.qi@...ux.alibaba.com>
Cc:     Mark Fasheh <mark@...heh.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ocfs2-devel@....oracle.com, Joel Becker <jlbec@...lplan.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Miguel Ojeda <ojeda@...nel.org>,
        Mike Rapoport <rppt@...nel.org>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH] ocfs2: fix check if list iterator did find an element


> On 21. Mar 2022, at 02:50, Joseph Qi <joseph.qi@...ux.alibaba.com> 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/bd0ec87e-b490-83dc-2363-5e5342c59fa4@linux.alibaba.com/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/20220308171818.384491-3-jakobkoschel@gmail.com/

> 
> 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/YhdfEIwI4EdtHdym@kroah.com/
>> Signed-off-by: Jakob Koschel <jakobkoschel@...il.com>
>> ---
>> 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