* Re: [PATCH 1/3] ext4: split __ext4_add_entry() out of ext4_add_entry()
From: Jan Kara @ 2026-03-18 17:56 UTC (permalink / raw)
To: NeilBrown
Cc: Theodore Ts'o, Andreas Dilger, Jan Kara, linux-ext4,
linux-fsdevel
In-Reply-To: <20260317224638.3809014-2-neilb@ownmail.net>
On Wed 18-03-26 09:39:49, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
>
> __ext4_add_entry() is not given a dentry - just inodes and name.
> This will help the next patch which simplifies __ex4_link().
>
> Signed-off-by: NeilBrown <neil@brown.name>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/ext4/namei.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index c4b5e252af0e..768036a109d7 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2353,10 +2353,10 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
> * may not sleep between calling this and putting something into
> * the entry, as someone else might have used it while you slept.
> */
> -static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> +static int __ext4_add_entry(handle_t *handle, struct inode *dir,
> + const struct qstr *d_name,
> struct inode *inode)
> {
> - struct inode *dir = d_inode(dentry->d_parent);
> struct buffer_head *bh = NULL;
> struct ext4_dir_entry_2 *de;
> struct super_block *sb;
> @@ -2373,13 +2373,10 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> sb = dir->i_sb;
> blocksize = sb->s_blocksize;
>
> - if (fscrypt_is_nokey_name(dentry))
> - return -ENOKEY;
> -
> - if (!generic_ci_validate_strict_name(dir, &dentry->d_name))
> + if (!generic_ci_validate_strict_name(dir, d_name))
> return -EINVAL;
>
> - retval = ext4_fname_setup_filename(dir, &dentry->d_name, 0, &fname);
> + retval = ext4_fname_setup_filename(dir, d_name, 0, &fname);
> if (retval)
> return retval;
>
> @@ -2460,6 +2457,16 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> return retval;
> }
>
> +static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> + struct inode *inode)
> +{
> + struct inode *dir = d_inode(dentry->d_parent);
> +
> + if (fscrypt_is_nokey_name(dentry))
> + return -ENOKEY;
> + return __ext4_add_entry(handle, dir, &dentry->d_name, inode);
> +}
> +
> /*
> * Returns 0 for success, or a negative error value
> */
> --
> 2.50.0.107.gf914562f5916.dirty
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH 32/53f] ext4: move dcache modifying code out of __ext4_link()
From: Jan Kara @ 2026-03-18 17:47 UTC (permalink / raw)
To: NeilBrown
Cc: Jan Kara, Alexander Viro, Christian Brauner, Jeff Layton,
Theodore Ts'o, Andreas Dilger, linux-fsdevel, linux-ext4,
linux-kernel
In-Reply-To: <177377925781.5556.10082450121311369542@noble.neil.brown.name>
On Wed 18-03-26 07:27:37, NeilBrown wrote:
> > > @@ -3460,8 +3460,6 @@ int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
> > > ext4_handle_sync(handle);
> > >
> > > inode_set_ctime_current(inode);
> > > - ext4_inc_count(inode);
> > > - ihold(inode);
> > >
> > > err = ext4_add_entry(handle, dentry, inode);
> > > if (!err) {
> > > @@ -3471,11 +3469,7 @@ int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
> > > */
> > > if (inode->i_nlink == 1)
> > > ext4_orphan_del(handle, inode);
> > > - d_instantiate(dentry, inode);
> > > - ext4_fc_track_link(handle, dentry);
> > > - } else {
> > > - drop_nlink(inode);
> > > - iput(inode);
> > > + __ext4_fc_track_link(handle, inode, dentry);
> >
> > This looks wrong. If fastcommit replay is running, we must skip calling
> > __ext4_fc_track_link(). Similarly if the filesystem is currently
> > inelligible for fastcommit (due to some complex unsupported operations
> > running in parallel). Why did you change ext4_fc_track_link() to
> > __ext4_fc_track_link()?
>
> I changed to __ext4_fc_track_link() because I needed something that
> accepted the inode separately from the dentry. As you point out, that
> means we lose some important code which makes the decision misguided.
>
> I'm wondering about taking a different approach - not using a dentry at
> all and not constifying anything.
> I could split __ext4_add_entry() out of ext4_add_entry() and instead of
> passing the dentry-to-add I could pass the dir inode qstr name, and
> d_flags.
>
> Then ext4_link could be passed the same plus a "do fast commit" flag.
>
> The result would be more verbose, but also hopefully more clear.
Yeah, I was considering that option as well when looking at this. Let's see
how the code will look like.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v2 0/3] decoupling the ext4 test module
From: Jan Kara @ 2026-03-18 16:59 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack
In-Reply-To: <20260314075258.1317579-1-yebin@huaweicloud.com>
On Sat 14-03-26 15:52:55, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> This patchset is split out from the "Fix some issues about ext4-test"
> patchset. It decouples mballoc-test.c and extents-test.c from the
> ext4 module, resolving the issue where these two tests could not be run
> when EXT4_KUNIT_TESTS is set to M.
>
> Ye Bin (3):
> ext4: introduce EXPORT_SYMBOL_FOR_EXT4_TEST() helper
> ext4: fix mballoc-test.c is not compiled when EXT4_KUNIT_TESTS=M
> ext4: fix extents-test.c is not compiled when EXT4_KUNIT_TESTS=M
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
>
> fs/ext4/Makefile | 5 +-
> fs/ext4/ext4.h | 5 ++
> fs/ext4/ext4_extents.h | 12 +++++
> fs/ext4/extents-test.c | 8 ++--
> fs/ext4/extents.c | 39 +++++++++++++---
> fs/ext4/mballoc-test.c | 81 ++++++++++++++++----------------
> fs/ext4/mballoc.c | 102 +++++++++++++++++++++++++++++++++++++++--
> fs/ext4/mballoc.h | 30 ++++++++++++
> 8 files changed, 227 insertions(+), 55 deletions(-)
>
> --
> 2.34.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v1] ext4: fix use-after-free in update_super_work when racing with umount
From: Jan Kara @ 2026-03-18 16:56 UTC (permalink / raw)
To: Jiayuan Chen
Cc: Jan Kara, linux-ext4, Theodore Ts'o, Andreas Dilger,
Ritesh Harjani, Ye Bin, linux-kernel
In-Reply-To: <4a102211-2039-46ac-bc54-81f1c5eda42a@linux.dev>
On Wed 18-03-26 13:04:03, Jiayuan Chen wrote:
>
> On 3/17/26 7:38 PM, Jan Kara wrote:
> > On Fri 13-03-26 14:52:04, Jiayuan Chen wrote:
> > > Commit b98535d09179 ("ext4: fix bug_on in start_this_handle during umount filesystem")
> > > moved ext4_unregister_sysfs() before flushing s_sb_upd_work to prevent new
> > > error work from being queued via /proc/fs/ext4/xx/mb_groups reads during
> > > unmount. However, this introduced a use-after-free because
> > > update_super_work calls ext4_notify_error_sysfs() -> sysfs_notify() which
> > > accesses the kobject's kernfs_node after it has been freed:
> > >
> > > update_super_work ext4_put_super
> > > ----------------- --------------
> > > ext4_unregister_sysfs(sb)
> > > kobject_del(&sbi->s_kobj)
> > > __kobject_del()
> > > sysfs_remove_dir()
> > > kobj->sd = NULL
> > > sysfs_put(sd)
> > > kernfs_put() // RCU free
> > > ext4_notify_error_sysfs(sbi)
> > > sysfs_notify(&sbi->s_kobj)
> > > kn = kobj->sd // stale pointer
> > > kernfs_get(kn) // UAF on freed kernfs_node
> > > ext4_journal_destroy()
> > > flush_work(&sbi->s_sb_upd_work)
> > >
> > > The original blamed commit needed procfs removal before the work
> > > flush to prevent /proc/fs/ext4/xx/mb_groups reads from queuing new error
> > > work. But it bundled procfs removal and kobject_del together in
> > > ext4_unregister_sysfs(), causing the sysfs kobject to be torn down too
> > > early.
> > >
> > > The correct teardown ordering has three constraints:
> > >
> > > 1. procfs removal must happen before flushing s_sb_upd_work, to prevent
> > > /proc reads from queuing new error work that would BUG_ON.
> > > 2. sysfs kobject removal must happen after flushing s_sb_upd_work, since
> > > the work calls sysfs_notify() which accesses the kernfs_node.
> > > 3. sysfs kobject removal must happen before jbd2_journal_destroy(), since
> > > userspace could read the journal_task sysfs attribute and dereference
> > > j_task after the journal thread has been killed.
> > >
> > > Fix this by:
> > > - Adding ext4_sb_release_proc() to remove procfs entries early.
> > > - Splitting ext4_journal_destroy() into ext4_journal_stop_work() and
> > > ext4_journal_finish(), so that ext4_unregister_sysfs() can be placed
> > > between them to satisfy all three ordering constraints.
> > >
> > > Fixes: b98535d09179 ("ext4: fix bug_on in start_this_handle during umount filesystem")
> > > Cc: Jiayuan Chen <jiayuan.chen@linux.dev>
> > > Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> > Thanks for the analysis and the patch! I fully agree with your analysis but
> > I think your solution shows that the teardown sequence is just too subtle
> > (too many different dependencies). Ideally, we'd instead modify
> > ext4_notify_error_sysfs() to detect sysfs is already torn down by
> > ext4_unregister_sysfs() and do nothing. We can check
> > sbi->s_kobj.state_in_sysfs for that. The only trouble with this is that
> > sysfs_notify() could still race with kobject_del() so we also need some
> > locking in ext4_unregister_sysfs() locking out parallel
> > ext4_notify_error_sysfs() and we probably need to introduce a separate
> > mutex for that. What do you think?
> >
> > Honza
> >
>
>
> Hi Honza,
>
> Thanks for the suggestion !
>
> I tried the approach you described — having ext4_notify_error_sysfs() check
> s_kobj.state_in_sysfs and using a
> dedicated mutex to serialize against kobject_del() in
> ext4_unregister_sysfs(). It is indeed much simpler than my original
> approach of reordering the teardown sequence.
Yes, the patch looks good to me. Please create a proper patch submission
with changelog etc. and I can give you my reviewed-by tag :).
Honza
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index b76966dc06c0..c012e85d2515 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1574,6 +1574,7 @@ struct ext4_sb_info {
> struct proc_dir_entry *s_proc;
> struct kobject s_kobj;
> struct completion s_kobj_unregister;
> + struct mutex s_error_notify_mutex; /* protects sysfs_notify vs
> kobject_del */
> struct super_block *s_sb;
> struct buffer_head *s_mmp_bh;
>
> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
> index d2ecc1026c0c..f2505988e010 100644
> --- a/fs/ext4/sysfs.c
> +++ b/fs/ext4/sysfs.c
> @@ -597,7 +597,10 @@ static const struct kobj_type ext4_feat_ktype = {
>
> void ext4_notify_error_sysfs(struct ext4_sb_info *sbi)
> {
> - sysfs_notify(&sbi->s_kobj, NULL, "errors_count");
> + mutex_lock(&sbi->s_error_notify_mutex);
> + if (sbi->s_kobj.state_in_sysfs)
> + sysfs_notify(&sbi->s_kobj, NULL, "errors_count");
> + mutex_unlock(&sbi->s_error_notify_mutex);
> }
>
> static struct kobject *ext4_root;
> @@ -610,6 +613,7 @@ int ext4_register_sysfs(struct super_block *sb)
> int err;
>
> init_completion(&sbi->s_kobj_unregister);
> + mutex_init(&sbi->s_error_notify_mutex);
> err = kobject_init_and_add(&sbi->s_kobj, &ext4_sb_ktype, ext4_root,
> "%s", sb->s_id);
> if (err) {
> @@ -644,7 +648,10 @@ void ext4_unregister_sysfs(struct super_block *sb)
>
> if (sbi->s_proc)
> remove_proc_subtree(sb->s_id, ext4_proc_root);
> +
> + mutex_lock(&sbi->s_error_notify_mutex);
> kobject_del(&sbi->s_kobj);
> + mutex_unlock(&sbi->s_error_notify_mutex);
> }
>
> int __init ext4_init_sysfs(void)
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH] ext4: xattr: fix out-of-bounds access in ext4_xattr_set_entry
From: Theodore Tso @ 2026-03-18 14:45 UTC (permalink / raw)
To: ZhengYuan Huang
Cc: adilger.kernel, tahsin, linux-ext4, linux-kernel, baijiaju1990,
r33s3n6, zzzccc427, stable
In-Reply-To: <20260318075842.3341370-1-gality369@gmail.com>
On Wed, Mar 18, 2026 at 03:58:42PM +0800, ZhengYuan Huang wrote:
> [BUG]
> KASAN reports show out-of-bounds and use-after-free memory accesses when
> ext4_xattr_set_entry() processes corrupted on-disk xattr entries:
Can you send us a pointer to the reproducer? And does the reproducer
involve actively modifying the mounted file system image, either via
the block device or the underlying file (if a loop device is being used)?
- Ted
^ permalink raw reply
* Re: [PATCH] ext4: xattr: fix out-of-bounds access in ext4_xattr_set_entry
From: Greg KH @ 2026-03-18 8:09 UTC (permalink / raw)
To: ZhengYuan Huang
Cc: tytso, adilger.kernel, tahsin, linux-ext4, linux-kernel,
baijiaju1990, r33s3n6, zzzccc427, stable
In-Reply-To: <20260318075842.3341370-1-gality369@gmail.com>
On Wed, Mar 18, 2026 at 03:58:42PM +0800, ZhengYuan Huang wrote:
> [BUG]
> KASAN reports show out-of-bounds and use-after-free memory accesses when
> ext4_xattr_set_entry() processes corrupted on-disk xattr entries:
Does runing fsck on the disk image before mounting it catch this error?
thanks,
greg k-h
^ permalink raw reply
* [PATCH] ext4: xattr: fix out-of-bounds access in ext4_xattr_set_entry
From: ZhengYuan Huang @ 2026-03-18 7:58 UTC (permalink / raw)
To: tytso, adilger.kernel, tahsin
Cc: linux-ext4, linux-kernel, baijiaju1990, r33s3n6, zzzccc427,
ZhengYuan Huang, stable
[BUG]
KASAN reports show out-of-bounds and use-after-free memory accesses when
ext4_xattr_set_entry() processes corrupted on-disk xattr entries:
BUG: KASAN: slab-out-of-bounds in ext4_xattr_set_entry+0xfc2/0x1f40 fs/ext4/xattr.c:1735
Write of size 12 at addr ffff88801a249af4 [slab OOB]
Call Trace:
...
ext4_xattr_set_entry+0xfc2/0x1f40 fs/ext4/xattr.c:1735
ext4_xattr_ibody_set+0x396/0x5a0 fs/ext4/xattr.c:2268
ext4_destroy_inline_data_nolock+0x25e/0x560 fs/ext4/inline.c:463
ext4_convert_inline_data_nolock+0x186/0xa80 fs/ext4/inline.c:1105
ext4_try_add_inline_entry+0x58e/0x960 fs/ext4/inline.c:1224
ext4_add_entry+0x6d2/0xce0 fs/ext4/namei.c:2389
ext4_rename+0x133c/0x2490 fs/ext4/namei.c:3929
ext4_rename2+0x1de/0x2c0 fs/ext4/namei.c:4208
vfs_rename+0xd42/0x1d50 fs/namei.c:5216
do_renameat2+0x715/0xb60 fs/namei.c:5364
...
BUG: KASAN: use-after-free in ext4_xattr_set_entry+0xfd3/0x1f40 fs/ext4/xattr.c:1736
Write of size 65796 at addr ffff88802feb6ee8 [UAF across page boundary]
[CAUSE]
During inode load, xattr_check_inode() validates the ibody xattr entries
found in the inode at that time, and ext4_read_inode_extra() sets
EXT4_STATE_XATTR after the validation succeeds.
Later, when updating an ibody xattr, ext4_xattr_ibody_find() does not rely
on those already validated contents. It calls ext4_get_inode_loc() and
reads the inode table block again, so the entry eventually passed to
ext4_xattr_set_entry() comes from a new on-disk read. xattr_find_entry()
may return that entry based on its name, but does not revalidate its
e_value_offs and e_value_size before they are dereferenced.
Therefore, if the inode table block is modified between inode load and the
later xattr update, the code ends up validating one version of the xattr
data and using another. ext4_xattr_set_entry() may then consume corrupted
e_value_offs/e_value_size fields from the newly read entry, which can cause
out-of-bounds accesses, size_t underflow, and use-after-free.
[FIX]
Fix this by validating the target entry's value offset and size in
ext4_xattr_set_entry() before using them. Reject invalid entries
with -EFSCORRUPTED, consistent with the checks already enforced by
check_xattrs() for ibody xattrs.
Fixes: dec214d00e0d7 ("ext4: xattr inode deduplication")
Cc: stable@vger.kernel.org
Signed-off-by: ZhengYuan Huang <gality369@gmail.com>
---
fs/ext4/xattr.c | 58 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 42 insertions(+), 16 deletions(-)
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index ce7253b3f549..3ebfe2dfcae9 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1638,6 +1638,48 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
EXT4_XATTR_SIZE(le32_to_cpu(here->e_value_size)) : 0;
new_size = (i->value && !in_inode) ? EXT4_XATTR_SIZE(i->value_len) : 0;
+ /* Compute min_offs and last. */
+ last = s->first;
+ for (; !IS_LAST_ENTRY(last); last = next) {
+ next = EXT4_XATTR_NEXT(last);
+ if ((void *)next >= s->end) {
+ EXT4_ERROR_INODE(inode, "corrupted xattr entries");
+ ret = -EFSCORRUPTED;
+ goto out;
+ }
+ if (!last->e_value_inum && last->e_value_size) {
+ size_t offs = le16_to_cpu(last->e_value_offs);
+
+ if (offs < min_offs)
+ min_offs = offs;
+ }
+ }
+
+ /*
+ * Validate the value range before dereferencing e_value_offs / e_value_size.
+ * This mirrors check_xattrs() for the entry we are about to touch.
+ */
+ if (!s->not_found && !here->e_value_inum && here->e_value_size) {
+ u16 offs = le16_to_cpu(here->e_value_offs);
+ size_t size = le32_to_cpu(here->e_value_size);
+ void *value;
+
+ if (offs > s->end - s->base) {
+ EXT4_ERROR_INODE(inode, "corrupted xattr entry: invalid value offset");
+ ret = -EFSCORRUPTED;
+ goto out;
+ }
+
+ value = s->base + offs;
+ if (value < (void *)last + sizeof(__u32) ||
+ size > s->end - value ||
+ EXT4_XATTR_SIZE(size) > s->end - value) {
+ EXT4_ERROR_INODE(inode, "corrupted xattr entry: invalid value range");
+ ret = -EFSCORRUPTED;
+ goto out;
+ }
+ }
+
/*
* Optimization for the simple case when old and new values have the
* same padded sizes. Not applicable if external inodes are involved.
@@ -1657,22 +1699,6 @@ static int ext4_xattr_set_entry(struct ext4_xattr_info *i,
goto update_hash;
}
- /* Compute min_offs and last. */
- last = s->first;
- for (; !IS_LAST_ENTRY(last); last = next) {
- next = EXT4_XATTR_NEXT(last);
- if ((void *)next >= s->end) {
- EXT4_ERROR_INODE(inode, "corrupted xattr entries");
- ret = -EFSCORRUPTED;
- goto out;
- }
- if (!last->e_value_inum && last->e_value_size) {
- size_t offs = le16_to_cpu(last->e_value_offs);
- if (offs < min_offs)
- min_offs = offs;
- }
- }
-
/* Check whether we have enough space. */
if (i->value) {
size_t free;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v18 00/42] DEPT(DEPendency Tracker)
From: Yunseong Kim @ 2026-03-18 5:58 UTC (permalink / raw)
To: torvalds, mingo, Byungchul Park, linux-kernel
Cc: kernel_team, damien.lemoal, linux-ide, adilger.kernel, linux-ext4,
peterz, will, tglx, rostedt, joel, sashal, daniel.vetter,
duyuyang, johannes.berg, tj, tytso, willy, david, amir73il,
gregkh, kernel-team, linux-mm, akpm, mhocko, minchan, hannes,
vdavydov.dev, sj, jglisse, dennis, cl, penberg, rientjes, vbabka,
ngupta, linux-block, josef, linux-fsdevel, jack, jlayton,
dan.j.williams, hch, djwong, dri-devel, rodrigosiqueiramelo,
melissa.srw, hamohammed.sa, harry.yoo, chris.p.wilson,
gwan-gyeong.mun, max.byungchul.park, boqun.feng, longman,
yunseong.kim, yeoreum.yun, netdev, matthew.brost, her0gyugyu,
corbet, catalin.marinas, bp, x86, hpa, luto, sumit.semwal,
gustavo, christian.koenig, andi.shyti, arnd, lorenzo.stoakes,
Liam.Howlett, rppt, surenb, mcgrof, petr.pavlu, da.gomez,
samitolvanen, paulmck, frederic, neeraj.upadhyay, joelagnelf,
josh, urezki, mathieu.desnoyers, jiangshanlai, qiang.zhang,
juri.lelli, vincent.guittot, dietmar.eggemann, bsegall, mgorman,
vschneid, chuck.lever, neil, okorniev, Dai.Ngo, tom, trondmy,
anna, kees, bigeasy, clrkwllms, mark.rutland, ada.coupriediaz,
kristina.martsenko, wangkefeng.wang, broonie, kevin.brodsky, dwmw,
shakeel.butt, ast, ziy, yuzhao, baolin.wang, usamaarif642,
joel.granados, richard.weiyang, geert+renesas, tim.c.chen, linux,
alexander.shishkin, lillian, chenhuacai, francesco,
guoweikang.kernel, link, jpoimboe, masahiroy, brauner,
thomas.weissschuh, oleg, mjguzik, andrii, wangfushuai, linux-doc,
linux-arm-kernel, linux-media, linaro-mm-sig, linux-i2c,
linux-arch, linux-modules, rcu, linux-nfs, linux-rt-devel,
2407018371, dakr, miguel.ojeda.sandonis, neilb, bagasdotme,
wsa+renesas, dave.hansen, geert, ojeda, alex.gaynor, gary,
bjorn3_gh, lossin, a.hindborg, aliceryhl, tmgross, rust-for-linux
In-Reply-To: <20260317044459.GA27353@system.software.com>
Hi all,
I would like to support this proposal by noting that we are currently tracking
a number of kernel bugs uniquely detectable by DEPT.
In my experience running kernel fuzzers, many of these issues were previously
flagged as hung task and left unresolved because they involved wait/event
dependencies for PG_locked and writeback that are difficult to track otherwise.
DEPT provides the clarity necessary to address these cases. I believe having
DEPT in the mainline tree will be a significant asset for identifying and fixing
these kinds of hidden, latent bugs.
Best regards,
Yunseong
On 3/17/26 1:44 PM, Byungchul Park wrote:
> On Fri, Dec 05, 2025 at 04:18:13PM +0900, Byungchul Park wrote:
>> I'm happy to see that DEPT reported real problems in practice:
>>
>> https://lore.kernel.org/lkml/6383cde5-cf4b-facf-6e07-1378a485657d@I-love.SAKURA.ne.jp/
>> https://lore.kernel.org/lkml/1674268856-31807-1-git-send-email-byungchul.park@lge.com/
>> https://lore.kernel.org/all/b6e00e77-4a8c-4e05-ab79-266bf05fcc2d@igalia.com/
>>
>> I’ve added documentation describing DEPT — this should help you
>> understand what DEPT is and how it works. You can use DEPT simply by
>> enabling CONFIG_DEPT and checking dmesg at runtime.
>> ---
>>
>> Hi Linus and folks,
>
> Hi Linus and Ingo,
>
> Although the DEPT tool still has areas for improvement, I am confident
> it employs the most appropriate method for tracking dependencies within
> Linux kernel.
>
> If it is to be maintained in the source tree, which subsystem would be
> the most suitable for its management? Personally, I believe introducing
> a dedicated dependency subsystem under 'kernel/' would be ideal, though
> managing it within the already well-maintained scheduler or locking
> subsystems might be more practical.
>
> Since DEPT tracks not only locking mechanisms but also general sleep and
> wake events, I have avoided placing it under the locking subsystem thus
> far. If you have alternative or more fitting suggestions, I would
> appreciate your input.
>
> Thanks.
>
> Byungchul
>>
>> I’ve been developing a tool to detect deadlock possibilities by tracking
>> waits/events — rather than lock acquisition order — to cover all the
>> synchronization mechanisms. To summarize the design rationale, starting
>> from the problem statement, through analysis, to the solution:
>>
>> CURRENT STATUS
>> --------------
>> Lockdep tracks lock acquisition order to identify deadlock conditions.
>> Additionally, it tracks IRQ state changes — via {en,dis}able — to
>> detect cases where locks are acquired unintentionally during
>> interrupt handling.
>>
>> PROBLEM
>> -------
>> Waits and their associated events that are never reachable can
>> eventually lead to deadlocks. However, since Lockdep focuses solely
>> on lock acquisition order, it has inherent limitations when handling
>> waits and events.
>>
>> Moreover, by tracking only lock acquisition order, Lockdep cannot
>> properly handle read locks or cross-event scenarios — such as
>> wait_for_completion() and complete() — making it increasingly
>> inadequate as a general-purpose deadlock detection tool.
>>
>> SOLUTION
>> --------
>> Once again, waits and their associated events that are never
>> reachable can eventually lead to deadlocks. The new solution, DEPT,
>> focuses directly on waits and events. DEPT monitors waits and events,
>> and reports them when any become unreachable.
>>
>> DEPT provides:
>>
>> * Correct handling of read locks.
>> * Support for general waits and events.
>> * Continuous operation, even after multiple reports.
>> * Simple, intuitive annotation APIs.
>>
>> There are still false positives, and some are already being worked on
>> for suppression. Especially splitting the folio class into several
>> appropriate classes e.g. block device mapping class and regular file
>> mapping class, is currently under active development by me and Yeoreum
>> Yun.
>>
>> Anyway, these efforts will need to continue for a while, as we’ve seen
>> with lockdep over two decades. DEPT is tagged as EXPERIMENTAL in
>> Kconfig — meaning it’s not yet suitable for use as an automation tool.
>>
>> However, for those who are interested in using DEPT to analyze complex
>> synchronization patterns and extract dependency insights, DEPT would be
>> a great tool for the purpose.
>>
>> Thanks for your support and contributions to:
>>
>> Harry Yoo <harry.yoo@oracle.com>
>> Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>> Yunseong Kim <ysk@kzalloc.com>
>> Yeoreum Yun <yeoreum.yun@arm.com>
>>
>> FAQ
>> ---
>> Q. Is this the first attempt to solve this problem?
>> A. No. The cross-release feature (commit b09be676e0ff2) attempted to
>> address it — as a Lockdep extension. It was merged, but quickly
>> reverted, because:
>>
>> While it uncovered valuable hidden issues, it also introduced false
>> positives. Since these false positives mask further real problems
>> with Lockdep — and developers strongly dislike them — the feature was
>> rolled back.
>>
>> Q. Why wasn’t DEPT built as a Lockdep extension?
>> A. Lockdep is the result of years of work by kernel developers — and is
>> now very stable. But I chose to build DEPT separately, because:
>>
>> While reusing BFS(Breadth First Search) and Lockdep’s hashing is
>> beneficial, the rest of the system must be rebuilt from scratch to
>> align with DEPT’s wait-event model — since Lockdep was originally
>> designed for tracking lock acquisition orders, not wait-event
>> dependencies.
>>
>> Q. Do you plan to replace Lockdep entirely?
>> A. Not at all — Lockdep still plays a vital role in validating correct
>> lock usage. While its dependency-checking logic should eventually be
>> superseded by DEPT, the rest of its functionality should stay.
>>
>> Q. Should we replace the dependency check immediately?
>> A. Absolutely not. Lockdep’s stability is the result of years of hard
>> work by kernel developers. Lockdep and DEPT should run side by side
>> until DEPT matures.
>>
>> Q. Stronger detection often leads to more false positives — which was a
>> major pain point when cross-release was added. Is DEPT designed to
>> handle this?
>> A. Yes. DEPT’s simple, generalized design enables flexible reporting —
>> so while false positives still need fixing, they’re far less
>> disruptive than they were under the Lockdep extension, cross-release.
>>
>> Q. Why not fix all false positives out-of-tree before merging?
>> A. Since the affected subsystems span the entire kernel, like Lockdep,
>> which has relied on annotations to avoid false positives over the
>> last two decades, DEPT too will require the annotation efforts.
>>
>> Performing annotation work within the mainline will help us add
>> annotations more appropriately and will also make DEPT a useful tool
>> for a wider range of users more quickly.
>>
>> CONFIG_DEPT is marked EXPERIMENTAL, so it’s opt-in. Some users are
>> already interested in using DEPT to analyze complex synchronization
>> patterns and extract dependency insights.
>>
>> Byungchul
>> ---
>> Changes from v17:
>>
>> 1. Rebase on the mainline as of 2025 Dec 5.
>> 2. Convert the documents' format from txt to rst. (feedbacked
>> by Jonathan Corbet and Bagas Sanjaya)
>> 3. Move the documents from 'Documentation/dependency' to
>> 'Documentation/dev-tools'. (feedbakced by Jonathan Corbet)
>> 4. Improve the documentation. (feedbacked by NeilBrown)
>> 5. Use a common function, enter_from_user_mode(), instead of
>> arch specific code, to notice context switch from user mode.
>> (feedbacked by Dave Hansen, Mark Rutland, and Mark Brown)
>> 6. Resolve the header dependency issue by using dept's internal
>> header, instead of relocating 'struct llist_{head,node}' to
>> another header. (feedbacked by Greg KH)
>> 7. Improve page(or folio) usage type APIs.
>> 8. Add rust helper for wait_for_completion(). (feedbacked by
>> Guangbo Cui, Boqun Feng, and Danilo Krummrich)
>> 9. Refine some commit messages.
>>
>> Changes from v16:
>>
>> 1. Rebase on v6.17.
>> 2. Fix a false positive from rcu (by Yunseong Kim)
>> 3. Introduce APIs to set page's usage, dept_set_page_usage() and
>> dept_reset_page_usage() to avoid false positives.
>> 4. Consider lock_page() as a potential wait unconditionally.
>> 5. Consider folio_lock_killable() as a potential wait
>> unconditionally.
>> 6. Add support for tracking PG_writeback waits and events.
>> 7. Fix two build errors due to the additional debug information
>> added by dept. (by Yunseong Kim)
>>
>> Changes from v15:
>>
>> 1. Fix typo and improve comments and commit messages (feedbacked
>> by ALOK TIWARI, Waiman Long, and kernel test robot).
>> 2. Do not stop dept on detection of cicular dependency of
>> recover event, allowing to keep reporting.
>> 3. Add SK hynix to copyright.
>> 4. Consider folio_lock() as a potential wait unconditionally.
>> 5. Fix Kconfig dependency bug (feedbacked by kernel test rebot).
>> 6. Do not suppress reports that involve classes even that have
>> already involved in other reports, allowing to keep
>> reporting.
>>
>> Changes from v14:
>>
>> 1. Rebase on the current latest, v6.15-rc6.
>> 2. Refactor dept code.
>> 3. With multi event sites for a single wait, even if an event
>> forms a circular dependency, the event can be recovered by
>> other event(or wake up) paths. Even though informing the
>> circular dependency is worthy but it should be suppressed
>> once informing it, if it doesn't lead an actual deadlock. So
>> introduce APIs to annotate the relationship between event
>> site and recover site, that are, event_site() and
>> dept_recover_event().
>> 4. wait_for_completion() worked with dept map embedded in struct
>> completion. However, it generates a few false positves since
>> all the waits using the instance of struct completion, share
>> the map and key. To avoid the false positves, make it not to
>> share the map and key but each wait_for_completion() caller
>> have its own key by default. Of course, external maps also
>> can be used if needed.
>> 5. Fix a bug about hardirq on/off tracing.
>> 6. Implement basic unit test for dept.
>> 7. Add more supports for dma fence synchronization.
>> 8. Add emergency stop of dept e.g. on panic().
>> 9. Fix false positives by mmu_notifier_invalidate_*().
>> 10. Fix recursive call bug by DEPT_WARN_*() and DEPT_STOP().
>> 11. Fix trivial bugs in DEPT_WARN_*() and DEPT_STOP().
>> 12. Fix a bug that a spin lock, dept_pool_spin, is used in
>> both contexts of irq disabled and enabled without irq
>> disabled.
>> 13. Suppress reports with classes, any of that already have
>> been reported, even though they have different chains but
>> being barely meaningful.
>> 14. Print stacktrace of the wait that an event is now waking up,
>> not only stacktrace of the event.
>> 15. Make dept aware of lockdep_cmp_fn() that is used to avoid
>> false positives in lockdep so that dept can also avoid them.
>> 16. Do do_event() only if there are no ecxts have been
>> delimited.
>> 17. Fix a bug that was not synchronized for stage_m in struct
>> dept_task, using a spin lock, dept_task()->stage_lock.
>> 18. Fix a bug that dept didn't handle the case that multiple
>> ttwus for a single waiter can be called at the same time
>> e.i. a race issue.
>> 19. Distinguish each kernel context from others, not only by
>> system call but also by user oriented fault so that dept can
>> work with more accuracy information about kernel context.
>> That helps to avoid a few false positives.
>> 20. Limit dept's working to x86_64 and arm64.
>>
>> Changes from v13:
>>
>> 1. Rebase on the current latest version, v6.9-rc7.
>> 2. Add 'dept' documentation describing dept APIs.
>>
>> Changes from v12:
>>
>> 1. Refine the whole document for dept.
>> 2. Add 'Interpret dept report' section in the document, using a
>> deadlock report obtained in practice. Hope this version of
>> document helps guys understand dept better.
>>
>> https://lore.kernel.org/lkml/6383cde5-cf4b-facf-6e07-1378a485657d@I-love.SAKURA.ne.jp/#t
>> https://lore.kernel.org/lkml/1674268856-31807-1-git-send-email-byungchul.park@lge.com/
>>
>> Changes from v11:
>>
>> 1. Add 'dept' documentation describing the concept of dept.
>> 2. Rewrite the commit messages of the following commits for
>> using weaker lockdep annotation, for better description.
>>
>> fs/jbd2: Use a weaker annotation in journal handling
>> cpu/hotplug: Use a weaker annotation in AP thread
>>
>> (feedbacked by Thomas Gleixner)
>>
>> Changes from v10:
>>
>> 1. Fix noinstr warning when building kernel source.
>> 2. dept has been reporting some false positives due to the folio
>> lock's unfairness. Reflect it and make dept work based on
>> dept annotaions instead of just wait and wake up primitives.
>> 3. Remove the support for PG_writeback while working on 2. I
>> will add the support later if needed.
>> 4. dept didn't print stacktrace for [S] if the participant of a
>> deadlock is not lock mechanism but general wait and event.
>> However, it made hard to interpret the report in that case.
>> So add support to print stacktrace of the requestor who asked
>> the event context to run - usually a waiter of the event does
>> it just before going to wait state.
>> 5. Give up tracking raw_local_irq_{disable,enable}() since it
>> totally messed up dept's irq tracking. So make it work in the
>> same way as lockdep does. I will consider it once any false
>> positives by those are observed again.
>> 6. Change the manual rwsem_acquire_read(->j_trans_commit_map)
>> annotation in fs/jbd2/transaction.c to the try version so
>> that it works as much as it exactly needs.
>> 7. Remove unnecessary 'inline' keyword in dept.c and add
>> '__maybe_unused' to a needed place.
>>
>> Changes from v9:
>>
>> 1. Fix a bug. SDT tracking didn't work well because of my big
>> mistake that I should've used waiter's map to indentify its
>> class but it had been working with waker's one. FYI,
>> PG_locked and PG_writeback weren't affected. They still
>> worked well. (reported by YoungJun)
>>
>> Changes from v8:
>>
>> 1. Fix build error by adding EXPORT_SYMBOL(PG_locked_map) and
>> EXPORT_SYMBOL(PG_writeback_map) for kernel module build -
>> appologize for that. (reported by kernel test robot)
>> 2. Fix build error by removing header file's circular dependency
>> that was caused by "atomic.h", "kernel.h" and "irqflags.h",
>> which I introduced - appolgize for that. (reported by kernel
>> test robot)
>>
>> Changes from v7:
>>
>> 1. Fix a bug that cannot track rwlock dependency properly,
>> introduced in v7. (reported by Boqun and lockdep selftest)
>> 2. Track wait/event of PG_{locked,writeback} more aggressively
>> assuming that when a bit of PG_{locked,writeback} is cleared
>> there might be waits on the bit. (reported by Linus, Hillf
>> and syzbot)
>> 3. Fix and clean bad style code e.i. unnecessarily introduced
>> a randome pattern and so on. (pointed out by Linux)
>> 4. Clean code for applying dept to wait_for_completion().
>>
>> Changes from v6:
>>
>> 1. Tie to task scheduler code to track sleep and try_to_wake_up()
>> assuming sleeps cause waits, try_to_wake_up()s would be the
>> events that those are waiting for, of course with proper dept
>> annotations, sdt_might_sleep_weak(), sdt_might_sleep_strong()
>> and so on. For these cases, class is classified at sleep
>> entrance rather than the synchronization initialization code.
>> Which would extremely reduce false alarms.
>> 2. Remove the dept associated instance in each page struct for
>> tracking dependencies by PG_locked and PG_writeback thanks to
>> the 1. work above.
>> 3. Introduce CONFIG_dept_AGGRESIVE_TIMEOUT_WAIT to suppress
>> reports that waits with timeout set are involved, for those
>> who don't like verbose reporting.
>> 4. Add a mechanism to refill the internal memory pools on
>> running out so that dept could keep working as long as free
>> memory is available in the system.
>> 5. Re-enable tracking hashed-waitqueue wait. That's going to no
>> longer generate false positives because class is classified
>> at sleep entrance rather than the waitqueue initailization.
>> 6. Refactor to make it easier to port onto each new version of
>> the kernel.
>> 7. Apply dept to dma fence.
>> 8. Do trivial optimizaitions.
>>
>> Changes from v5:
>>
>> 1. Use just pr_warn_once() rather than WARN_ONCE() on the lack
>> of internal resources because WARN_*() printing stacktrace is
>> too much for informing the lack. (feedback from Ted, Hyeonggon)
>> 2. Fix trivial bugs like missing initializing a struct before
>> using it.
>> 3. Assign a different class per task when handling onstack
>> variables for waitqueue or the like. Which makes dept
>> distinguish between onstack variables of different tasks so
>> as to prevent false positives. (reported by Hyeonggon)
>> 4. Make dept aware of even raw_local_irq_*() to prevent false
>> positives. (reported by Hyeonggon)
>> 5. Don't consider dependencies between the events that might be
>> triggered within __schedule() and the waits that requires
>> __schedule(), real ones. (reported by Hyeonggon)
>> 6. Unstage the staged wait that has prepare_to_wait_event()'ed
>> *and* yet to get to __schedule(), if we encounter __schedule()
>> in-between for another sleep, which is possible if e.g. a
>> mutex_lock() exists in 'condition' of ___wait_event().
>> 7. Turn on CONFIG_PROVE_LOCKING when CONFIG_DEPT is on, to rely
>> on the hardirq and softirq entrance tracing to make dept more
>> portable for now.
>>
>> Changes from v4:
>>
>> 1. Fix some bugs that produce false alarms.
>> 2. Distinguish each syscall context from another *for arm64*.
>> 3. Make it not warn it but just print it in case dept ring
>> buffer gets exhausted. (feedback from Hyeonggon)
>> 4. Explicitely describe "EXPERIMENTAL" and "dept might produce
>> false positive reports" in Kconfig. (feedback from Ted)
>>
>> Changes from v3:
>>
>> 1. dept shouldn't create dependencies between different depths
>> of a class that were indicated by *_lock_nested(). dept
>> normally doesn't but it does once another lock class comes
>> in. So fixed it. (feedback from Hyeonggon)
>> 2. dept considered a wait as a real wait once getting to
>> __schedule() even if it has been set to TASK_RUNNING by wake
>> up sources in advance. Fixed it so that dept doesn't consider
>> the case as a real wait. (feedback from Jan Kara)
>> 3. Stop tracking dependencies with a map once the event
>> associated with the map has been handled. dept will start to
>> work with the map again, on the next sleep.
>>
>> Changes from v2:
>>
>> 1. Disable dept on bit_wait_table[] in sched/wait_bit.c
>> reporting a lot of false positives, which is my fault.
>> Wait/event for bit_wait_table[] should've been tagged in a
>> higher layer for better work, which is a future work.
>> (feedback from Jan Kara)
>> 2. Disable dept on crypto_larval's completion to prevent a false
>> positive.
>>
>> Changes from v1:
>>
>> 1. Fix coding style and typo. (feedback from Steven)
>> 2. Distinguish each work context from another in workqueue.
>> 3. Skip checking lock acquisition with nest_lock, which is about
>> correct lock usage that should be checked by lockdep.
>>
>> Changes from RFC(v0):
>>
>> 1. Prevent adding a wait tag at prepare_to_wait() but __schedule().
>> (feedback from Linus and Matthew)
>> 2. Use try version at lockdep_acquire_cpus_lock() annotation.
>> 3. Distinguish each syscall context from another.
>>
>> Byungchul Park (41):
>> dept: implement DEPT(DEPendency Tracker)
>> dept: add single event dependency tracker APIs
>> dept: add lock dependency tracker APIs
>> dept: tie to lockdep and IRQ tracing
>> dept: add proc knobs to show stats and dependency graph
>> dept: distinguish each kernel context from another
>> dept: distinguish each work from another
>> dept: add a mechanism to refill the internal memory pools on running
>> out
>> dept: record the latest one out of consecutive waits of the same class
>> dept: apply sdt_might_sleep_{start,end}() to
>> wait_for_completion()/complete()
>> dept: apply sdt_might_sleep_{start,end}() to swait
>> dept: apply sdt_might_sleep_{start,end}() to waitqueue wait
>> dept: apply sdt_might_sleep_{start,end}() to hashed-waitqueue wait
>> dept: apply sdt_might_sleep_{start,end}() to dma fence
>> dept: track timeout waits separately with a new Kconfig
>> dept: apply timeout consideration to wait_for_completion()/complete()
>> dept: apply timeout consideration to swait
>> dept: apply timeout consideration to waitqueue wait
>> dept: apply timeout consideration to hashed-waitqueue wait
>> dept: apply timeout consideration to dma fence wait
>> dept: make dept able to work with an external wgen
>> dept: track PG_locked with dept
>> dept: print staged wait's stacktrace on report
>> locking/lockdep: prevent various lockdep assertions when
>> lockdep_off()'ed
>> dept: add documents for dept
>> cpu/hotplug: use a weaker annotation in AP thread
>> dept: assign dept map to mmu notifier invalidation synchronization
>> dept: assign unique dept_key to each distinct dma fence caller
>> dept: make dept aware of lockdep_set_lock_cmp_fn() annotation
>> dept: make dept stop from working on debug_locks_off()
>> dept: assign unique dept_key to each distinct wait_for_completion()
>> caller
>> completion, dept: introduce init_completion_dmap() API
>> dept: introduce a new type of dependency tracking between multi event
>> sites
>> dept: add module support for struct dept_event_site and
>> dept_event_site_dep
>> dept: introduce event_site() to disable event tracking if it's
>> recoverable
>> dept: implement a basic unit test for dept
>> dept: call dept_hardirqs_off() in local_irq_*() regardless of irq
>> state
>> dept: introduce APIs to set page usage and use subclasses_evt for the
>> usage
>> dept: track PG_writeback with dept
>> SUNRPC: relocate struct rcu_head to the first field of struct rpc_xprt
>> mm: percpu: increase PERCPU_DYNAMIC_SIZE_SHIFT on DEPT and large
>> PAGE_SIZE
>>
>> Yunseong Kim (1):
>> rcu/update: fix same dept key collision between various types of RCU
>>
>> Documentation/dev-tools/dept.rst | 778 ++++++
>> Documentation/dev-tools/dept_api.rst | 125 +
>> drivers/dma-buf/dma-fence.c | 23 +-
>> include/asm-generic/vmlinux.lds.h | 13 +-
>> include/linux/completion.h | 124 +-
>> include/linux/dept.h | 402 +++
>> include/linux/dept_ldt.h | 78 +
>> include/linux/dept_sdt.h | 68 +
>> include/linux/dept_unit_test.h | 67 +
>> include/linux/dma-fence.h | 74 +-
>> include/linux/hardirq.h | 3 +
>> include/linux/irq-entry-common.h | 4 +
>> include/linux/irqflags.h | 21 +-
>> include/linux/local_lock_internal.h | 1 +
>> include/linux/lockdep.h | 105 +-
>> include/linux/lockdep_types.h | 3 +
>> include/linux/mm_types.h | 4 +
>> include/linux/mmu_notifier.h | 26 +
>> include/linux/module.h | 5 +
>> include/linux/mutex.h | 1 +
>> include/linux/page-flags.h | 217 +-
>> include/linux/pagemap.h | 37 +-
>> include/linux/percpu-rwsem.h | 2 +-
>> include/linux/percpu.h | 4 +
>> include/linux/rcupdate_wait.h | 13 +-
>> include/linux/rtmutex.h | 1 +
>> include/linux/rwlock_types.h | 1 +
>> include/linux/rwsem.h | 1 +
>> include/linux/sched.h | 118 +
>> include/linux/seqlock.h | 2 +-
>> include/linux/spinlock_types_raw.h | 3 +
>> include/linux/srcu.h | 2 +-
>> include/linux/sunrpc/xprt.h | 9 +-
>> include/linux/swait.h | 3 +
>> include/linux/wait.h | 3 +
>> include/linux/wait_bit.h | 3 +
>> init/init_task.c | 2 +
>> init/main.c | 2 +
>> kernel/Makefile | 1 +
>> kernel/cpu.c | 2 +-
>> kernel/dependency/Makefile | 5 +
>> kernel/dependency/dept.c | 3499 ++++++++++++++++++++++++++
>> kernel/dependency/dept_hash.h | 10 +
>> kernel/dependency/dept_internal.h | 314 +++
>> kernel/dependency/dept_object.h | 13 +
>> kernel/dependency/dept_proc.c | 94 +
>> kernel/dependency/dept_unit_test.c | 173 ++
>> kernel/exit.c | 1 +
>> kernel/fork.c | 2 +
>> kernel/locking/lockdep.c | 33 +
>> kernel/module/main.c | 19 +
>> kernel/rcu/rcu.h | 1 +
>> kernel/rcu/update.c | 5 +-
>> kernel/sched/completion.c | 62 +-
>> kernel/sched/core.c | 9 +
>> kernel/workqueue.c | 3 +
>> lib/Kconfig.debug | 48 +
>> lib/debug_locks.c | 2 +
>> lib/locking-selftest.c | 2 +
>> mm/filemap.c | 38 +
>> mm/mm_init.c | 3 +
>> mm/mmu_notifier.c | 31 +-
>> rust/helpers/completion.c | 5 +
>> 63 files changed, 6602 insertions(+), 121 deletions(-)
>> create mode 100644 Documentation/dev-tools/dept.rst
>> create mode 100644 Documentation/dev-tools/dept_api.rst
>> create mode 100644 include/linux/dept.h
>> create mode 100644 include/linux/dept_ldt.h
>> create mode 100644 include/linux/dept_sdt.h
>> create mode 100644 include/linux/dept_unit_test.h
>> create mode 100644 kernel/dependency/Makefile
>> create mode 100644 kernel/dependency/dept.c
>> create mode 100644 kernel/dependency/dept_hash.h
>> create mode 100644 kernel/dependency/dept_internal.h
>> create mode 100644 kernel/dependency/dept_object.h
>> create mode 100644 kernel/dependency/dept_proc.c
>> create mode 100644 kernel/dependency/dept_unit_test.c
>>
>>
>> base-commit: 43dfc13ca972988e620a6edb72956981b75ab6b0
>> --
>> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v1] ext4: fix use-after-free in update_super_work when racing with umount
From: Jiayuan Chen @ 2026-03-18 5:04 UTC (permalink / raw)
To: Jan Kara
Cc: linux-ext4, Theodore Ts'o, Andreas Dilger, Ritesh Harjani,
Ye Bin, linux-kernel
In-Reply-To: <jn23562fj73siqgtlzqknce34eqiazitghu55pyvczustuey32@zhx3wpu5eciy>
On 3/17/26 7:38 PM, Jan Kara wrote:
> On Fri 13-03-26 14:52:04, Jiayuan Chen wrote:
>> Commit b98535d09179 ("ext4: fix bug_on in start_this_handle during umount filesystem")
>> moved ext4_unregister_sysfs() before flushing s_sb_upd_work to prevent new
>> error work from being queued via /proc/fs/ext4/xx/mb_groups reads during
>> unmount. However, this introduced a use-after-free because
>> update_super_work calls ext4_notify_error_sysfs() -> sysfs_notify() which
>> accesses the kobject's kernfs_node after it has been freed:
>>
>> update_super_work ext4_put_super
>> ----------------- --------------
>> ext4_unregister_sysfs(sb)
>> kobject_del(&sbi->s_kobj)
>> __kobject_del()
>> sysfs_remove_dir()
>> kobj->sd = NULL
>> sysfs_put(sd)
>> kernfs_put() // RCU free
>> ext4_notify_error_sysfs(sbi)
>> sysfs_notify(&sbi->s_kobj)
>> kn = kobj->sd // stale pointer
>> kernfs_get(kn) // UAF on freed kernfs_node
>> ext4_journal_destroy()
>> flush_work(&sbi->s_sb_upd_work)
>>
>> The original blamed commit needed procfs removal before the work
>> flush to prevent /proc/fs/ext4/xx/mb_groups reads from queuing new error
>> work. But it bundled procfs removal and kobject_del together in
>> ext4_unregister_sysfs(), causing the sysfs kobject to be torn down too
>> early.
>>
>> The correct teardown ordering has three constraints:
>>
>> 1. procfs removal must happen before flushing s_sb_upd_work, to prevent
>> /proc reads from queuing new error work that would BUG_ON.
>> 2. sysfs kobject removal must happen after flushing s_sb_upd_work, since
>> the work calls sysfs_notify() which accesses the kernfs_node.
>> 3. sysfs kobject removal must happen before jbd2_journal_destroy(), since
>> userspace could read the journal_task sysfs attribute and dereference
>> j_task after the journal thread has been killed.
>>
>> Fix this by:
>> - Adding ext4_sb_release_proc() to remove procfs entries early.
>> - Splitting ext4_journal_destroy() into ext4_journal_stop_work() and
>> ext4_journal_finish(), so that ext4_unregister_sysfs() can be placed
>> between them to satisfy all three ordering constraints.
>>
>> Fixes: b98535d09179 ("ext4: fix bug_on in start_this_handle during umount filesystem")
>> Cc: Jiayuan Chen <jiayuan.chen@linux.dev>
>> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.dev>
> Thanks for the analysis and the patch! I fully agree with your analysis but
> I think your solution shows that the teardown sequence is just too subtle
> (too many different dependencies). Ideally, we'd instead modify
> ext4_notify_error_sysfs() to detect sysfs is already torn down by
> ext4_unregister_sysfs() and do nothing. We can check
> sbi->s_kobj.state_in_sysfs for that. The only trouble with this is that
> sysfs_notify() could still race with kobject_del() so we also need some
> locking in ext4_unregister_sysfs() locking out parallel
> ext4_notify_error_sysfs() and we probably need to introduce a separate
> mutex for that. What do you think?
>
> Honza
>
Hi Honza,
Thanks for the suggestion !
I tried the approach you described — having ext4_notify_error_sysfs()
check s_kobj.state_in_sysfs and using a
dedicated mutex to serialize against kobject_del() in
ext4_unregister_sysfs(). It is indeed much simpler than my original
approach of reordering the teardown sequence.
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index b76966dc06c0..c012e85d2515 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1574,6 +1574,7 @@ struct ext4_sb_info {
struct proc_dir_entry *s_proc;
struct kobject s_kobj;
struct completion s_kobj_unregister;
+ struct mutex s_error_notify_mutex; /* protects sysfs_notify vs
kobject_del */
struct super_block *s_sb;
struct buffer_head *s_mmp_bh;
diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index d2ecc1026c0c..f2505988e010 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -597,7 +597,10 @@ static const struct kobj_type ext4_feat_ktype = {
void ext4_notify_error_sysfs(struct ext4_sb_info *sbi)
{
- sysfs_notify(&sbi->s_kobj, NULL, "errors_count");
+ mutex_lock(&sbi->s_error_notify_mutex);
+ if (sbi->s_kobj.state_in_sysfs)
+ sysfs_notify(&sbi->s_kobj, NULL, "errors_count");
+ mutex_unlock(&sbi->s_error_notify_mutex);
}
static struct kobject *ext4_root;
@@ -610,6 +613,7 @@ int ext4_register_sysfs(struct super_block *sb)
int err;
init_completion(&sbi->s_kobj_unregister);
+ mutex_init(&sbi->s_error_notify_mutex);
err = kobject_init_and_add(&sbi->s_kobj, &ext4_sb_ktype, ext4_root,
"%s", sb->s_id);
if (err) {
@@ -644,7 +648,10 @@ void ext4_unregister_sysfs(struct super_block *sb)
if (sbi->s_proc)
remove_proc_subtree(sb->s_id, ext4_proc_root);
+
+ mutex_lock(&sbi->s_error_notify_mutex);
kobject_del(&sbi->s_kobj);
+ mutex_unlock(&sbi->s_error_notify_mutex);
}
int __init ext4_init_sysfs(void)
^ permalink raw reply related
* Re: [PATCH v3 1/5] ext4: call deactivate_super() in extents_kunit_exit()
From: yebin (H) @ 2026-03-18 2:21 UTC (permalink / raw)
To: Ojaswin Mujoo, Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack
In-Reply-To: <abZGBuItRfxDGe7m@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com>
On 2026/3/15 13:39, Ojaswin Mujoo wrote:
> On Sat, Mar 14, 2026 at 03:48:59PM +0800, Ye Bin wrote:
>> From: Ye Bin <yebin10@huawei.com>
>>
>> Call deactivate_super() is called in extents_kunit_exit() to cleanup
>> the file system resource.
>>
>> Fixes: cb1e0c1d1fad ("ext4: kunit tests for extent splitting and conversion")
>> Signed-off-by: Ye Bin <yebin10@huawei.com>
>> ---
>> fs/ext4/extents-test.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/ext4/extents-test.c b/fs/ext4/extents-test.c
>> index 5496b2c8e2cd..e3d23e3cda87 100644
>> --- a/fs/ext4/extents-test.c
>> +++ b/fs/ext4/extents-test.c
>> @@ -146,6 +146,7 @@ static void extents_kunit_exit(struct kunit *test)
>> struct ext4_sb_info *sbi = sb->s_fs_info;
>>
>> ext4_es_unregister_shrinker(sbi);
>> + deactivate_super(sbi->s_sb);
>> kfree(sbi);
>> kfree(k_ctx.k_ei);
>> kfree(k_ctx.k_data);
>> --
>> 2.34.1
>
> Hi,
>
> We need to keep this patch after Patch 2 because else there is a
> deadlock:
>
> extents_kunit_init()
> sget
> down_write(s_umount)
> extents_kunit_exit()
> deactivate_super
> down_write(s_umount)
>
> Regards,
> ojaswin
>
Thank you for your suggestion. I will change the order of the two
patches and release another version.
>
> .
>
^ permalink raw reply
* Re: [PATCH 2/3] ext4: add ext4_fc_eligible()
From: Andreas Dilger @ 2026-03-18 0:27 UTC (permalink / raw)
To: NeilBrown
Cc: Theodore Ts'o, Jan Kara, linux-ext4, linux-fsdevel,
Harshad Shirwadkar
In-Reply-To: <20260317224638.3809014-3-neilb@ownmail.net>
On Mar 17, 2026, at 16:39, NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
(add Harshad to CC list)
> Testing EXT4_MF_FC_INELIGIBLE is almost always combined with testing
> ext4_fc_disabled(). The code can be simplified by combining these two
> in a new ext4_fc_eligible().
>
> In ext4_fc_track_inode() this moves the ext4_fc_disabled() test after
> ext4_fc_mark_ineligible(), but as that is a non-op when
> ext4_fc_disabled() is true, this is no no consequence.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Nice cleanup. One minor semantic change in ext4_fc_track_inode() that
deserves a bit of scrutiny is that ext4_fc_mark_ineligible() is now
always called before ext4_fc_disabled() is checked. That looks OK,
since ext4_fc_mark_ineligible() checks ext4_fc_disabled() internally.
It looks slightly more efficient to check ext4_fc_eligible(inode->i_sb)
first?
Reviewed-by: Andreas Dilger <adilger@dilger.ca <mailto:adilger@dilger.ca>>
> @@ -557,16 +548,13 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
> if (S_ISDIR(inode->i_mode))
> return;
>
> - if (ext4_fc_disabled(inode->i_sb))
> - return;
> -
> if (ext4_should_journal_data(inode)) {
> ext4_fc_mark_ineligible(inode->i_sb,
> EXT4_FC_REASON_INODE_JOURNAL_DATA, handle);
> return;
> }
>
> - if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
> + if (!ext4_fc_eligible(inode->i_sb))
> return;
>
> /*
^ permalink raw reply
* Re: [PATCH 1/3] ext4: split __ext4_add_entry() out of ext4_add_entry()
From: Andreas Dilger @ 2026-03-18 0:20 UTC (permalink / raw)
To: NeilBrown; +Cc: Theodore Ts'o, Jan Kara, linux-ext4, linux-fsdevel
In-Reply-To: <20260317224638.3809014-2-neilb@ownmail.net>
On Mar 17, 2026, at 16:39, NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> __ext4_add_entry() is not given a dentry - just inodes and name.
> This will help the next patch which simplifies __ex4_link().
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Andreas Dilger <adilger@dilger.ca <mailto:adilger@dilger.ca>>
> ---
> fs/ext4/namei.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index c4b5e252af0e..768036a109d7 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -2353,10 +2353,10 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
> * may not sleep between calling this and putting something into
> * the entry, as someone else might have used it while you slept.
> */
> -static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> +static int __ext4_add_entry(handle_t *handle, struct inode *dir,
> + const struct qstr *d_name,
> struct inode *inode)
> {
> - struct inode *dir = d_inode(dentry->d_parent);
> struct buffer_head *bh = NULL;
> struct ext4_dir_entry_2 *de;
> struct super_block *sb;
> @@ -2373,13 +2373,10 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> sb = dir->i_sb;
> blocksize = sb->s_blocksize;
>
> - if (fscrypt_is_nokey_name(dentry))
> - return -ENOKEY;
> -
> - if (!generic_ci_validate_strict_name(dir, &dentry->d_name))
> + if (!generic_ci_validate_strict_name(dir, d_name))
> return -EINVAL;
>
> - retval = ext4_fname_setup_filename(dir, &dentry->d_name, 0, &fname);
> + retval = ext4_fname_setup_filename(dir, d_name, 0, &fname);
> if (retval)
> return retval;
>
> @@ -2460,6 +2457,16 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> return retval;
> }
>
> +static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
> + struct inode *inode)
> +{
> + struct inode *dir = d_inode(dentry->d_parent);
> +
> + if (fscrypt_is_nokey_name(dentry))
> + return -ENOKEY;
> + return __ext4_add_entry(handle, dir, &dentry->d_name, inode);
> +}
> +
> /*
> * Returns 0 for success, or a negative error value
> */
> --
> 2.50.0.107.gf914562f5916.dirty
>
^ permalink raw reply
* [PATCH 3/3] ext4: move dcache manipulation out of __ext4_link()
From: NeilBrown @ 2026-03-17 22:39 UTC (permalink / raw)
To: Theodore Ts'o, Andreas Dilger, Jan Kara; +Cc: linux-ext4, linux-fsdevel
In-Reply-To: <20260317224638.3809014-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
__ext4_link() has two callers.
- ext4_link() calls it during normal handling of the link() system
call or similar
- ext4_fc_replay_link_internal() calls it when replaying the journal
at mount time.
The former needs changes to dcache - instaniating the dentry to the
inode on success. The latter doesn't need or want any dcache
manipluation.
So move the manipulation out of __ext4_link() and do it in ext4_link()
only.
This requires:
- passing the qname from the dentry explicitly to __ext4_link.
The parent dir is already passed. The dentry is still passed
in the ext4_link() case purely for use by ext4_fc_track_link().
- passing the inode separately to ext4_fc_track_link() as the
dentry will not be instantiated yet.
- using __ext4_add_entry() in ext4_link, which doesn't need a dentry.
- moving ext4_inc_count(), ihold)(, d_instantiate(), drop_nlink() and
iput() calls out of __ext4_link() into ext4_link().
This substantially simplifies ext4_fc_replay_link_internal(), and
removes a use of d_alloc() which, it is planned, will be removed.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/ext4/ext4.h | 5 +++--
fs/ext4/fast_commit.c | 32 ++++----------------------------
fs/ext4/namei.c | 25 ++++++++++++++-----------
3 files changed, 21 insertions(+), 41 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 293f698b7042..e757e9cf9728 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2974,7 +2974,8 @@ void __ext4_fc_track_unlink(handle_t *handle, struct inode *inode,
void __ext4_fc_track_link(handle_t *handle, struct inode *inode,
struct dentry *dentry);
void ext4_fc_track_unlink(handle_t *handle, struct dentry *dentry);
-void ext4_fc_track_link(handle_t *handle, struct dentry *dentry);
+void ext4_fc_track_link(handle_t *handle, struct inode *inode,
+ struct dentry *dentry);
void __ext4_fc_track_create(handle_t *handle, struct inode *inode,
struct dentry *dentry);
void ext4_fc_track_create(handle_t *handle, struct dentry *dentry);
@@ -3719,7 +3720,7 @@ extern int ext4_handle_dirty_dirblock(handle_t *handle, struct inode *inode,
extern int __ext4_unlink(struct inode *dir, const struct qstr *d_name,
struct inode *inode, struct dentry *dentry);
extern int __ext4_link(struct inode *dir, struct inode *inode,
- struct dentry *dentry);
+ const struct qstr *d_name, struct dentry *dentry);
#define S_SHIFT 12
static const unsigned char ext4_type_by_mode[(S_IFMT >> S_SHIFT) + 1] = {
diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index 3ee302b73f45..175dda11f377 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -497,10 +497,9 @@ void __ext4_fc_track_link(handle_t *handle,
trace_ext4_fc_track_link(handle, inode, dentry, ret);
}
-void ext4_fc_track_link(handle_t *handle, struct dentry *dentry)
+void ext4_fc_track_link(handle_t *handle, struct inode *inode,
+ struct dentry *dentry)
{
- struct inode *inode = d_inode(dentry);
-
if (ext4_fc_eligible(inode->i_sb))
__ext4_fc_track_link(handle, inode, dentry);
}
@@ -1431,7 +1430,6 @@ static int ext4_fc_replay_link_internal(struct super_block *sb,
struct inode *inode)
{
struct inode *dir = NULL;
- struct dentry *dentry_dir = NULL, *dentry_inode = NULL;
struct qstr qstr_dname = QSTR_INIT(darg->dname, darg->dname_len);
int ret = 0;
@@ -1442,21 +1440,7 @@ static int ext4_fc_replay_link_internal(struct super_block *sb,
goto out;
}
- dentry_dir = d_obtain_alias(dir);
- if (IS_ERR(dentry_dir)) {
- ext4_debug("Failed to obtain dentry");
- dentry_dir = NULL;
- goto out;
- }
-
- dentry_inode = d_alloc(dentry_dir, &qstr_dname);
- if (!dentry_inode) {
- ext4_debug("Inode dentry not created.");
- ret = -ENOMEM;
- goto out;
- }
-
- ret = __ext4_link(dir, inode, dentry_inode);
+ ret = __ext4_link(dir, inode, &qstr_dname, NULL);
/*
* It's possible that link already existed since data blocks
* for the dir in question got persisted before we crashed OR
@@ -1470,16 +1454,8 @@ static int ext4_fc_replay_link_internal(struct super_block *sb,
ret = 0;
out:
- if (dentry_dir) {
- d_drop(dentry_dir);
- dput(dentry_dir);
- } else if (dir) {
+ if (dir)
iput(dir);
- }
- if (dentry_inode) {
- d_drop(dentry_inode);
- dput(dentry_inode);
- }
return ret;
}
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 768036a109d7..23942320c515 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3452,7 +3452,8 @@ static int ext4_symlink(struct mnt_idmap *idmap, struct inode *dir,
return err;
}
-int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
+int __ext4_link(struct inode *dir, struct inode *inode,
+ const struct qstr *d_name, struct dentry *dentry)
{
handle_t *handle;
int err, retries = 0;
@@ -3467,10 +3468,8 @@ int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
ext4_handle_sync(handle);
inode_set_ctime_current(inode);
- ext4_inc_count(inode);
- ihold(inode);
- err = ext4_add_entry(handle, dentry, inode);
+ err = __ext4_add_entry(handle, dir, d_name, inode);
if (!err) {
err = ext4_mark_inode_dirty(handle, inode);
/* this can happen only for tmpfile being
@@ -3478,11 +3477,8 @@ int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
*/
if (inode->i_nlink == 1)
ext4_orphan_del(handle, inode);
- d_instantiate(dentry, inode);
- ext4_fc_track_link(handle, dentry);
- } else {
- drop_nlink(inode);
- iput(inode);
+ if (dentry)
+ ext4_fc_track_link(handle, inode, dentry);
}
ext4_journal_stop(handle);
if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries))
@@ -3511,9 +3507,16 @@ static int ext4_link(struct dentry *old_dentry,
err = dquot_initialize(dir);
if (err)
return err;
- return __ext4_link(dir, inode, dentry);
+ ext4_inc_count(inode);
+ err = __ext4_link(dir, inode, &dentry->d_name, dentry);
+ if (!err) {
+ ihold(inode);
+ d_instantiate(dentry, inode);
+ } else {
+ drop_nlink(inode);
+ }
+ return err;
}
-
/*
* Try to find buffer head where contains the parent block.
* It should be the inode block if it is inlined or the 1st block
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH 2/3] ext4: add ext4_fc_eligible()
From: NeilBrown @ 2026-03-17 22:39 UTC (permalink / raw)
To: Theodore Ts'o, Andreas Dilger, Jan Kara; +Cc: linux-ext4, linux-fsdevel
In-Reply-To: <20260317224638.3809014-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
Testing EXT4_MF_FC_INELIGIBLE is almost always combined with testing
ext4_fc_disabled(). The code can be simplified by combining these two
in a new ext4_fc_eligible().
In ext4_fc_track_inode() this moves the ext4_fc_disabled() test after
ext4_fc_mark_ineligible(), but as that is a non-op when
ext4_fc_disabled() is true, this is no no consequence.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/ext4/fast_commit.c | 43 ++++++++++++++-----------------------------
1 file changed, 14 insertions(+), 29 deletions(-)
diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
index f575751f1cae..3ee302b73f45 100644
--- a/fs/ext4/fast_commit.c
+++ b/fs/ext4/fast_commit.c
@@ -224,6 +224,12 @@ static bool ext4_fc_disabled(struct super_block *sb)
(EXT4_SB(sb)->s_mount_state & EXT4_FC_REPLAY));
}
+static bool ext4_fc_eligible(struct super_block *sb)
+{
+ return !ext4_fc_disabled(sb) &&
+ !(ext4_test_mount_flag(sb, EXT4_MF_FC_INELIGIBLE));
+}
+
/*
* Remove inode from fast commit list. If the inode is being committed
* we wait until inode commit is done.
@@ -473,13 +479,8 @@ void ext4_fc_track_unlink(handle_t *handle, struct dentry *dentry)
{
struct inode *inode = d_inode(dentry);
- if (ext4_fc_disabled(inode->i_sb))
- return;
-
- if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
- return;
-
- __ext4_fc_track_unlink(handle, inode, dentry);
+ if (ext4_fc_eligible(inode->i_sb))
+ __ext4_fc_track_unlink(handle, inode, dentry);
}
void __ext4_fc_track_link(handle_t *handle,
@@ -500,13 +501,8 @@ void ext4_fc_track_link(handle_t *handle, struct dentry *dentry)
{
struct inode *inode = d_inode(dentry);
- if (ext4_fc_disabled(inode->i_sb))
- return;
-
- if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
- return;
-
- __ext4_fc_track_link(handle, inode, dentry);
+ if (ext4_fc_eligible(inode->i_sb))
+ __ext4_fc_track_link(handle, inode, dentry);
}
void __ext4_fc_track_create(handle_t *handle, struct inode *inode,
@@ -527,13 +523,8 @@ void ext4_fc_track_create(handle_t *handle, struct dentry *dentry)
{
struct inode *inode = d_inode(dentry);
- if (ext4_fc_disabled(inode->i_sb))
- return;
-
- if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
- return;
-
- __ext4_fc_track_create(handle, inode, dentry);
+ if (ext4_fc_eligible(inode->i_sb))
+ __ext4_fc_track_create(handle, inode, dentry);
}
/* __track_fn for inode tracking */
@@ -557,16 +548,13 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
if (S_ISDIR(inode->i_mode))
return;
- if (ext4_fc_disabled(inode->i_sb))
- return;
-
if (ext4_should_journal_data(inode)) {
ext4_fc_mark_ineligible(inode->i_sb,
EXT4_FC_REASON_INODE_JOURNAL_DATA, handle);
return;
}
- if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
+ if (!ext4_fc_eligible(inode->i_sb))
return;
/*
@@ -644,10 +632,7 @@ void ext4_fc_track_range(handle_t *handle, struct inode *inode, ext4_lblk_t star
if (S_ISDIR(inode->i_mode))
return;
- if (ext4_fc_disabled(inode->i_sb))
- return;
-
- if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
+ if (!ext4_fc_eligible(inode->i_sb))
return;
if (ext4_has_inline_data(inode)) {
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH 1/3] ext4: split __ext4_add_entry() out of ext4_add_entry()
From: NeilBrown @ 2026-03-17 22:39 UTC (permalink / raw)
To: Theodore Ts'o, Andreas Dilger, Jan Kara; +Cc: linux-ext4, linux-fsdevel
In-Reply-To: <20260317224638.3809014-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
__ext4_add_entry() is not given a dentry - just inodes and name.
This will help the next patch which simplifies __ex4_link().
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/ext4/namei.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index c4b5e252af0e..768036a109d7 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2353,10 +2353,10 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
* may not sleep between calling this and putting something into
* the entry, as someone else might have used it while you slept.
*/
-static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
+static int __ext4_add_entry(handle_t *handle, struct inode *dir,
+ const struct qstr *d_name,
struct inode *inode)
{
- struct inode *dir = d_inode(dentry->d_parent);
struct buffer_head *bh = NULL;
struct ext4_dir_entry_2 *de;
struct super_block *sb;
@@ -2373,13 +2373,10 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
sb = dir->i_sb;
blocksize = sb->s_blocksize;
- if (fscrypt_is_nokey_name(dentry))
- return -ENOKEY;
-
- if (!generic_ci_validate_strict_name(dir, &dentry->d_name))
+ if (!generic_ci_validate_strict_name(dir, d_name))
return -EINVAL;
- retval = ext4_fname_setup_filename(dir, &dentry->d_name, 0, &fname);
+ retval = ext4_fname_setup_filename(dir, d_name, 0, &fname);
if (retval)
return retval;
@@ -2460,6 +2457,16 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
return retval;
}
+static int ext4_add_entry(handle_t *handle, struct dentry *dentry,
+ struct inode *inode)
+{
+ struct inode *dir = d_inode(dentry->d_parent);
+
+ if (fscrypt_is_nokey_name(dentry))
+ return -ENOKEY;
+ return __ext4_add_entry(handle, dir, &dentry->d_name, inode);
+}
+
/*
* Returns 0 for success, or a negative error value
*/
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH 0/3] ext4: remove use of d_alloc()
From: NeilBrown @ 2026-03-17 22:39 UTC (permalink / raw)
To: Theodore Ts'o, Andreas Dilger, Jan Kara; +Cc: linux-ext4, linux-fsdevel
This is a revised version of the ext4 patches from my recent patchset
for revising directory locking. My particular interest in changing ext4
is to remove the use of d_alloc(). I will want to deprecate d_alloc()
as it doesn't fit the new model well.
The use of d_alloc() in ext4 is incidental to the actual task at hand.
The code really wants to pass around a parent directory and a name, and
only uses the dentry because that seems convenient. As these patches
show the code actually becomes simpler when we avoid the dentry.
The second patch here isn't needed. I thought it would be, wrote it,
then found it didn't directly help. However I think it is still a nice
simplification so I left it. If you don't want it, please drop it.
Thanks to Jan Kara for his initial review which helpped me see some
problems with my initial attempt more clearly.
NeilBrown
Note: these patches are quite independant of the rest of that big
patchset, except for the late removal of d_alloc(). They can safely
land indepenantly if you would like to take them now in the ext4 tree.
[PATCH 1/3] ext4: split __ext4_add_entry() out of ext4_add_entry()
[PATCH 2/3] ext4: add ext4_fc_eligible()
[PATCH 3/3] ext4: move dcache manipulation out of __ext4_link()
^ permalink raw reply
* Re: [PATCH 32/53f] ext4: move dcache modifying code out of __ext4_link()
From: NeilBrown @ 2026-03-17 20:27 UTC (permalink / raw)
To: Jan Kara
Cc: Alexander Viro, Christian Brauner, Jan Kara, Jeff Layton,
Theodore Ts'o, Andreas Dilger, linux-fsdevel, linux-ext4,
linux-kernel
In-Reply-To: <imo3cvhqtvginngr2ofsotmrsxixutn2jagbkj6322cfgxm3lj@wosdq3xvlym5>
(cc trimmed...)
On Tue, 17 Mar 2026, Jan Kara wrote:
> On Fri 13-03-26 08:12:19, NeilBrown wrote:
> ...
> > diff --git a/fs/dcache.c b/fs/dcache.c
> > index a1219b446b74..c48337d95f9a 100644
> > --- a/fs/dcache.c
> > +++ b/fs/dcache.c
> > @@ -358,7 +358,7 @@ static inline int dname_external(const struct dentry *dentry)
> > return dentry->d_name.name != dentry->d_shortname.string;
> > }
> >
> > -void take_dentry_name_snapshot(struct name_snapshot *name, struct dentry *dentry)
> > +void take_dentry_name_snapshot(struct name_snapshot *name, const struct dentry *dentry)
> > {
> > unsigned seq;
> > const unsigned char *s;
>
> The constification of take_dentry_name_snapshot() should probably be a
> separate patch? Also I'd note that this constification (and the
> constification of __ext4_fc_track_link()) isn't really needed here because
> ext4_fc_track_link() will immediately bail through ext4_fc_disabled() when
> fast commit replay is happening so __ext4_fc_track_link() never gets called
> in that case - more about that below.
I thought I might have overdone the constantification, but I didn't want
to under-do it, at least for my compile tests.
>
> > @@ -1471,7 +1471,15 @@ static int ext4_fc_replay_link_internal(struct super_block *sb,
> > goto out;
> > }
> >
> > + ihold(inode);
> > + inc_nlink(inode);
> > ret = __ext4_link(dir, inode, dentry_inode);
> > + if (ret) {
> > + drop_nlink(inode);
> > + iput(inode);
> > + } else {
> > + d_instantiate(dentry_inode, inode);
> > + }
> > /*
> > * It's possible that link already existed since data blocks
> > * for the dir in question got persisted before we crashed OR
> ...
> > @@ -3460,8 +3460,6 @@ int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
> > ext4_handle_sync(handle);
> >
> > inode_set_ctime_current(inode);
> > - ext4_inc_count(inode);
> > - ihold(inode);
> >
> > err = ext4_add_entry(handle, dentry, inode);
> > if (!err) {
> > @@ -3471,11 +3469,7 @@ int __ext4_link(struct inode *dir, struct inode *inode, struct dentry *dentry)
> > */
> > if (inode->i_nlink == 1)
> > ext4_orphan_del(handle, inode);
> > - d_instantiate(dentry, inode);
> > - ext4_fc_track_link(handle, dentry);
> > - } else {
> > - drop_nlink(inode);
> > - iput(inode);
> > + __ext4_fc_track_link(handle, inode, dentry);
>
> This looks wrong. If fastcommit replay is running, we must skip calling
> __ext4_fc_track_link(). Similarly if the filesystem is currently
> inelligible for fastcommit (due to some complex unsupported operations
> running in parallel). Why did you change ext4_fc_track_link() to
> __ext4_fc_track_link()?
I changed to __ext4_fc_track_link() because I needed something that
accepted the inode separately from the dentry. As you point out, that
means we lose some important code which makes the decision misguided.
I'm wondering about taking a different approach - not using a dentry at
all and not constifying anything.
I could split __ext4_add_entry() out of ext4_add_entry() and instead of
passing the dentry-to-add I could pass the dir inode qstr name, and
d_flags.
Then ext4_link could be passed the same plus a "do fast commit" flag.
The result would be more verbose, but also hopefully more clear.
>
> > @@ -3504,7 +3498,16 @@ static int ext4_link(struct dentry *old_dentry,
> > err = dquot_initialize(dir);
> > if (err)
> > return err;
> > - return __ext4_link(dir, inode, dentry);
> > + ihold(inode);
> > + ext4_inc_count(inode);
>
> I'd put inc_nlink() here instead. We are guaranteed to have a regular file
> anyway and it matches what we do in ext4_fc_replay_link_internal().
> Alternatively we could consistently use ext4_inc_count() &
> ext4_dec_count() in these functions.
Current __ext4_link() has ext4_inc_count().
But it is only usable in namei.c. So I didn't use it in
ext4_fc_replay_link_internal() as that is in a different file.
I'll revise and resend these patches just to the ext4 team.
Thanks,
NeilBrown
>
> > + err = __ext4_link(dir, inode, dentry);
> > + if (err) {
> > + drop_nlink(inode);
> > + iput(inode);
> > + } else {
> > + d_instantiate(dentry, inode);
> > + }
> > + return err;
> > }
>
> Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>
^ permalink raw reply
* Re: [RFC v5 6/7] ext4: fast commit: add lock_updates tracepoint
From: Steven Rostedt @ 2026-03-17 16:21 UTC (permalink / raw)
To: Li Chen
Cc: Zhang Yi, Theodore Ts'o, Andreas Dilger, Masami Hiramatsu,
Mathieu Desnoyers, linux-ext4, linux-kernel, linux-trace-kernel,
Vineeth Remanan Pillai
In-Reply-To: <20260317084624.457185-7-me@linux.beauty>
On Tue, 17 Mar 2026 16:46:21 +0800
Li Chen <me@linux.beauty> wrote:
> Commit-time fast commit snapshots run under jbd2_journal_lock_updates(),
> so it is useful to quantify the time spent with updates locked and to
> understand why snapshotting can fail.
>
> Add a new tracepoint, ext4_fc_lock_updates, reporting the time spent in
> the updates-locked window along with the number of snapshotted inodes
> and ranges. Record the first snapshot failure reason in a stable snap_err
> field for tooling.
>
> Signed-off-by: Li Chen <me@linux.beauty>
> ---
> fs/ext4/ext4.h | 15 ++++++++
> fs/ext4/fast_commit.c | 71 +++++++++++++++++++++++++++++--------
> include/trace/events/ext4.h | 61 +++++++++++++++++++++++++++++++
> 3 files changed, 132 insertions(+), 15 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 68a64fa0be926..b9e146f3dd9e4 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1037,6 +1037,21 @@ enum {
>
> struct ext4_fc_inode_snap;
>
> +/*
> + * Snapshot failure reasons for ext4_fc_lock_updates tracepoint.
> + * Keep these stable for tooling.
> + */
> +enum ext4_fc_snap_err {
> + EXT4_FC_SNAP_ERR_NONE = 0,
> + EXT4_FC_SNAP_ERR_ES_MISS = 1,
> + EXT4_FC_SNAP_ERR_ES_DELAYED = 2,
> + EXT4_FC_SNAP_ERR_ES_OTHER = 3,
> + EXT4_FC_SNAP_ERR_INODES_CAP = 4,
> + EXT4_FC_SNAP_ERR_RANGES_CAP = 5,
> + EXT4_FC_SNAP_ERR_NOMEM = 6,
> + EXT4_FC_SNAP_ERR_INODE_LOC = 7,
You don't need to explicitly state the assignments, the enum will increment
them without them.
> +};
> +
> /*
> * fourth extended file system inode data in memory
> */
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index d1eefee609120..4929e2990b292 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -193,6 +193,12 @@ static struct kmem_cache *ext4_fc_range_cachep;
> #define EXT4_FC_SNAPSHOT_MAX_INODES 1024
> #define EXT4_FC_SNAPSHOT_MAX_RANGES 2048
>
> +static inline void ext4_fc_set_snap_err(int *snap_err, int err)
> +{
> + if (snap_err && *snap_err == EXT4_FC_SNAP_ERR_NONE)
> + *snap_err = err;
> +}
> +
> static void ext4_end_buffer_io_sync(struct buffer_head *bh, int uptodate)
> {
> BUFFER_TRACE(bh, "");
> @@ -983,11 +989,12 @@ static void ext4_fc_free_inode_snap(struct inode *inode)
> static int ext4_fc_snapshot_inode_data(struct inode *inode,
> struct list_head *ranges,
> unsigned int nr_ranges_total,
> - unsigned int *nr_rangesp)
> + unsigned int *nr_rangesp,
> + int *snap_err)
> {
> struct ext4_inode_info *ei = EXT4_I(inode);
> - unsigned int nr_ranges = 0;
> ext4_lblk_t start_lblk, end_lblk, cur_lblk;
> + unsigned int nr_ranges = 0;
>
> spin_lock(&ei->i_fc_lock);
> if (ei->i_fc_lblk_len == 0) {
> @@ -1010,11 +1017,16 @@ static int ext4_fc_snapshot_inode_data(struct inode *inode,
> struct ext4_fc_range *range;
> ext4_lblk_t len;
>
> - if (!ext4_es_lookup_extent(inode, cur_lblk, NULL, &es, NULL))
> + if (!ext4_es_lookup_extent(inode, cur_lblk, NULL, &es, NULL)) {
> + ext4_fc_set_snap_err(snap_err, EXT4_FC_SNAP_ERR_ES_MISS);
> return -EAGAIN;
> + }
>
> - if (ext4_es_is_delayed(&es))
> + if (ext4_es_is_delayed(&es)) {
> + ext4_fc_set_snap_err(snap_err,
> + EXT4_FC_SNAP_ERR_ES_DELAYED);
> return -EAGAIN;
> + }
>
> len = es.es_len - (cur_lblk - es.es_lblk);
> if (len > end_lblk - cur_lblk + 1)
> @@ -1024,12 +1036,17 @@ static int ext4_fc_snapshot_inode_data(struct inode *inode,
> continue;
> }
>
> - if (nr_ranges_total + nr_ranges >= EXT4_FC_SNAPSHOT_MAX_RANGES)
> + if (nr_ranges_total + nr_ranges >= EXT4_FC_SNAPSHOT_MAX_RANGES) {
> + ext4_fc_set_snap_err(snap_err,
> + EXT4_FC_SNAP_ERR_RANGES_CAP);
> return -E2BIG;
> + }
>
> range = kmem_cache_alloc(ext4_fc_range_cachep, GFP_NOFS);
> - if (!range)
> + if (!range) {
> + ext4_fc_set_snap_err(snap_err, EXT4_FC_SNAP_ERR_NOMEM);
> return -ENOMEM;
> + }
> nr_ranges++;
>
> range->lblk = cur_lblk;
> @@ -1054,6 +1071,7 @@ static int ext4_fc_snapshot_inode_data(struct inode *inode,
> range->len = max;
> } else {
> kmem_cache_free(ext4_fc_range_cachep, range);
> + ext4_fc_set_snap_err(snap_err, EXT4_FC_SNAP_ERR_ES_OTHER);
> return -EAGAIN;
> }
>
> @@ -1070,7 +1088,7 @@ static int ext4_fc_snapshot_inode_data(struct inode *inode,
>
> static int ext4_fc_snapshot_inode(struct inode *inode,
> unsigned int nr_ranges_total,
> - unsigned int *nr_rangesp)
> + unsigned int *nr_rangesp, int *snap_err)
> {
> struct ext4_inode_info *ei = EXT4_I(inode);
> struct ext4_fc_inode_snap *snap;
> @@ -1082,8 +1100,10 @@ static int ext4_fc_snapshot_inode(struct inode *inode,
> int alloc_ctx;
>
> ret = ext4_get_inode_loc_noio(inode, &iloc);
> - if (ret)
> + if (ret) {
> + ext4_fc_set_snap_err(snap_err, EXT4_FC_SNAP_ERR_INODE_LOC);
> return ret;
> + }
>
> if (ext4_test_inode_flag(inode, EXT4_INODE_INLINE_DATA))
> inode_len = EXT4_INODE_SIZE(inode->i_sb);
> @@ -1092,6 +1112,7 @@ static int ext4_fc_snapshot_inode(struct inode *inode,
>
> snap = kmalloc(struct_size(snap, inode_buf, inode_len), GFP_NOFS);
> if (!snap) {
> + ext4_fc_set_snap_err(snap_err, EXT4_FC_SNAP_ERR_NOMEM);
> brelse(iloc.bh);
> return -ENOMEM;
> }
> @@ -1102,7 +1123,7 @@ static int ext4_fc_snapshot_inode(struct inode *inode,
> brelse(iloc.bh);
>
> ret = ext4_fc_snapshot_inode_data(inode, &ranges, nr_ranges_total,
> - &nr_ranges);
> + &nr_ranges, snap_err);
> if (ret) {
> kfree(snap);
> ext4_fc_free_ranges(&ranges);
> @@ -1203,7 +1224,10 @@ static int ext4_fc_alloc_snapshot_inodes(struct super_block *sb,
> unsigned int *nr_inodesp);
>
> static int ext4_fc_snapshot_inodes(journal_t *journal, struct inode **inodes,
> - unsigned int inodes_size)
> + unsigned int inodes_size,
> + unsigned int *nr_inodesp,
> + unsigned int *nr_rangesp,
> + int *snap_err)
> {
> struct super_block *sb = journal->j_private;
> struct ext4_sb_info *sbi = EXT4_SB(sb);
> @@ -1221,6 +1245,8 @@ static int ext4_fc_snapshot_inodes(journal_t *journal, struct inode **inodes,
> alloc_ctx = ext4_fc_lock(sb);
> list_for_each_entry(iter, &sbi->s_fc_q[FC_Q_MAIN], i_fc_list) {
> if (i >= inodes_size) {
> + ext4_fc_set_snap_err(snap_err,
> + EXT4_FC_SNAP_ERR_INODES_CAP);
> ret = -E2BIG;
> goto unlock;
> }
> @@ -1244,6 +1270,8 @@ static int ext4_fc_snapshot_inodes(journal_t *journal, struct inode **inodes,
> continue;
>
> if (i >= inodes_size) {
> + ext4_fc_set_snap_err(snap_err,
> + EXT4_FC_SNAP_ERR_INODES_CAP);
> ret = -E2BIG;
> goto unlock;
> }
> @@ -1268,16 +1296,20 @@ static int ext4_fc_snapshot_inodes(journal_t *journal, struct inode **inodes,
> unsigned int inode_ranges = 0;
>
> ret = ext4_fc_snapshot_inode(inodes[idx], nr_ranges,
> - &inode_ranges);
> + &inode_ranges, snap_err);
> if (ret)
> break;
> nr_ranges += inode_ranges;
> }
>
> + if (nr_inodesp)
> + *nr_inodesp = i;
> + if (nr_rangesp)
> + *nr_rangesp = nr_ranges;
> return ret;
> }
>
> -static int ext4_fc_perform_commit(journal_t *journal)
> +static int ext4_fc_perform_commit(journal_t *journal, tid_t commit_tid)
> {
> struct super_block *sb = journal->j_private;
> struct ext4_sb_info *sbi = EXT4_SB(sb);
> @@ -1286,10 +1318,15 @@ static int ext4_fc_perform_commit(journal_t *journal)
> struct inode *inode;
> struct inode **inodes;
> unsigned int inodes_size;
> + unsigned int snap_inodes = 0;
> + unsigned int snap_ranges = 0;
> + int snap_err = EXT4_FC_SNAP_ERR_NONE;
> struct blk_plug plug;
> int ret = 0;
> u32 crc = 0;
> int alloc_ctx;
> + ktime_t lock_start;
> + u64 locked_ns;
>
> /*
> * Step 1: Mark all inodes on s_fc_q[MAIN] with
> @@ -1337,13 +1374,13 @@ static int ext4_fc_perform_commit(journal_t *journal)
> if (ret)
> return ret;
>
> -
> ret = ext4_fc_alloc_snapshot_inodes(sb, &inodes, &inodes_size);
> if (ret)
> return ret;
>
> /* Step 4: Mark all inodes as being committed. */
> jbd2_journal_lock_updates(journal);
> + lock_start = ktime_get();
> /*
> * The journal is now locked. No more handles can start and all the
> * previous handles are now drained. Snapshotting happens in this
> @@ -1357,8 +1394,12 @@ static int ext4_fc_perform_commit(journal_t *journal)
> }
> ext4_fc_unlock(sb, alloc_ctx);
>
> - ret = ext4_fc_snapshot_inodes(journal, inodes, inodes_size);
> + ret = ext4_fc_snapshot_inodes(journal, inodes, inodes_size,
> + &snap_inodes, &snap_ranges, &snap_err);
> jbd2_journal_unlock_updates(journal);
> + locked_ns = ktime_to_ns(ktime_sub(ktime_get(), lock_start));
If locked_ns is only used for the tracepoint, it should either be
calculated in the tracepoint, or add:
if (trace_ext4_fc_lock_updates_enabled()) {
locked_ns = ktime_to_ns(ktime_sub(ktime_get(), lock_start));
> + trace_ext4_fc_lock_updates(sb, commit_tid, locked_ns, snap_inodes,
> + snap_ranges, ret, snap_err);
}
Note, we are going to also add a code to call the tracepoint directly, to
remove the double static_branch.
https://lore.kernel.org/all/20260312150523.2054552-1-vineeth@bitbyteword.org/
But that code is still being worked on so you don't need to worry about it
at the moment.
-- Steve
> kvfree(inodes);
> if (ret)
> return ret;
> @@ -1563,7 +1604,7 @@ int ext4_fc_commit(journal_t *journal, tid_t commit_tid)
> journal_ioprio = EXT4_DEF_JOURNAL_IOPRIO;
> set_task_ioprio(current, journal_ioprio);
> fc_bufs_before = (sbi->s_fc_bytes + bsize - 1) / bsize;
> - ret = ext4_fc_perform_commit(journal);
> + ret = ext4_fc_perform_commit(journal, commit_tid);
> if (ret < 0) {
> if (ret == -EAGAIN || ret == -E2BIG || ret == -ECANCELED)
> status = EXT4_FC_STATUS_INELIGIBLE;
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index fd76d14c2776e..dc084f39b74ad 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -104,6 +104,26 @@ TRACE_DEFINE_ENUM(EXT4_FC_REASON_INODE_JOURNAL_DATA);
> TRACE_DEFINE_ENUM(EXT4_FC_REASON_ENCRYPTED_FILENAME);
> TRACE_DEFINE_ENUM(EXT4_FC_REASON_MAX);
>
> +#undef EM
> +#undef EMe
> +#define EM(a) TRACE_DEFINE_ENUM(EXT4_FC_SNAP_ERR_##a);
> +#define EMe(a) TRACE_DEFINE_ENUM(EXT4_FC_SNAP_ERR_##a);
> +
> +#define TRACE_SNAP_ERR \
> + EM(NONE) \
> + EM(ES_MISS) \
> + EM(ES_DELAYED) \
> + EM(ES_OTHER) \
> + EM(INODES_CAP) \
> + EM(RANGES_CAP) \
> + EM(NOMEM) \
> + EMe(INODE_LOC)
> +
> +TRACE_SNAP_ERR
> +
> +#undef EM
> +#undef EMe
> +
> #define show_fc_reason(reason) \
> __print_symbolic(reason, \
> { EXT4_FC_REASON_XATTR, "XATTR"}, \
> @@ -2812,6 +2832,47 @@ TRACE_EVENT(ext4_fc_commit_stop,
> __entry->num_fc_ineligible, __entry->nblks_agg, __entry->tid)
> );
>
> +#define EM(a) { EXT4_FC_SNAP_ERR_##a, #a },
> +#define EMe(a) { EXT4_FC_SNAP_ERR_##a, #a }
> +
> +TRACE_EVENT(ext4_fc_lock_updates,
> + TP_PROTO(struct super_block *sb, tid_t commit_tid, u64 locked_ns,
> + unsigned int nr_inodes, unsigned int nr_ranges, int err,
> + int snap_err),
> +
> + TP_ARGS(sb, commit_tid, locked_ns, nr_inodes, nr_ranges, err, snap_err),
> +
> + TP_STRUCT__entry(/* entry */
> + __field(dev_t, dev)
> + __field(tid_t, tid)
> + __field(u64, locked_ns)
> + __field(unsigned int, nr_inodes)
> + __field(unsigned int, nr_ranges)
> + __field(int, err)
> + __field(int, snap_err)
> + ),
> +
> + TP_fast_assign(/* assign */
> + __entry->dev = sb->s_dev;
> + __entry->tid = commit_tid;
> + __entry->locked_ns = locked_ns;
> + __entry->nr_inodes = nr_inodes;
> + __entry->nr_ranges = nr_ranges;
> + __entry->err = err;
> + __entry->snap_err = snap_err;
> + ),
> +
> + TP_printk("dev %d,%d tid %u locked_ns %llu nr_inodes %u nr_ranges %u err %d snap_err %s",
> + MAJOR(__entry->dev), MINOR(__entry->dev), __entry->tid,
> + __entry->locked_ns, __entry->nr_inodes, __entry->nr_ranges,
> + __entry->err, __print_symbolic(__entry->snap_err,
> + TRACE_SNAP_ERR))
> +);
> +
> +#undef EM
> +#undef EMe
> +#undef TRACE_SNAP_ERR
> +
> #define FC_REASON_NAME_STAT(reason) \
> show_fc_reason(reason), \
> __entry->fc_ineligible_rc[reason]
^ permalink raw reply
* Re: [PATCHBLIZZARD v7] fuse/libfuse/e2fsprogs: containerize ext4 for safer operation
From: Theodore Tso @ 2026-03-17 15:20 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: Darrick J. Wong, Joanne Koong, linux-fsdevel, bpf, linux-ext4,
Miklos Szeredi, Bernd Schubert, Neal Gompa, Amir Goldstein,
Christian Brauner, Jeff Layton, John
In-Reply-To: <8adf57a0-ee3f-4554-bb0d-cedcf6c51e47@gmail.com>
On Tue, Mar 17, 2026 at 10:05:59AM -0400, Demi Marie Obenour wrote:
>
> This doesn't require a physical attack. An attacker who has previously
> gained root access could use a metadata parsing attack to keep that
> access across reboots.
So the concern that you are raising is that an attacker might be able
to corrupt the metadata in such a way that it triggers some kind of
buffer overrun or other zero-day attack, but that it will *not*
trigger an EXT4-fs error indication (since if it does, then on the
next reboot, an fsck will be forced since the file system will be
marked corrupt). Is that correct?
That's... possible, but in my experience, the vast majority of syzbot
reported bugs involving fuzzed file systems do actually trigger the
file system being detected as corrupted. It's just that the file
system is mounted in errors=continue mode, as opposed to
errors=remount-ro or errirs=panic. And more often than not, it
triggers a warning or an OOPS, which is considered CVE fodder. Not
all syzbot issues can be translated into a privilege escalation
attack.
But if the attacker could pull it off, and reliably find a way to
force the system to trip against the hidden file system corruption, I
agree that this would represent a violation of the security model for
Android, which is that a reboot should always get the system back to a
secure state. I will note, though, that you can also carry out
zero-day attacks by putting malicious data into an image file that
triggers a buffer overrun, and in some ways, that might be an easier
way to persist some kind of security vulnerability across reboots
compared to trying to find a more complex file system metadata parsing
attack.
Ultimately, it's really a cost/benefit tradeoff. We need to take a
big picture view when considering which attacks we should be expending
resources to protect against, since security mitigation budget is not
infinite. That being said, patches are welcome. :-)
Cheers,
- Ted
^ permalink raw reply
* [PATCH] ext4: reject mount if bigalloc with s_first_data_block != 0
From: Helen Koike @ 2026-03-17 14:23 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4, linux-fsdevel, linux-kernel,
kernel-dev, koike, syzbot+b73703b873a33d8eb8f6
bigalloc with s_first_data_block != 0 is not supported, reject mounting
it.
Signed-off-by: Helen Koike <koike@igalia.com>
Suggested-by: Theodore Ts'o <tytso@mit.edu>
Reported-by: syzbot+b73703b873a33d8eb8f6@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b73703b873a33d8eb8f6
---
fs/ext4/super.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 495f99c10085..9f0cc29b718f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3626,6 +3626,13 @@ int ext4_feature_set_ok(struct super_block *sb, int readonly)
"extents feature\n");
return 0;
}
+ if (ext4_has_feature_bigalloc(sb) &&
+ le32_to_cpu(EXT4_SB(sb)->s_es->s_first_data_block)) {
+ ext4_msg(sb, KERN_WARNING,
+ "bad geometry: bigalloc file system with non-zero "
+ "first_data_block\n");
+ return 0;
+ }
#if !IS_ENABLED(CONFIG_QUOTA) || !IS_ENABLED(CONFIG_QFMT_V2)
if (!readonly && (ext4_has_feature_quota(sb) ||
--
2.53.0
^ permalink raw reply related
* Re: [PATCHBLIZZARD v7] fuse/libfuse/e2fsprogs: containerize ext4 for safer operation
From: Demi Marie Obenour @ 2026-03-17 14:05 UTC (permalink / raw)
To: Theodore Tso
Cc: Darrick J. Wong, Joanne Koong, linux-fsdevel, bpf, linux-ext4,
Miklos Szeredi, Bernd Schubert, Neal Gompa, Amir Goldstein,
Christian Brauner, Jeff Layton, John
In-Reply-To: <20260317135921.GB53921@macsyma-wired.lan>
[-- Attachment #1.1.1: Type: text/plain, Size: 2098 bytes --]
On 3/17/26 09:59, Theodore Tso wrote:
> On Mon, Mar 16, 2026 at 08:20:29PM -0400, Demi Marie Obenour wrote:
>>
>> It's worth noting that on ChromeOS and Android, the only trusted
>> disk images are those that are read-only and protected by dm-verity.
>> *Every* writable image is considered untrusted.
>
> So I can't speak for ChromeOS or Android, but given discussions that
> I've had with folks in those teams when we were developing fscrypt and
> fsverity, the writeable images which are soldered onto the mainboard,
> where user data is stored, is protected by fscrypt, which provide
> confidentiality but not integrity for user data.
>
> However, from a trust perspective, if there is an "evil maid attack"
> (where someone leaves their device unattended in a hotel room, and the
> housecleaning staff removes the device, and the flash is removed from
> the mainboard and modified) is something which is considered an attack
> which is realistically only going to be carried out by a nation state,
> and the primary priority was protecting the privileged APK's (the
> moral equivalent of setuid binaries), and that's where fsverity is
> used to protect against that threat.
>
> Yes, the nation state attacker could potentially corrupt the metadata
> of the writeable file system image, and but there are enough zero day
> attacks that involve sending corrupted image files that trigger buffer
> overflows, etc., that while it would be nice to protect against this
> sort of thing, given that it requires a physical attack (and that
> point, the nation state attacker could also enhance the device with
> remotely denotated explosives, something which spies on the
> touchscreen or microphone and ships the output over the network,
> etc.), I don't believe this is considered a high priority threat that
> is worth spending $$$ to mitigate.
This doesn't require a physical attack. An attacker who has previously
gained root access could use a metadata parsing attack to keep that
access across reboots.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCHBLIZZARD v7] fuse/libfuse/e2fsprogs: containerize ext4 for safer operation
From: Theodore Tso @ 2026-03-17 13:59 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: Darrick J. Wong, Joanne Koong, linux-fsdevel, bpf, linux-ext4,
Miklos Szeredi, Bernd Schubert, Neal Gompa, Amir Goldstein,
Christian Brauner, Jeff Layton, John
In-Reply-To: <208bfbd2-d671-462c-925f-4d51b7df1f18@gmail.com>
On Mon, Mar 16, 2026 at 08:20:29PM -0400, Demi Marie Obenour wrote:
>
> It's worth noting that on ChromeOS and Android, the only trusted
> disk images are those that are read-only and protected by dm-verity.
> *Every* writable image is considered untrusted.
So I can't speak for ChromeOS or Android, but given discussions that
I've had with folks in those teams when we were developing fscrypt and
fsverity, the writeable images which are soldered onto the mainboard,
where user data is stored, is protected by fscrypt, which provide
confidentiality but not integrity for user data.
However, from a trust perspective, if there is an "evil maid attack"
(where someone leaves their device unattended in a hotel room, and the
housecleaning staff removes the device, and the flash is removed from
the mainboard and modified) is something which is considered an attack
which is realistically only going to be carried out by a nation state,
and the primary priority was protecting the privileged APK's (the
moral equivalent of setuid binaries), and that's where fsverity is
used to protect against that threat.
Yes, the nation state attacker could potentially corrupt the metadata
of the writeable file system image, and but there are enough zero day
attacks that involve sending corrupted image files that trigger buffer
overflows, etc., that while it would be nice to protect against this
sort of thing, given that it requires a physical attack (and that
point, the nation state attacker could also enhance the device with
remotely denotated explosives, something which spies on the
touchscreen or microphone and ships the output over the network,
etc.), I don't believe this is considered a high priority threat that
is worth spending $$$ to mitigate.
The only other extrenal image, which is where something like using
fuse would be interesting, is a USB thumb drive being connected to the
ChromeOS or Android device. In that case, performance is really not
an issue, so running fsck before mounting would probably be considered
acceptable. The main issue here is that fsck for NTFS, FAT, etc.,
probably isn't as good as the fsck for a file systems such as xfs or
ext4. So the primary protection that we have is that enterprise
managed devices can simply prohibit mounting external USB thumb drives
if the enterprise administrators elects to do so. This doesn't help
personal devices, but users have to take affirmative action to mount a
device (simply inserting a device isn't enough, at least on Android),
so people who are paranoid can simply not pick up a USB thumb drive
they find in a parking lot and insert it into their phone.
Cheers,
- Ted
^ permalink raw reply
* Re: [PATCH] ext4: fix bigalloc cluster arithmetic when s_first_data_block != 0
From: Helen Koike @ 2026-03-17 13:53 UTC (permalink / raw)
To: Theodore Tso
Cc: adilger.kernel, linux-ext4, linux-fsdevel, linux-kernel,
kernel-dev, syzbot+b73703b873a33d8eb8f6
In-Reply-To: <20260314012930.GB38016@macsyma-wired.lan>
Hi Ted,
On 3/13/26 10:29 PM, Theodore Tso wrote:
> On Fri, Mar 13, 2026 at 12:18:30PM -0300, Helen Koike wrote:
>> On filesystems with bigalloc enabled and s_first_data_block != 0,
>
> This is never supposed to happen; fsck already checks for this. From
> e2fsck/super.c:
>
> should_be = (sb->s_log_block_size == 0 &&
> EXT2FS_CLUSTER_RATIO(fs) == 1) ? 1 : 0;
> if (sb->s_first_data_block != should_be) {
> pctx.blk = sb->s_first_data_block;
> pctx.blk2 = should_be;
> fix_problem(ctx, PR_0_FIRST_DATA_BLOCK, &pctx);
> ctx->flags |= E2F_FLAG_ABORT;
> return;
> }
>
> You can also see evidence of this code path getting trigger by
> clicking on "corrupt_fs" in the Syzkaller dashboard.
I see, thanks for the pointers.
>
>> Introduce four macros that subtract s_first_data_block before
>> operating, matching the coordinate system used by mballoc:
>
> This is *way* more complicated than it needs to be. All you need to
> do is just reject the mount if the file system is this insanely
> corrupted. For example:
>
> if (ext4_has_feature_bigalloc(sb) &&
> es->s_first_data_block) {
> ext4_msg(sb, KERN_WARNING, "bad geometry: bigalloc "
> "file system with non-zero first_data_block");
> return -EINVAL;
> }
Agreed. I thought it should be supported from [1] but I missed [2].
[1] https://lore.kernel.org/all/20130221134943.GA3818@gmail.com/
[2] https://lore.kernel.org/all/20130222030327.GB3421@gmail.com/
>
>> Regarding the issue reported by syzbot...
>
> Yeah, this is why I generally don't pay that much attention to syzbot
> reports that mount a corrupted file system. Users are supposed to run
> fsck on a file system before they blindly mount it. If they don't, I
> don't consider it a security vulnerability; it's just a stupid system
> administrator trick, and he or she deserves everything that happens to
> them. Hence, such issues are not a security issue, but just a quality
> of implementation issue. I'll accept patches to addrese sorts of
> things, but it doesn't rate a CVE or high priority processing.
Make sense.
I think I'll submit this patch as per your suggestion since I'm already
looking at this, but I won't bother you further with these sorts of
low-priority issues.
>
> Furthermore, any fix needs to as simple as possible, and avoids adding
> extra overhead especially on hot paths. In this particular case,
> rejecting the mount is the simplest fix, since the file system
> corruption is *easy* to detect.
Ack, and thanks for your reply!
Helen,
Cheers
^ permalink raw reply
* Re: [PATCH] ext4: Fix possible NULL pointer dereference in ext4_group_desc_free()
From: Zqiang @ 2026-03-17 13:23 UTC (permalink / raw)
To: Baokun Li; +Cc: linux-ext4, linux-kernel, tytso, adilger.kernel, libaokun
In-Reply-To: <5e747089-7fc8-4230-b9e2-65e479fcebab@linux.alibaba.com>
>
> On 3/17/26 7:33 AM, Zqiang wrote:
>
> >
> > >
> > > On 3/16/26 4:20 PM, Zqiang wrote:
> > >
> > This can happen if the kvmalloc_objs() fails and sbi->s_group_desc pointer
> > is NULL in the ext4_group_desc_init(), and then the ext4_group_desc_free()
> > is called, leading to a NULL group_desc pointer dereference.
> >
> > This commit therefore adds a NULL check for sbi->s_group_desc before
> > accessing its internal members.
> >
> > Signed-off-by: Zqiang <qiang.zhang@linux.dev>
> > ---
> > fs/ext4/super.c | 8 +++++---
> > 1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 43f680c750ae..c4307dc04687 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -1256,9 +1256,11 @@ static void ext4_group_desc_free(struct ext4_sb_info *sbi)
> >
> > rcu_read_lock();
> > group_desc = rcu_dereference(sbi->s_group_desc);
> > - for (i = 0; i < sbi->s_gdb_count; i++)
> >
> > >
> > > In ext4_group_desc_init(), s_gdb_count is only assigned after kvmalloc_array
> > > allocation succeeds. Therefore, when kvmalloc_array fails, the
> > > brelse(group_desc[i]) in ext4_group_desc_free() will not actually be
> > > executed,
> > > and thus this NULL pointer dereference issue will not be triggered.
> > >
> > Thanks for replay, got it, sorry for make noise.
> >
> > Just then, I find that warning may be trigger:
> >
> > the kvfree() is called in RCU read critical section, if
> > the sbi->s_group_desc pointer comes from vmalloc(),
> > the vfree() is called to release it, but the might_sleep()
> > is called in the vfree(), this may be trigger warnings in
> > rcu_sleep_check() when the enable CONFIG_DEBUG_ATOMIC_SLEEP.
> >
> Indeed, vfree triggers the following warning: ```
> ===========================================================================
> EXT4-fs (vdc): unmounting filesystem
> c478da00-c52c-4dd4-81c1-d4f93e12ab50. BUG: sleeping function called from
> invalid context at mm/vmalloc.c:3441 in_atomic(): 0, irqs_disabled(): 0,
> non_block: 0, pid: 457, name: umount preempt_count: 0, expected: 0 RCU
> nest depth: 1, expected: 0 CPU: 0 UID: 0 PID: 457 Comm: umount Tainted:
> G W 6.19.0-rc4-g4f5e8e6f0123-dirty #10 PREEMPT(none) Tainted: [W]=WARN
> Call Trace: <TASK> dump_stack_lvl+0x55/0x70 __might_resched+0x116/0x160
> vfree+0x38/0x60 ext4_put_super+0x1ac/0x490
> generic_shutdown_super+0x81/0x180 kill_block_super+0x1a/0x40
> ext4_kill_sb+0x22/0x40 deactivate_locked_super+0x35/0xb0
> cleanup_mnt+0x101/0x170 task_work_run+0x5c/0xa0
> exit_to_user_mode_loop+0xe2/0x460 do_syscall_64+0x1de/0x1f0
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
This can also actually happen, depending on your kernel configuration:
CONFIG_PREEMPT_NONE=y
CONFIG_PROVE_RCU=y
CONFIG_DEBUG_ATOMIC_SLEEP=y
CONFIG_PREEMPT_RCU=n
CONFIG_PREEMPT_COUNT=y
CONFIG_DEBUG_PREEMPT=y
./include/linux/rcupdate.h:409 Illegal context switch in RCU read-side critical section!
other info that might help us debug this:
rcu_scheduler_active = 2, debug_locks = 1
2 locks held by umount/555:
#0: ffff88800da680e8 (&type->s_umount_key#29){++++}-{4:4}, at: deactivate_super+0x76/0xa0
#1: ffffffff86a0e580 (rcu_read_lock){....}-{1:3}, at: ext4_group_desc_free+0x27/0x270
Call Trace:
<TASK>
dump_stack_lvl+0xbb/0xd0
dump_stack+0x14/0x20
lockdep_rcu_suspicious+0x15a/0x1b0
__might_resched+0x375/0x4d0
? put_object.part.0+0x2c/0x50
__might_sleep+0x108/0x160
vfree+0x58/0x910
? ext4_group_desc_free+0x27/0x270
kvfree+0x23/0x40
ext4_group_desc_free+0x111/0x270
ext4_put_super+0x3c8/0xd40
generic_shutdown_super+0x14c/0x4a0
? __pfx_shrinker_free+0x10/0x10
kill_block_super+0x40/0x90
ext4_kill_sb+0x6d/0xb0
deactivate_locked_super+0xb4/0x180
deactivate_super+0x7e/0xa0
cleanup_mnt+0x296/0x3e0
__cleanup_mnt+0x16/0x20
task_work_run+0x157/0x250
? __pfx_task_work_run+0x10/0x10
? exit_to_user_mode_loop+0x6a/0x550
exit_to_user_mode_loop+0x102/0x550
do_syscall_64+0x44a/0x500
entry_SYSCALL_64_after_hwframe+0x77/0x7f
> ===========================================================================
> ``` And ext4_mb_init_backend, ext4_mb_release, ext4_flex_groups_free
> have similar issues.Indeed, vfree triggers the following warning:
>
> ===========================================================================
> EXT4-fs (vdc): unmounting filesystem c478da00-c52c-4dd4-81c1-d4f93e12ab50.
> BUG: sleeping function called from invalid context at mm/vmalloc.c:3441
> in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 457, name: umount
> preempt_count: 0, expected: 0
> RCU nest depth: 1, expected: 0
> CPU: 0 UID: 0 PID: 457 Comm: umount Tainted: G W
> 6.19.0-rc4-g4f5e8e6f0123-dirty #10 PREEMPT(none)
> Tainted: [W]=WARN
> Call Trace:
> <TASK>
> dump_stack_lvl+0x55/0x70
> __might_resched+0x116/0x160
> vfree+0x38/0x60
> ext4_put_super+0x1ac/0x490
> generic_shutdown_super+0x81/0x180
> kill_block_super+0x1a/0x40
> ext4_kill_sb+0x22/0x40
> deactivate_locked_super+0x35/0xb0
> cleanup_mnt+0x101/0x170
> task_work_run+0x5c/0xa0
> exit_to_user_mode_loop+0xe2/0x460
> do_syscall_64+0x1de/0x1f0
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
> ===========================================================================
>
> And ext4_mb_init_backend, ext4_mb_release, ext4_flex_groups_free have
> similar issues.
>
> >
> > May be use rcu_access_pointer() to access sbi->s_group_desc
> > is enough.
> >
> Yes, these are all initialization or teardown paths and cannot run
> concurrently with
> online resize, so using rcu_access_pointer() seems sufficient.
Will make a patch and CC you :)
>
> Also, should we add might_sleep() to kvfree() to prevent similar issues?
I think it makes sense.
Thanks
Zqiang
>
> Cheers,
> Baokun
>
^ permalink raw reply
* Re: [PATCH v2 1/1] jbd2: gracefully abort on checkpointing state corruptions
From: Jan Kara @ 2026-03-17 12:28 UTC (permalink / raw)
To: Milos Nikic
Cc: jack, tytso, linux-ext4, linux-kernel, Andreas Dilger, Zhang Yi,
Baokun Li
In-Reply-To: <20260311041548.159424-1-nikic.milos@gmail.com>
On Tue 10-03-26 21:15:48, Milos Nikic wrote:
> This patch targets two internal state machine invariants in checkpoint.c
> residing inside functions that natively return integer error codes.
>
> - In jbd2_cleanup_journal_tail(): A blocknr of 0 indicates a severely
> corrupted journal superblock. Replaced the J_ASSERT with a WARN_ON_ONCE
> and a graceful journal abort, returning -EFSCORRUPTED.
>
> - In jbd2_log_do_checkpoint(): Replaced the J_ASSERT_BH checking for
> an unexpected buffer_jwrite state. If the warning triggers, we
> explicitly drop the just-taken get_bh() reference and call __flush_batch()
> to safely clean up any previously queued buffers in the j_chkpt_bhs array,
> preventing a memory leak before returning -EFSCORRUPTED.
>
> Signed-off-by: Milos Nikic <nikic.milos@gmail.com>
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> Reviewed-by: Zhang Yi <yi.zhang@huawei.com>
> Reviewed-by: Baokun Li <libaokun@linux.alibaba.com>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> Changes in v2:
>
> Replaced the -EUCLEAN error code with -EFSCORRUPTED to better align with ext4/jbd2 semantics for on-disk metadata inconsistencies (per Baokun's review).
>
> Reordered the error path in jbd2_log_do_checkpoint() so that jbd2_journal_abort() is called after __flush_batch(). This ensures cleanly batched buffers are logically flushed before the journal kill switch is flipped.
>
> Collected Reviewed-by tags from Andreas Dilger, Zhang Yi, and Baokun Li.
>
> Changes in v1:
>
> Initial implementation converting J_ASSERTs in jbd2_cleanup_journal_tail() and jbd2_log_do_checkpoint() to WARN_ON_ONCE and graceful journal aborts.
>
> fs/jbd2/checkpoint.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/fs/jbd2/checkpoint.c b/fs/jbd2/checkpoint.c
> index de89c5bef607..1508e2f54462 100644
> --- a/fs/jbd2/checkpoint.c
> +++ b/fs/jbd2/checkpoint.c
> @@ -267,7 +267,15 @@ int jbd2_log_do_checkpoint(journal_t *journal)
> */
> BUFFER_TRACE(bh, "queue");
> get_bh(bh);
> - J_ASSERT_BH(bh, !buffer_jwrite(bh));
> + if (WARN_ON_ONCE(buffer_jwrite(bh))) {
> + put_bh(bh); /* drop the ref we just took */
> + spin_unlock(&journal->j_list_lock);
> + /* Clean up any previously batched buffers */
> + if (batch_count)
> + __flush_batch(journal, &batch_count);
> + jbd2_journal_abort(journal, -EFSCORRUPTED);
> + return -EFSCORRUPTED;
> + }
> journal->j_chkpt_bhs[batch_count++] = bh;
> transaction->t_chp_stats.cs_written++;
> transaction->t_checkpoint_list = jh->b_cpnext;
> @@ -325,7 +333,10 @@ int jbd2_cleanup_journal_tail(journal_t *journal)
>
> if (!jbd2_journal_get_log_tail(journal, &first_tid, &blocknr))
> return 1;
> - J_ASSERT(blocknr != 0);
> + if (WARN_ON_ONCE(blocknr == 0)) {
> + jbd2_journal_abort(journal, -EFSCORRUPTED);
> + return -EFSCORRUPTED;
> + }
>
> /*
> * We need to make sure that any blocks that were recently written out
> --
> 2.53.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox