From: Joseph Qi via Ocfs2-devel <ocfs2-devel@oss.oracle.com> To: Jakob Koschel <jakobkoschel@gmail.com> Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "Bos, H.J." <h.j.bos@vu.nl>, Brian Johannesmeyer <bjohannesmeyer@gmail.com>, Cristiano Giuffrida <c.giuffrida@vu.nl>, Geert Uytterhoeven <geert@linux-m68k.org>, Mike Rapoport <rppt@kernel.org>, Miguel Ojeda <ojeda@kernel.org>, Dan Carpenter <dan.carpenter@oracle.com>, Masahiro Yamada <masahiroy@kernel.org>, ocfs2-devel@oss.oracle.com Subject: Re: [Ocfs2-devel] [PATCH] ocfs2: fix check if list iterator did find an element Date: Tue, 22 Mar 2022 10:15:34 +0800 [thread overview] Message-ID: <5aefc78f-9a75-9b44-9471-87f42011b7c2@linux.alibaba.com> (raw) In-Reply-To: <A7AA04AA-6B4C-4211-99A6-0D3C04ED7B26@gmail.com> On 3/21/22 9:34 PM, Jakob Koschel wrote: > >> On 21. Mar 2022, at 02:50, Joseph Qi <joseph.qi@linux.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. > 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/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@gmail.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 _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel
WARNING: multiple messages have this Message-ID (diff)
From: Joseph Qi <joseph.qi@linux.alibaba.com> To: Jakob Koschel <jakobkoschel@gmail.com> 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> Subject: Re: [PATCH] ocfs2: fix check if list iterator did find an element Date: Tue, 22 Mar 2022 10:15:34 +0800 [thread overview] Message-ID: <5aefc78f-9a75-9b44-9471-87f42011b7c2@linux.alibaba.com> (raw) In-Reply-To: <A7AA04AA-6B4C-4211-99A6-0D3C04ED7B26@gmail.com> On 3/21/22 9:34 PM, Jakob Koschel wrote: > >> On 21. Mar 2022, at 02:50, Joseph Qi <joseph.qi@linux.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. > 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/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@gmail.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
next prev parent reply other threads:[~2022-03-22 2:15 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-19 20:31 [PATCH] ocfs2: fix check if list iterator did find an element Jakob Koschel 2022-03-21 1:50 ` Joseph Qi 2022-03-21 1:50 ` [Ocfs2-devel] " Joseph Qi via Ocfs2-devel 2022-03-21 13:34 ` Jakob Koschel 2022-03-21 13:54 ` [Ocfs2-devel] " Dan Carpenter via Ocfs2-devel 2022-03-21 13:54 ` Dan Carpenter 2022-03-21 16:00 ` David Laight 2022-03-21 16:00 ` [Ocfs2-devel] " David Laight via Ocfs2-devel 2022-03-21 16:22 ` Dan Carpenter 2022-03-21 16:22 ` [Ocfs2-devel] " Dan Carpenter via Ocfs2-devel 2022-03-22 2:15 ` Joseph Qi via Ocfs2-devel [this message] 2022-03-22 2:15 ` Joseph Qi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5aefc78f-9a75-9b44-9471-87f42011b7c2@linux.alibaba.com \ --to=ocfs2-devel@oss.oracle.com \ --cc=bjohannesmeyer@gmail.com \ --cc=c.giuffrida@vu.nl \ --cc=dan.carpenter@oracle.com \ --cc=geert@linux-m68k.org \ --cc=h.j.bos@vu.nl \ --cc=jakobkoschel@gmail.com \ --cc=joseph.qi@linux.alibaba.com \ --cc=linux-kernel@vger.kernel.org \ --cc=masahiroy@kernel.org \ --cc=ojeda@kernel.org \ --cc=rppt@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.