Messages in this thread Patch in this message | | | From | Jakob Koschel <> | Date | Thu, 02 Mar 2023 00:02:46 +0100 | Subject | [PATCH] fs/jfs: avoid usage of list iterator after loop in lmPostGC() |
| |
If the list_for_each_entry_safe() iteration never breaks, 'tblk' would contain an invalid pointer past the iterator loop. To ensure 'tblk' is always valid, we only set it if the correct element was found. That allows adding a BUG_ON in case the code works incorrectly, exposing currently undetectable potential bugs.
Additionally, Linus proposed to avoid any use of the list iterator variable after the loop, in the attempt to move the list iterator variable declaration into the marcro to avoid any potential misuse after the loop [1].
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1] Signed-off-by: Jakob Koschel <jkl820.git@gmail.com> --- I've found this code path looking at the output of the use_after_iter.cocci script. From an outsider perspective it's difficult for me to judge if the condition '!(tblk->flag & tblkGC_COMMIT)' is guaranteed to be true for at least one entry in the list.
If not then the access 'tblk->bp->l_wqnext' in line 887 will not work with a valid 'tblk' entry.
For now I've replaced the iterator variable with 'iter' and only set the 'tblk' (used after the iteration), in the break case where it is guaranteed to be correct.
I'm happy to get some input on this and open for any suggestion. --- fs/jfs/jfs_logmgr.c | 37 ++++++++++++++++++++----------------- 1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/fs/jfs/jfs_logmgr.c b/fs/jfs/jfs_logmgr.c index 695415cbfe98..e3ca0bb6dbd5 100644 --- a/fs/jfs/jfs_logmgr.c +++ b/fs/jfs/jfs_logmgr.c @@ -804,7 +804,7 @@ static void lmPostGC(struct lbuf * bp) unsigned long flags; struct jfs_log *log = bp->l_log; struct logpage *lp; - struct tblock *tblk, *temp; + struct tblock *tblk = NULL, *iter, *temp; //LOGGC_LOCK(log); spin_lock_irqsave(&log->gclock, flags); @@ -814,54 +814,56 @@ static void lmPostGC(struct lbuf * bp) * remove/wakeup transactions from commit queue who were * group committed with the current log page */ - list_for_each_entry_safe(tblk, temp, &log->cqueue, cqueue) { - if (!(tblk->flag & tblkGC_COMMIT)) + list_for_each_entry_safe(iter, temp, &log->cqueue, cqueue) { + if (!(iter->flag & tblkGC_COMMIT)) { + tblk = iter; break; + } /* if transaction was marked GC_COMMIT then * it has been shipped in the current pageout * and made it to disk - it is committed. */ if (bp->l_flag & lbmERROR) - tblk->flag |= tblkGC_ERROR; + iter->flag |= tblkGC_ERROR; /* remove it from the commit queue */ - list_del(&tblk->cqueue); - tblk->flag &= ~tblkGC_QUEUE; + list_del(&iter->cqueue); + iter->flag &= ~tblkGC_QUEUE; - if (tblk == log->flush_tblk) { + if (iter == log->flush_tblk) { /* we can stop flushing the log now */ clear_bit(log_FLUSH, &log->flag); log->flush_tblk = NULL; } - jfs_info("lmPostGC: tblk = 0x%p, flag = 0x%x", tblk, - tblk->flag); + jfs_info("lmPostGC: tblk = 0x%p, flag = 0x%x", iter, + iter->flag); - if (!(tblk->xflag & COMMIT_FORCE)) + if (!(iter->xflag & COMMIT_FORCE)) /* - * Hand tblk over to lazy commit thread + * Hand iter over to lazy commit thread */ - txLazyUnlock(tblk); + txLazyUnlock(iter); else { /* state transition: COMMIT -> COMMITTED */ - tblk->flag |= tblkGC_COMMITTED; + iter->flag |= tblkGC_COMMITTED; - if (tblk->flag & tblkGC_READY) + if (iter->flag & tblkGC_READY) log->gcrtc--; - LOGGC_WAKEUP(tblk); + LOGGC_WAKEUP(iter); } /* was page full before pageout ? * (and this is the last tblk bound with the page) */ - if (tblk->flag & tblkGC_FREE) + if (iter->flag & tblkGC_FREE) lbmFree(bp); /* did page become full after pageout ? * (and this is the last tblk bound with the page) */ - else if (tblk->flag & tblkGC_EOP) { + else if (iter->flag & tblkGC_EOP) { /* finalize the page */ lp = (struct logpage *) bp->l_ldata; bp->l_ceor = bp->l_eor; @@ -880,6 +882,7 @@ static void lmPostGC(struct lbuf * bp) * select the latest ready transaction as new group leader and * wake her up to lead her group. */ + BUG_ON(!tblk); if ((!list_empty(&log->cqueue)) && ((log->gcrtc > 0) || (tblk->bp->l_wqnext != NULL) || test_bit(log_FLUSH, &log->flag) || jfs_tlocks_low)) --- base-commit: c0927a7a5391f7d8e593e5e50ead7505a23cadf9 change-id: 20230301-fs-jfs-avoid-iter-after-loop-82a17102e548 Best regards, -- Jakob Koschel <jkl820.git@gmail.com>
| |