Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.80.82.1.1\))
Subject: Re: [PATCH] ocfs2: fix check if list iterator did find an element
From:   Jakob Koschel <jakobkoschel@gmail.com>
In-Reply-To: <abcbf5d2-c348-7dc0-cfd3-9bcc6431bc64@linux.alibaba.com>
Date:   Mon, 21 Mar 2022 14:34:34 +0100
Cc:     Mark Fasheh <mark@fasheh.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        ocfs2-devel@oss.oracle.com, Joel Becker <jlbec@evilplan.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Masahiro Yamada <masahiroy@kernel.org>,
        Miguel Ojeda <ojeda@kernel.org>,
        Mike Rapoport <rppt@kernel.org>,
        Brian Johannesmeyer <bjohannesmeyer@gmail.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>,
        Dan Carpenter <dan.carpenter@oracle.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <A7AA04AA-6B4C-4211-99A6-0D3C04ED7B26@gmail.com>
References: <20220319203106.2541700-1-jakobkoschel@gmail.com>
 <abcbf5d2-c348-7dc0-cfd3-9bcc6431bc64@linux.alibaba.com>
To:     Joseph Qi <joseph.qi@linux.alibaba.com>
Precedence: bulk


> On 21. Mar 2022, at 02:50, Joseph Qi <joseph.qi@linux.alibaba.com> =
wrote:
>=20
>=20
>=20
> 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.
>>=20
>> In the original code 'res' would have been set to an incorrect =
pointer
>> if the list is empty.
>>=20
> 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@l=
inux.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/

>=20
> Thanks,
> Joseph
>=20
>> 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].
>>=20
>> Link: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/
>> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
>> ---
>> fs/ocfs2/dlm/dlmdebug.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>=20
>> 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 =3D m->private;
>> 	struct dlm_ctxt *dlm =3D dl->dl_ctxt;
>> 	struct dlm_lock_resource *oldres =3D dl->dl_res;
>> -	struct dlm_lock_resource *res =3D NULL;
>> +	struct dlm_lock_resource *res =3D NULL, *iter;
>> 	struct list_head *track_list;
>>=20
>> 	spin_lock(&dlm->track_lock);
>> @@ -556,11 +556,11 @@ static void *lockres_seq_start(struct seq_file =
*m, loff_t *pos)
>> 		}
>> 	}
>>=20
>> -	list_for_each_entry(res, track_list, tracking) {
>> -		if (&res->tracking =3D=3D &dlm->tracking_list)
>> -			res =3D NULL;
>> -		else
>> -			dlm_lockres_get(res);
>> +	list_for_each_entry(iter, track_list, tracking) {
>> +		if (&iter->tracking !=3D &dlm->tracking_list) {
>> +			dlm_lockres_get(iter);
>> +			res =3D iter;
>> +		}
>> 		break;
>> 	}
>> 	spin_unlock(&dlm->track_lock);
>>=20
>> base-commit: 34e047aa16c0123bbae8e2f6df33e5ecc1f56601
>> --
>> 2.25.1

	Jakob=