| 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
| ||
|
Message-ID: <aN5mwwFE2aEJwlT1@e129823.arm.com>
Date: Thu, 2 Oct 2025 12:49:23 +0100
From: Yeoreum Yun <yeoreum.yun@....com>
To: Byungchul Park <byungchul@...com>
Cc: akpm@...ux-foundation.org, david@...hat.com, ziy@...dia.com,
matthew.brost@...el.com, joshua.hahnjy@...il.com, rakie.kim@...com,
gourry@...rry.net, ying.huang@...ux.alibaba.com, apopple@...dia.com,
clameter@....com, kravetz@...ibm.com, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, max.byungchul.park@...il.com,
kernel_team@...ynix.com, harry.yoo@...cle.com,
gwan-gyeong.mun@...el.com, syzkaller@...glegroups.com,
ysk@...lloc.com
Subject: Re: [RFC] mm/migrate: make sure folio_unlock() before
folio_wait_writeback()
Sorry code was wrong.
> Hi Byoungchul,
>
> > DEPT(Dependency Tracker) reported a deadlock:
> >
> > ===================================================
> > DEPT: Circular dependency has been detected.
> > 6.15.11-00046-g2c223fa7bd9a-dirty #13 Not tainted
> > ---------------------------------------------------
> > summary
> > ---------------------------------------------------
> > *** DEADLOCK ***
> >
> > context A
> > [S] (unknown)(pg_locked_map:0)
> > [W] dept_page_wait_on_bit(pg_writeback_map:0)
> > [E] dept_page_clear_bit(pg_locked_map:0)
> >
> > context B
> > [S] (unknown)(pg_writeback_map:0)
> > [W] dept_page_wait_on_bit(pg_locked_map:0)
> > [E] dept_page_clear_bit(pg_writeback_map:0)
> >
> > [S]: start of the event context
> > [W]: the wait blocked
> > [E]: the event not reachable
> > ---------------------------------------------------
> > context A's detail
> > ---------------------------------------------------
> > context A
> > [S] (unknown)(pg_locked_map:0)
> > [W] dept_page_wait_on_bit(pg_writeback_map:0)
> > [E] dept_page_clear_bit(pg_locked_map:0)
> >
> > [S] (unknown)(pg_locked_map:0):
> > (N/A)
> >
> > [W] dept_page_wait_on_bit(pg_writeback_map:0):
> > [<ffff800080589c94>] folio_wait_bit+0x2c/0x38
> > stacktrace:
> > folio_wait_bit_common+0x824/0x8b8
> > folio_wait_bit+0x2c/0x38
> > folio_wait_writeback+0x5c/0xa4
> > migrate_pages_batch+0x5e4/0x1788
> > migrate_pages+0x15c4/0x1840
> > compact_zone+0x9c8/0x1d20
> > compact_node+0xd4/0x27c
> > sysctl_compaction_handler+0x104/0x194
> > proc_sys_call_handler+0x25c/0x3f8
> > proc_sys_write+0x20/0x2c
> > do_iter_readv_writev+0x350/0x448
> > vfs_writev+0x1ac/0x44c
> > do_pwritev+0x100/0x15c
> > __arm64_sys_pwritev2+0x6c/0xcc
> > invoke_syscall.constprop.0+0x64/0x18c
> > el0_svc_common.constprop.0+0x80/0x198
> >
> > [E] dept_page_clear_bit(pg_locked_map:0):
> > [<ffff800080700914>] migrate_folio_undo_src+0x1b4/0x200
> > stacktrace:
> > migrate_folio_undo_src+0x1b4/0x200
> > migrate_pages_batch+0x1578/0x1788
> > migrate_pages+0x15c4/0x1840
> > compact_zone+0x9c8/0x1d20
> > compact_node+0xd4/0x27c
> > sysctl_compaction_handler+0x104/0x194
> > proc_sys_call_handler+0x25c/0x3f8
> > proc_sys_write+0x20/0x2c
> > do_iter_readv_writev+0x350/0x448
> > vfs_writev+0x1ac/0x44c
> > do_pwritev+0x100/0x15c
> > __arm64_sys_pwritev2+0x6c/0xcc
> > invoke_syscall.constprop.0+0x64/0x18c
> > el0_svc_common.constprop.0+0x80/0x198
> > do_el0_svc+0x28/0x3c
> > el0_svc+0x50/0x220
> > ---------------------------------------------------
> > context B's detail
> > ---------------------------------------------------
> > context B
> > [S] (unknown)(pg_writeback_map:0)
> > [W] dept_page_wait_on_bit(pg_locked_map:0)
> > [E] dept_page_clear_bit(pg_writeback_map:0)
> >
> > [S] (unknown)(pg_writeback_map:0):
> > (N/A)
> >
> > [W] dept_page_wait_on_bit(pg_locked_map:0):
> > [<ffff80008081e478>] bdev_getblk+0x58/0x120
> > stacktrace:
> > find_get_block_common+0x224/0xbc4
> > bdev_getblk+0x58/0x120
> > __ext4_get_inode_loc+0x194/0x98c
> > ext4_get_inode_loc+0x4c/0xcc
> > ext4_reserve_inode_write+0x74/0x158
> > __ext4_mark_inode_dirty+0xd4/0x4e0
> > __ext4_ext_dirty+0x118/0x164
> > ext4_ext_map_blocks+0x1578/0x2ca8
> > ext4_map_blocks+0x2a4/0xa60
> > ext4_convert_unwritten_extents+0x1b0/0x3c0
> > ext4_convert_unwritten_io_end_vec+0x90/0x1a0
> > ext4_end_io_end+0x58/0x194
> > ext4_end_io_rsv_work+0xc4/0x150
> > process_one_work+0x3b4/0xac0
> > worker_thread+0x2b0/0x53c
> > kthread+0x1a0/0x33c
> >
> > [E] dept_page_clear_bit(pg_writeback_map:0):
> > [<ffff8000809dfc5c>] ext4_finish_bio+0x638/0x820
> > stacktrace:
> > folio_end_writeback+0x140/0x488
> > ext4_finish_bio+0x638/0x820
> > ext4_release_io_end+0x74/0x188
> > ext4_end_io_end+0xa0/0x194
> > ext4_end_io_rsv_work+0xc4/0x150
> > process_one_work+0x3b4/0xac0
> > worker_thread+0x2b0/0x53c
> > kthread+0x1a0/0x33c
> > ret_from_fork+0x10/0x20
> >
> > To simplify the scenario:
> >
> > context X (wq worker) context Y (process context)
> >
> > migrate_pages_batch()
> > ext4_end_io_end() ...
> > ... migrate_folio_unmap()
> > ext4_get_inode_loc() ...
> > ... folio_lock() // hold the folio lock
> > bdev_getblk() ...
> > ... folio_wait_writeback() // wait forever
> > __find_get_block_slow()
> > ... ...
> > folio_lock() // wait forever
> > folio_unlock() migrate_folio_undo_src()
> > ...
> > ... folio_unlock() // never reachable
> > ext4_finish_bio()
> > ...
> > folio_end_writeback() // never reachable
> >
> > context X is waiting for the folio lock to be released by context Y,
> > while context Y is waiting for the writeback to end in context X.
> > Ultimately, two contexts are waiting for the event that will never
> > happen, say, deadlock.
> >
> > *Only one* of the following two conditions should be allowed, or we
> > cannot avoid this kind of deadlock:
> >
> > 1. while holding a folio lock (and heading for folio_unlock()),
> > waiting for a writeback to end,
> > 2. while heading for the writeback end, waiting for the folio lock to
> > be released,
> >
> > Since allowing 2 and avoiding 1 sound more sensible than the other,
> > remove the first condition by making sure folio_unlock() before
> > folio_wait_writeback() in migrate_folio_unmap().
> >
> > Fixes: 49d2e9cc45443 ("[PATCH] Swap Migration V5: migrate_pages() function")
> > Reported-by: Yunseong Kim <ysk@...lloc.com>
> > Signed-off-by: Byungchul Park <byungchul@...com>
> > Tested-by: Yunseong Kim <ysk@...lloc.com>
> > ---
> >
> > Hi,
> >
> > Thanks to Yunseong for reporting the issue, testing, and confirming if
> > this patch can resolve the issue. We used the latest version of DEPT
> > to detect the issue:
> >
> > https://lore.kernel.org/all/20251002081247.51255-1-byungchul@sk.com/
> >
> > I mentioned in the commit message above like:
> >
> > *Only one* of the following two conditions should be allowed, or we
> > cannot avoid this kind of deadlock:
> >
> > 1. while holding a folio lock (and heading for folio_unlock()),
> > waiting for a writeback to end,
> > 2. while heading for the writeback end, waiting for the folio lock
> > to be released,
> >
> > Honestly, I'm not convinced which one we should choose between two, I
> > chose 'allowing 2 and avoiding 1' to resolve this issue though.
> >
> > However, please let me know if I was wrong and we should go for
> > 'allowing 1 and avoiding 2'. If so, I should try a different approach,
> > for example, to fix by preventing folio_lock() or using folio_try_lock()
> > while heading for writeback end in ext4_end_io_end() or something.
> >
> > To Yunseong,
> >
> > The link you shared for a system hang is:
> >
> > https://gist.github.com/kzall0c/a6091bb2fd536865ca9aabfd017a1fc5
> >
> > I think an important stacktrace for this issue, this is, waiting for
> > PG_writeback, was missed in the log.
> >
> > Byungchul
> >
> > ---
> > mm/migrate.c | 57 +++++++++++++++++++++++++++++++++++++---------------
> > 1 file changed, 41 insertions(+), 16 deletions(-)
> >
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index 9e5ef39ce73a..60b0b054f27a 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -1215,6 +1215,17 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
> >
> > dst->private = NULL;
> >
> > +retry_wait_writeback:
> > + /*
> > + * Only in the case of a full synchronous migration is it
> > + * necessary to wait for PageWriteback. In the async case, the
> > + * retry loop is too short and in the sync-light case, the
> > + * overhead of stalling is too much. Plus, do not write-back if
> > + * it's in the middle of direct compaction
> > + */
> > + if (folio_test_writeback(src) && mode == MIGRATE_SYNC)
> > + folio_wait_writeback(src);
> > +
> > if (!folio_trylock(src)) {
> > if (mode == MIGRATE_ASYNC)
> > goto out;
> > @@ -1245,27 +1256,41 @@ static int migrate_folio_unmap(new_folio_t get_new_folio,
> >
> > folio_lock(src);
> > }
> > - locked = true;
> > - if (folio_test_mlocked(src))
> > - old_page_state |= PAGE_WAS_MLOCKED;
> >
> > if (folio_test_writeback(src)) {
> > - /*
> > - * Only in the case of a full synchronous migration is it
> > - * necessary to wait for PageWriteback. In the async case,
> > - * the retry loop is too short and in the sync-light case,
> > - * the overhead of stalling is too much
> > - */
> > - switch (mode) {
> > - case MIGRATE_SYNC:
> > - break;
> > - default:
> > - rc = -EBUSY;
> > - goto out;
> > + if (mode == MIGRATE_SYNC) {
> > + /*
> > + * folio_unlock() is required before trying
> > + * folio_wait_writeback(). Or it leads a
> > + * deadlock like:
> > + *
> > + * context x context y
> > + * in XXX_io_end() in migrate_folio_unmap()
> > + *
> > + * ... ...
> > + * bdev_getblk(); folio_lock();
> > + *
> > + * // wait forever // wait forever
> > + * folio_lock(); folio_wait_writeback();
> > + *
> > + * ... ...
> > + * folio_unlock();
> > + * ... // never reachable
> > + * folio_unlock();
> > + * // never reachable
> > + * folio_end_writeback();
> > + */
> > + folio_unlock(src);
> > + goto retry_wait_writeback;
> > }
> > - folio_wait_writeback(src);
> > + rc = -EBUSY;
> > + goto out;
> > }
> >
> > + locked = true;
> > + if (folio_test_mlocked(src))
> > + old_page_state |= PAGE_WAS_MLOCKED;
> > +
> > /*
> > * By try_to_migrate(), src->mapcount goes down to 0 here. In this case,
> > * we cannot notice that anon_vma is freed while we migrate a page.
>
> Hmm, I still have concerns about this change.
> (1) seems to imply that the use of WB_SYNC_ALL by
> mpage_writebacks() is also incorrect. In addition,
> this change could introduce another theoretical livelock
> when the folio enters writeback frequently.
>
> AFAIK, while a folio is under writeback,
> its related buffers won’t be freed by migration, and
> since try_free_buffer() checks the writeback state first,
> taking folio_lock() shouldn’t be necessary while bdev_getblk().
>
> Therefore, it seems sufficient to check whether
> the folio is under writeback in __find_get_block_slow(), e.g.:
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 6a8752f7bbed..804d33df6b0f 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -194,6 +194,9 @@ __find_get_block_slow(struct block_device *bdev, sector_t block, bool atomic)
> if (IS_ERR(folio))
> goto out;
>
> + if (folio_test_writeback(folio))
> + return true;
> +
> /*
> * Folio lock protects the buffers. Callers that cannot block
> * will fallback to serializing vs try_to_free_buffers() via
>
> Am I missing something?
Sorry, the code was wrong. the suggestion is:
diff --git a/fs/buffer.c b/fs/buffer.c
index 6a8752f7bbed..804d33df6b0f 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -194,6 +194,9 @@ __find_get_block_slow(struct block_device *bdev, sector_t block, bool atomic)
if (IS_ERR(folio))
goto out;
+ if (folio_test_writeback(folio))
+ atomic = true;
+
/*
* Folio lock protects the buffers. Callers that cannot block
* will fallback to serializing vs try_to_free_buffers() via
--
Sincerely,
Yeoreum Yun
Powered by blists - more mailing lists