* [PATCH] ext4: fix bigalloc cluster arithmetic when s_first_data_block != 0
From: Helen Koike @ 2026-03-13 15:18 UTC (permalink / raw)
To: tytso, adilger.kernel, linux-ext4, linux-fsdevel, linux-kernel,
kernel-dev, koike, syzbot+b73703b873a33d8eb8f6
On filesystems with bigalloc enabled and s_first_data_block != 0,
mballoc defines clusters starting from s_first_data_block, so cluster
N covers blocks [s_first_data_block + N*s_cluster_ratio, ...).
EXT4_B2C/EXT4_C2B perform a plain bit-shift on the absolute block
number, ignoring s_first_data_block.
For instance, with cluster_ratio = 4 and s_first_data_block = 1:
This is how EXT4_B2C/EXT4_C2B view it:
block: 0 1 2 3 4 5 6 7 8 9 10 11 12 ...
|___________| |___________| |___________|
cluster: 0 1 2
This is how mballoc views it:
block: 0 1 2 3 4 5 6 7 8 9 10 11 12 ...
|___________| |___________| |___________|
cluster: 0 1 2
This causes the following issues:
1) In extents.c, partial->pclu is stored and compared using EXT4_B2C,
then passed to ext4_free_blocks() via EXT4_C2B. The resulting
block address is misaligned with mballoc's cluster boundaries,
causing the wrong cluster to be freed.
2) In ext4_free_blocks(), EXT4_PBLK_COFF uses the same plain
bit-mask, so the intra-cluster offset of the start block is
computed incorrectly, misaligning the freed range.
Introduce four macros that subtract s_first_data_block before
operating, matching the coordinate system used by mballoc:
EXT4_MB_B2C(sbi, blk) block -> cluster number
EXT4_MB_C2B(sbi, cluster) cluster -> first block of cluster
EXT4_MB_PBLK_COFF(sbi, blk) intra-cluster offset of a block
EXT4_MB_LBLK_COFF(sbi, n) intra-cluster offset of a block count
Use EXT4_MB_B2C/EXT4_MB_C2B for all partial->pclu operations in
extents.c, and EXT4_MB_PBLK_COFF/EXT4_MB_LBLK_COFF in the alignment
prologue of ext4_free_blocks().
Regarding the issue reported by syzbot:
Context: s_first_data_block=1, cluster size 16 and block 145 contains
a extents leaf for inode 15.
When an extent is removed (block 145 went from 7 to 6 valid extent
entries), ext4_ext_rm_leaf() sees the cluster partial->pclu (which is
10) to be freed.
Note that EXT4_C2B(partial->pclu=10) is 160, and EXT4_B2C(145) is 9
(so the extents leaf is in a different cluster). Finally
ext4_free_blocks(..,block=160, count=16..) is called, which shouldn't
be a problem (it should not free block 145).
Except that in ext4_free_blocks() (and later in ext4_mb_clear_bb()
and mb_free_blocks()) , block 160 resolves to group 0 bit 9 count 1
(which frees block 145 containing extents leaf!!!!), causing block
145 to be reused by other inodes while inode 15 still thinks that
block 145 contains extents metadata, resulting later in the UAF we see
by syzbot.
The issue doesn't reproduce with the current patch.
Test results:
> kvm-xfstests --kernel $BUILD_DIR/arch/x86/boot/bzImage smoke
ext4/4k: 7 tests, 1 skipped, 1113 seconds
generic/475 Pass 185s
generic/476 Pass 184s
generic/521 Pass 182s
generic/522 Pass 181s
generic/642 Pass 185s
generic/750 Pass 185s
generic/751 Skipped 1s
Totals: 7 tests, 1 skipped, 0 failures, 0 errors, 1103s
> kvm-xfstests --kernel $BUILD_DIR/arch/x86/boot/bzImage -c 1k smoke
ext4/1k: 5 tests, 1 skipped, 755 seconds
generic/521 Pass 183s
generic/522 Pass 182s
generic/642 Pass 186s
generic/750 Pass 186s
generic/751 Skipped 2s
Totals: 5 tests, 1 skipped, 0 failures, 0 errors, 739s
Signed-off-by: Helen Koike <koike@igalia.com>
Reported-by: syzbot+b73703b873a33d8eb8f6@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b73703b873a33d8eb8f6
---
Hi all,
I'm sorry if the commit message is too verbose. I'm not a file system
developer, so I tried to be didactic for those like me.
I was also wondering if I should update the existing macros instead of
creating new ones, but since I'm not very familiar with this subsystem
and current discussions I decided to be safe and change only for the use
case I could test.
If you think this line of solution make sense I could update the
existing macros, please just let me know.
Thanks
Helen
---
fs/ext4/ext4.h | 21 +++++++++++++++++++++
fs/ext4/extents.c | 20 ++++++++++----------
fs/ext4/mballoc.c | 4 ++--
3 files changed, 33 insertions(+), 12 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 7e8f66ba17f4..83fd6e53c003 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -315,6 +315,17 @@ struct ext4_io_submit {
#define EXT4_B2C(sbi, blk) ((blk) >> (sbi)->s_cluster_bits)
/* Translate a cluster number to a block number */
#define EXT4_C2B(sbi, cluster) ((cluster) << (sbi)->s_cluster_bits)
+/*
+ * Translate a physical block number to a cluster number using mballoc's
+ * cluster definition, which accounts for s_first_data_block. Use these
+ * when tracking partial->pclu so that the cluster numbers are consistent
+ * with what ext4_get_group_no_and_offset() (and thus ext4_free_blocks())
+ * expects.
+ */
+#define EXT4_MB_B2C(sbi, nr) \
+ (((nr) - le32_to_cpu((sbi)->s_es->s_first_data_block)) >> (sbi)->s_cluster_bits)
+#define EXT4_MB_C2B(sbi, cluster) \
+ (((cluster) << (sbi)->s_cluster_bits) + le32_to_cpu((sbi)->s_es->s_first_data_block))
/* Translate # of blks to # of clusters */
#define EXT4_NUM_B2C(sbi, blks) (((blks) + (sbi)->s_cluster_ratio - 1) >> \
(sbi)->s_cluster_bits)
@@ -331,6 +342,16 @@ struct ext4_io_submit {
((ext4_fsblk_t) (s)->s_cluster_ratio - 1))
#define EXT4_LBLK_COFF(s, lblk) ((lblk) & \
((ext4_lblk_t) (s)->s_cluster_ratio - 1))
+/*
+ * Cluster-offset macros that account for s_first_data_block, consistent
+ * with mballoc's cluster numbering. Use EXT4_MB_PBLK_COFF when aligning a
+ * physical block number and EXT4_MB_LBLK_COFF when aligning a block count.
+ */
+#define EXT4_MB_PBLK_COFF(sbi, pblk) \
+ (((pblk) - le32_to_cpu((sbi)->s_es->s_first_data_block)) & \
+ ((ext4_fsblk_t)(sbi)->s_cluster_ratio - 1))
+#define EXT4_MB_LBLK_COFF(sbi, lblk) \
+ ((lblk) & ((ext4_lblk_t)(sbi)->s_cluster_ratio - 1))
/*
* Structure of a blocks group descriptor
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 35703dce23a3..38d49f784c22 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2493,13 +2493,13 @@ static int ext4_remove_blocks(handle_t *handle, struct inode *inode,
last_pblk = ext4_ext_pblock(ex) + ee_len - 1;
if (partial->state != initial &&
- partial->pclu != EXT4_B2C(sbi, last_pblk)) {
+ partial->pclu != EXT4_MB_B2C(sbi, last_pblk)) {
if (partial->state == tofree) {
flags = get_default_free_blocks_flags(inode);
if (ext4_is_pending(inode, partial->lblk))
flags |= EXT4_FREE_BLOCKS_RERESERVE_CLUSTER;
ext4_free_blocks(handle, inode, NULL,
- EXT4_C2B(sbi, partial->pclu),
+ EXT4_MB_C2B(sbi, partial->pclu),
sbi->s_cluster_ratio, flags);
if (flags & EXT4_FREE_BLOCKS_RERESERVE_CLUSTER)
ext4_rereserve_cluster(inode, partial->lblk);
@@ -2545,7 +2545,7 @@ static int ext4_remove_blocks(handle_t *handle, struct inode *inode,
ext4_free_blocks(handle, inode, NULL, pblk, num, flags);
/* reset the partial cluster if we've freed past it */
- if (partial->state != initial && partial->pclu != EXT4_B2C(sbi, pblk))
+ if (partial->state != initial && partial->pclu != EXT4_MB_B2C(sbi, pblk))
partial->state = initial;
/*
@@ -2560,7 +2560,7 @@ static int ext4_remove_blocks(handle_t *handle, struct inode *inode,
*/
if (EXT4_LBLK_COFF(sbi, from) && num == ee_len) {
if (partial->state == initial) {
- partial->pclu = EXT4_B2C(sbi, pblk);
+ partial->pclu = EXT4_MB_B2C(sbi, pblk);
partial->lblk = from;
partial->state = tofree;
}
@@ -2651,7 +2651,7 @@ ext4_ext_rm_leaf(handle_t *handle, struct inode *inode,
*/
if (sbi->s_cluster_ratio > 1) {
pblk = ext4_ext_pblock(ex);
- partial->pclu = EXT4_B2C(sbi, pblk);
+ partial->pclu = EXT4_MB_B2C(sbi, pblk);
partial->state = nofree;
}
ex--;
@@ -2766,13 +2766,13 @@ ext4_ext_rm_leaf(handle_t *handle, struct inode *inode,
*/
if (partial->state == tofree && ex >= EXT_FIRST_EXTENT(eh)) {
pblk = ext4_ext_pblock(ex) + ex_ee_len - 1;
- if (partial->pclu != EXT4_B2C(sbi, pblk)) {
+ if (partial->pclu != EXT4_MB_B2C(sbi, pblk)) {
int flags = get_default_free_blocks_flags(inode);
if (ext4_is_pending(inode, partial->lblk))
flags |= EXT4_FREE_BLOCKS_RERESERVE_CLUSTER;
ext4_free_blocks(handle, inode, NULL,
- EXT4_C2B(sbi, partial->pclu),
+ EXT4_MB_C2B(sbi, partial->pclu),
sbi->s_cluster_ratio, flags);
if (flags & EXT4_FREE_BLOCKS_RERESERVE_CLUSTER)
ext4_rereserve_cluster(inode, partial->lblk);
@@ -2886,7 +2886,7 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
*/
if (sbi->s_cluster_ratio > 1) {
pblk = ext4_ext_pblock(ex) + end - ee_block + 1;
- partial.pclu = EXT4_B2C(sbi, pblk);
+ partial.pclu = EXT4_MB_B2C(sbi, pblk);
partial.state = nofree;
}
@@ -2919,7 +2919,7 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
if (err < 0)
goto out;
if (pblk) {
- partial.pclu = EXT4_B2C(sbi, pblk);
+ partial.pclu = EXT4_MB_B2C(sbi, pblk);
partial.state = nofree;
}
}
@@ -3041,7 +3041,7 @@ int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start,
if (ext4_is_pending(inode, partial.lblk))
flags |= EXT4_FREE_BLOCKS_RERESERVE_CLUSTER;
ext4_free_blocks(handle, inode, NULL,
- EXT4_C2B(sbi, partial.pclu),
+ EXT4_MB_C2B(sbi, partial.pclu),
sbi->s_cluster_ratio, flags);
if (flags & EXT4_FREE_BLOCKS_RERESERVE_CLUSTER)
ext4_rereserve_cluster(inode, partial.lblk);
diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 9c7881a4ea75..5c7ad20e4e7a 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -6360,7 +6360,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
* blocks at the beginning or the end unless we are explicitly
* requested to avoid doing so.
*/
- overflow = EXT4_PBLK_COFF(sbi, block);
+ overflow = EXT4_MB_PBLK_COFF(sbi, block);
if (overflow) {
if (flags & EXT4_FREE_BLOCKS_NOFREE_FIRST_CLUSTER) {
overflow = sbi->s_cluster_ratio - overflow;
@@ -6376,7 +6376,7 @@ void ext4_free_blocks(handle_t *handle, struct inode *inode,
/* The range changed so it's no longer validated */
flags &= ~EXT4_FREE_BLOCKS_VALIDATED;
}
- overflow = EXT4_LBLK_COFF(sbi, count);
+ overflow = EXT4_MB_LBLK_COFF(sbi, count);
if (overflow) {
if (flags & EXT4_FREE_BLOCKS_NOFREE_LAST_CLUSTER) {
if (count > overflow)
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v4 19/25] xfs: remove unwritten extents after preallocations in fsverity metadata
From: Darrick J. Wong @ 2026-03-13 14:55 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Andrey Albershteyn, linux-xfs, fsverity, linux-fsdevel, ebiggers,
hch, linux-ext4, linux-f2fs-devel, linux-btrfs
In-Reply-To: <dz3va7indkh4to2pingopyndck737qvyxvlfomkqqothzj4pdk@wu26ba5stnf6>
On Fri, Mar 13, 2026 at 12:17:15PM +0100, Andrey Albershteyn wrote:
> On 2026-03-12 07:52:50, Darrick J. Wong wrote:
> > On Thu, Mar 12, 2026 at 02:50:57PM +0100, Andrey Albershteyn wrote:
> > > On 2026-03-09 18:29:14, Darrick J. Wong wrote:
> > > > > XFS preallocates spaces during writes. In normal I/O this space, if
> > > > > unused, is removed by truncate. For files with fsverity, XFS does not
> > > > > use truncate as fsverity metadata is stored past EOF.
> > > > >
> > > > > After we're done with writing fsverity metadata iterate over extents in
> > > > > that region and remove any unwritten ones. These would be preallocation
> > > > > leftovers in the merkle tree holes and past fsverity descriptor.
> > > > >
> > > > > Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> > > > > ---
> > >
> > > > There's an upper limit on the number of blocks you can unmap/free in a
> > > > single transaction. Maybe move the xfs_trans_{alloc,commit} into the
> > > > loop body?
> > > >
> > > > Oh wait, you skip the written extents. Ok, so maybe just roll it after
> > > > you've done a bunmapi.
> > >
> > > I see, I will add a rolling transaction. Btw, why skipping the
> > > written extents let's us use rolling here?
> >
> > Hrmm. At first I thought: why can't xfs_fsverity_cancel_unwritten
> > allocate (and commit) the transaction inside the loop body? Then I
> > thought "well, it's only conditionally unmapping things, and it's sort
> > of a pain to allocate a transaction only then to find out if you
> > actually want to run one".
> >
> > OTOH there could be billions of extents in the data fork, so holding the
> > ILOCK for that many transactions isn't a good thing because tr_write
> > only preallocates space for a certain number of transaction rolls.
> > fsverity already holds IOLOCK_EXCL so there can't be any other programs
> > using the file.
> >
> > So now I arrive back at "The transaction allocation and commit/cancel
> > should be inside the loop body, since crashing midway through wouldn't
> > result in user-visible changes to the file data".
> >
> > > > Do you need to purge the cow fork too?
> > >
> > > hmm, what case are you thinking about here? The fsverity is written
> > > in past EOF region, I don't see how COW extent could be left there.
> > > Can they somehow be left mapped for blocks past i_size?
> >
> > I was talking about speculative cow fork preallocations below i_size.
> > They'll eventually get purged by blockgc, but you could give the space
> > back once you've committed enabling the fsverity file flag.
>
> hmm, if they're purged by blockgc, then why to do it here? I'm
Since you've made the file read-only by turning on fsverity, I think
it's appropriate to return the COW fork preallocations to the free space
pool. Granted, you're correct that blockgc will eventually call
xfs_reflink_cancel_cow_range for you either due to timeout or because
something hit ENOSPC and kicked blockgc. So I guess its not a
requirement to land this series; I was just surprised not to see
xfs_reflink_cancel_cow_range done explicitly here.
> iterating and removing unwritten extents here only because
> xfs_inode_free_eofblocks() will skip fsverity inodes, the check in
> xfs_can_free_eofblocks():
>
> if (IS_VERITY(VFS_I(ip)))
> return false;
>
> The skipping is done to not remove all the fsverity metadata
> extents. With preallocations enabled there could be unwritten
> extents left in the metadata.
<nod> Cleaning out the unwritten extents from the data fork when
enabling fsverity makes sense to me and is totally fine.
> Am I missing something?
Nah, but we might be talking past each other a bit at this point. :/
--D
^ permalink raw reply
* Re: [RFC PATCH] ext4: handle wraparound when searching for blocks for indirect mapped blocks
From: Theodore Tso @ 2026-03-13 14:31 UTC (permalink / raw)
To: Baokun Li; +Cc: Jan Kara, Ext4 Developers List
In-Reply-To: <2a047aeb-6db8-4be9-8908-b12632d5e632@linux.alibaba.com>
On Fri, Mar 13, 2026 at 10:00:04AM +0800, Baokun Li wrote:
> IIUC, ngroups/end here only depend on filesystem size and whether the inode
> is extent-based, and both should stay unchanged during block allocation.
> So doing the check once at the beginning should be sufficient. Am I missing
> anything?
The problem here is that case where we use
ext4_get_allocation_groups_count as ngroups and end are different. In
some places we do this:
ext4_group_t ngroups = ext4_get_allocation_groups_count(ac);
and in others we do this:
end = ext4_get_allocation_groups_count(ac);
Now, if start is zero, then these two are equivalent. But if start is
not zero, but say, is 2**32 - 8, then where we use
ext4_allocaiton_groups_count() as the last block group to search, then
we only will search exactly 8 block groups, and if
there are free blocks in the first 8 block groups, then the scanning
function will fail.
Alternatively, if we just do the check at the beginning, then 2**32 -
8 is a valid starting point, but if we just search forward by ngroups,
then we may end up returning a block group which won't work for
indirect mapped inodes.
Hence, in *every* function where we call
ext4_get_allocaiton_groups_count(), if the goal is to search all block
groups that are valid for indirect mapped inodes, and start might be
greater than 0, we *have* to handle wraparound.
Does that make sense?
- Ted
^ permalink raw reply
* Re: [PATCH -next 5/8] ext4: fix miss free super_block in extents_kunit_exit()
From: Ojaswin Mujoo @ 2026-03-13 14:03 UTC (permalink / raw)
To: Ritesh Harjani; +Cc: Ye Bin, tytso, adilger.kernel, linux-ext4, jack
In-Reply-To: <tsukszr0.ritesh.list@gmail.com>
On Fri, Mar 13, 2026 at 05:33:15PM +0530, Ritesh Harjani wrote:
> Ye Bin <yebin@huaweicloud.com> writes:
>
> > From: Ye Bin <yebin10@huawei.com>
> >
> > There's issue as follows:
> > ODEBUG: free active (active state 0) object: ffff88812f2d68a0 object type: percpu_counter hint: 0x0
> > <TASK>
> > debug_check_no_obj_freed+0x3d9/0x4d0
> > kfree+0x2bb/0x6c0
> > extents_kunit_exit+0x65/0x90 [ext4_test]
> > kunit_try_run_case_cleanup+0xbc/0x100 [kunit]
> > kunit_generic_run_threadfn_adapter+0x89/0x100 [kunit]
> > kthread+0x408/0x540
> > ret_from_fork+0xa76/0xdf0
> > ret_from_fork_asm+0x1a/0x30
> >
> > The above issue was caused because the super_block cleanup process
> > was not properly performed.
> > Therefore, deactivate_super() is called in extents_kunit_exit() to
> > cleanup the super_block.
> >
> > Fixes: cb1e0c1d1fad ("ext4: kunit tests for extent splitting and conversion")
> > Signed-off-by: Ye Bin <yebin10@huawei.com>
> > ---
> > fs/ext4/extents-test.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/ext4/extents-test.c b/fs/ext4/extents-test.c
> > index 5a5016ea1ecc..70d84c0a18e2 100644
> > --- a/fs/ext4/extents-test.c
> > +++ b/fs/ext4/extents-test.c
> > @@ -144,7 +144,7 @@ static void extents_kunit_exit(struct kunit *test)
> > {
> > struct ext4_sb_info *sbi = k_ctx.k_ei->vfs_inode.i_sb->s_fs_info;
> >
> > - kfree(sbi);
> > + deactivate_super(sbi->s_sb);
>
> This doesn't look correct. We don't have sbi->s_sb->s_op->put_super() registered, so
> kfree(sbi) never happens. Maybe it will be good to check with kmemleak
> too, that this could cause a memory leak.
>
> I think we can do:
>
> ext4_es_unregister_shrinker(sbi); // already taken care in [1]
> deactivate_super(sbi->s_sb)
> kfree(sbi);
I checked in my setup and can see that we don't have sb->s_op
registered.
@@ -149,6 +150,8 @@ static void extents_kunit_exit(struct kunit *test)
sbi = k_ctx.k_ei->vfs_inode.i_sb->s_fs_info;
deactivate_super(sbi->s_sb);
+ kunit_log(KERN_ALERT, test,
+ "sbi: %px sb->s_op: %pS", sbi, sbi->s_sb->s_op);
kfree(k_ctx.k_ei);
kfree(k_ctx.k_data);
}
Output:
[ 12.176807] sbi: ffff8881089cc000 sb->s_op: default_op.3+0x0/0x100
So we are using the default noop s_ops.
Regards,
ojaswin
>
> [1]: https://lore.kernel.org/linux-ext4/5bb9041471dab8ce870c191c19cbe4df57473be8.1772381213.git.ritesh.list@gmail.com/
>
> -ritesh
^ permalink raw reply
* Re: [PATCH -next 8/8] ext4: fix possible null-ptr-deref in mbt_kunit_exit()
From: Ritesh Harjani @ 2026-03-13 12:33 UTC (permalink / raw)
To: Ye Bin, tytso, adilger.kernel, linux-ext4; +Cc: jack
In-Reply-To: <20260310130412.3156753-9-yebin@huaweicloud.com>
Ye Bin <yebin@huaweicloud.com> writes:
> From: Ye Bin <yebin10@huawei.com>
>
> There's issue as follows:
> # test_new_blocks_simple: failed to initialize: -12
> KASAN: null-ptr-deref in range [0x0000000000000638-0x000000000000063f]
> Tainted: [E]=UNSIGNED_MODULE, [N]=TEST
> RIP: 0010:mbt_kunit_exit+0x5e/0x3e0 [ext4_test]
> Call Trace:
> <TASK>
> kunit_try_run_case_cleanup+0xbc/0x100 [kunit]
> kunit_generic_run_threadfn_adapter+0x89/0x100 [kunit]
> kthread+0x408/0x540
> ret_from_fork+0xa76/0xdf0
> ret_from_fork_asm+0x1a/0x30
>
> If mbt_kunit_init() init testcase failed will lead to null-ptr-deref.
> So add test if 'sb' is inited success in mbt_kunit_exit().
>
> Fixes: 7c9fa399a369 ("ext4: add first unit test for ext4_mb_new_blocks_simple in mballoc")
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> fs/ext4/mballoc-test.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/mballoc-test.c b/fs/ext4/mballoc-test.c
> index c75b91ae0cf0..90ed505fa4b1 100644
> --- a/fs/ext4/mballoc-test.c
> +++ b/fs/ext4/mballoc-test.c
> @@ -362,7 +362,6 @@ static int mbt_kunit_init(struct kunit *test)
> return ret;
> }
>
> - test->priv = sb;
> kunit_activate_static_stub(test,
> ext4_read_block_bitmap_nowait,
> ext4_read_block_bitmap_nowait_stub);
> @@ -383,6 +382,8 @@ static int mbt_kunit_init(struct kunit *test)
> return -ENOMEM;
> }
>
> + test->priv = sb;
> +
So, I see that "test" which is of "struct kunit" type, is always
initialized locally on stack, so test->priv by default is always NULL.
Hence this make sense to me.
Feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> return 0;
> }
>
> @@ -390,6 +391,9 @@ static void mbt_kunit_exit(struct kunit *test)
> {
> struct super_block *sb = (struct super_block *)test->priv;
>
> + if (!sb)
> + return;
> +
> mbt_mb_release(sb);
> mbt_ctx_release(sb);
> mbt_ext4_free_super_block(sb);
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH -next 7/8] ext4: fix possible null-ptr-deref in extents_kunit_exit()
From: Ritesh Harjani @ 2026-03-13 12:27 UTC (permalink / raw)
To: Ye Bin, tytso, adilger.kernel, linux-ext4; +Cc: jack
In-Reply-To: <20260310130412.3156753-8-yebin@huaweicloud.com>
Ye Bin <yebin@huaweicloud.com> writes:
> From: Ye Bin <yebin10@huawei.com>
>
> There's issue as follows:
> KASAN: null-ptr-deref in range [0x00000000000002c0-0x00000000000002c7]
> Tainted: [E]=UNSIGNED_MODULE, [N]=TEST
> RIP: 0010:extents_kunit_exit+0x2e/0xc0 [ext4_test]
> Call Trace:
> <TASK>
> kunit_try_run_case_cleanup+0xbc/0x100 [kunit]
> kunit_generic_run_threadfn_adapter+0x89/0x100 [kunit]
> kthread+0x408/0x540
> ret_from_fork+0xa76/0xdf0
> ret_from_fork_asm+0x1a/0x30
>
> Above issue happens as extents_kunit_init() init testcase failed.
> So test if testcase is inited success.
>
> Fixes: cb1e0c1d1fad ("ext4: kunit tests for extent splitting and conversion")
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> fs/ext4/extents-test.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/extents-test.c b/fs/ext4/extents-test.c
> index 7e2796f72d45..b1ea37e74f12 100644
> --- a/fs/ext4/extents-test.c
> +++ b/fs/ext4/extents-test.c
> @@ -142,8 +142,12 @@ static struct file_system_type ext_fs_type = {
>
> static void extents_kunit_exit(struct kunit *test)
> {
> - struct ext4_sb_info *sbi = k_ctx.k_ei->vfs_inode.i_sb->s_fs_info;
> + struct ext4_sb_info *sbi;
>
> + if (!k_ctx.k_ei)
> + return;
> +
Make sense to me. Feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
^ permalink raw reply
* Re: [PATCH -next 6/8] ext4: fix the error handling process in extents_kunit_init).
From: Ritesh Harjani @ 2026-03-13 12:15 UTC (permalink / raw)
To: Ye Bin, tytso, adilger.kernel, linux-ext4; +Cc: jack
In-Reply-To: <20260310130412.3156753-7-yebin@huaweicloud.com>
Ye Bin <yebin@huaweicloud.com> writes:
> From: Ye Bin <yebin10@huawei.com>
>
> The error processing in extents_kunit_init() is improper, causing
> resource leakage.
> Reconstruct the error handling process to prevent potential resource
> leaks
>
> Fixes: cb1e0c1d1fad ("ext4: kunit tests for extent splitting and conversion")
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> fs/ext4/extents-test.c | 41 +++++++++++++++++++++++++++++------------
> 1 file changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/extents-test.c b/fs/ext4/extents-test.c
> index 70d84c0a18e2..7e2796f72d45 100644
> --- a/fs/ext4/extents-test.c
> +++ b/fs/ext4/extents-test.c
> @@ -222,33 +222,37 @@ static int extents_kunit_init(struct kunit *test)
> (struct kunit_ext_test_param *)(test->param_value);
> int err;
>
> - sb = sget(&ext_fs_type, NULL, ext_set, 0, NULL);
> - if (IS_ERR(sb))
> - return PTR_ERR(sb);
> -
> - sb->s_blocksize = 4096;
> - sb->s_blocksize_bits = 12;
> -
> sbi = kzalloc_obj(struct ext4_sb_info);
> if (sbi == NULL)
> return -ENOMEM;
>
> + sb = sget(&ext_fs_type, NULL, ext_set, 0, NULL);
> + if (IS_ERR(sb)) {
> + kfree(sbi);
> + return PTR_ERR(sb);
> + }
> +
> sbi->s_sb = sb;
> sb->s_fs_info = sbi;
>
> + sb->s_blocksize = 4096;
> + sb->s_blocksize_bits = 12;
> +
> if (!param || !param->disable_zeroout)
> sbi->s_extent_max_zeroout_kb = 32;
>
> /* setup the mock inode */
> k_ctx.k_ei = kzalloc_obj(struct ext4_inode_info);
> - if (k_ctx.k_ei == NULL)
> - return -ENOMEM;
> + if (k_ctx.k_ei == NULL) {
> + err = -ENOMEM;
> + goto out_deactivate;
> + }
> ei = k_ctx.k_ei;
> inode = &ei->vfs_inode;
>
> err = ext4_es_register_shrinker(sbi);
> if (err)
> - return err;
> + goto out_deactivate;
>
> ext4_es_init_tree(&ei->i_es_tree);
> rwlock_init(&ei->i_es_lock);
> @@ -264,8 +268,10 @@ static int extents_kunit_init(struct kunit *test)
> inode->i_sb = sb;
>
> k_ctx.k_data = kzalloc(EXT_DATA_LEN * 4096, GFP_KERNEL);
> - if (k_ctx.k_data == NULL)
> - return -ENOMEM;
> + if (k_ctx.k_data == NULL) {
> + err = -ENOMEM;
> + goto out_deactivate;
> + }
>
> /*
> * set the data area to a junk value
> @@ -310,6 +316,17 @@ static int extents_kunit_init(struct kunit *test)
> up_write(&sb->s_umount);
>
> return 0;
> +
> +out_deactivate:
> + kfree(k_ctx.k_ei);
> + k_ctx.k_ei = NULL;
> +
> + kfree(k_ctx.k_data);
> + k_ctx.k_data = NULL;
> +
> + deactivate_locked_super(sb);
So I guess the right thing to do for now is, what Ojaswin also mentioned
[1]. Let's go with patch [2], as is.. and we can fix all the remaining
issues as part of this series. With that in mind, this patch series
needs to be applid on top of [2]. So that means.. our error handling
should also properly take care of unregister shrinker and freeing sbi..
[1]: https://lore.kernel.org/linux-ext4/abPh6GX0t22m628D@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com/
[2]: https://lore.kernel.org/linux-ext4/5bb9041471dab8ce870c191c19cbe4df57473be8.1772381213.git.ritesh.list@gmail.com/#t
-ritesh
> +
> + return err;
> }
>
> /*
> --
> 2.34.1
^ permalink raw reply
* Re: [PATCH -next 5/8] ext4: fix miss free super_block in extents_kunit_exit()
From: Ritesh Harjani @ 2026-03-13 12:03 UTC (permalink / raw)
To: Ye Bin, tytso, adilger.kernel, linux-ext4; +Cc: jack
In-Reply-To: <20260310130412.3156753-6-yebin@huaweicloud.com>
Ye Bin <yebin@huaweicloud.com> writes:
> From: Ye Bin <yebin10@huawei.com>
>
> There's issue as follows:
> ODEBUG: free active (active state 0) object: ffff88812f2d68a0 object type: percpu_counter hint: 0x0
> <TASK>
> debug_check_no_obj_freed+0x3d9/0x4d0
> kfree+0x2bb/0x6c0
> extents_kunit_exit+0x65/0x90 [ext4_test]
> kunit_try_run_case_cleanup+0xbc/0x100 [kunit]
> kunit_generic_run_threadfn_adapter+0x89/0x100 [kunit]
> kthread+0x408/0x540
> ret_from_fork+0xa76/0xdf0
> ret_from_fork_asm+0x1a/0x30
>
> The above issue was caused because the super_block cleanup process
> was not properly performed.
> Therefore, deactivate_super() is called in extents_kunit_exit() to
> cleanup the super_block.
>
> Fixes: cb1e0c1d1fad ("ext4: kunit tests for extent splitting and conversion")
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> fs/ext4/extents-test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ext4/extents-test.c b/fs/ext4/extents-test.c
> index 5a5016ea1ecc..70d84c0a18e2 100644
> --- a/fs/ext4/extents-test.c
> +++ b/fs/ext4/extents-test.c
> @@ -144,7 +144,7 @@ static void extents_kunit_exit(struct kunit *test)
> {
> struct ext4_sb_info *sbi = k_ctx.k_ei->vfs_inode.i_sb->s_fs_info;
>
> - kfree(sbi);
> + deactivate_super(sbi->s_sb);
This doesn't look correct. We don't have sbi->s_sb->s_op->put_super() registered, so
kfree(sbi) never happens. Maybe it will be good to check with kmemleak
too, that this could cause a memory leak.
I think we can do:
ext4_es_unregister_shrinker(sbi); // already taken care in [1]
deactivate_super(sbi->s_sb)
kfree(sbi);
[1]: https://lore.kernel.org/linux-ext4/5bb9041471dab8ce870c191c19cbe4df57473be8.1772381213.git.ritesh.list@gmail.com/
-ritesh
^ permalink raw reply
* Re: [PATCH -next 4/8] ext4: fix miss unlock 'sb->s_umount' in extents_kunit_init()
From: Ritesh Harjani @ 2026-03-13 11:32 UTC (permalink / raw)
To: Ye Bin, tytso, adilger.kernel, linux-ext4; +Cc: jack
In-Reply-To: <20260310130412.3156753-5-yebin@huaweicloud.com>
Ye Bin <yebin@huaweicloud.com> writes:
> From: Ye Bin <yebin10@huawei.com>
>
> There's warning as follows when do ext4 kunit test:
> WARNING: kunit_try_catch/15923 still has locks held!
> 7.0.0-rc3-next-20260309-00028-g73f965a1bbb1-dirty #281 Tainted: G E N
> 1 lock held by kunit_try_catch/15923:
Strange lockdep didn't complaint this for me.. I had lockdep enabled in
my config.
> #0: ffff888139f860e0 (&type->s_umount_key#70/1){+.+.}-{4:4}, at: alloc_super.constprop.0+0x172/0xa90
> Call Trace:
> <TASK>
> dump_stack_lvl+0x180/0x1b0
> debug_check_no_locks_held+0xc8/0xd0
> do_exit+0x1502/0x2b20
> kthread+0x3a9/0x540
> ret_from_fork+0xa76/0xdf0
> ret_from_fork_asm+0x1a/0x30
>
> As sget() will return 'sb' which holds 's->s_umount' lock. However,
> "extents-test" miss unlock this lock.
> So unlock 's->s_umount' in the end of extents_kunit_init().
>
> Fixes: cb1e0c1d1fad ("ext4: kunit tests for extent splitting and conversion")
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
> fs/ext4/extents-test.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/ext4/extents-test.c b/fs/ext4/extents-test.c
> index 9e055f399167..5a5016ea1ecc 100644
> --- a/fs/ext4/extents-test.c
> +++ b/fs/ext4/extents-test.c
> @@ -307,6 +307,8 @@ static int extents_kunit_init(struct kunit *test)
> kunit_activate_static_stub(test, ext4_ext_zeroout, ext4_ext_zeroout_stub);
> kunit_activate_static_stub(test, ext4_issue_zeroout,
> ext4_issue_zeroout_stub);
> + up_write(&sb->s_umount);
> +
Can we re-arrange these patches please?
Let's bring the error handling fix first (I think i.e. patch-6 in this
series) so that, s_umount lock also gets unlocked properly from the
error handling paths as well.
With that, I agree releasing the s_umount lock is the right thing to do
here which is taken in:
sget()
alloc_super()
down_write_nested(&s->s_umount, SINGLE_DEPTH_NESTING);
So feel free to add:
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
-ritesh
^ permalink raw reply
* Re: [PATCH v4 19/25] xfs: remove unwritten extents after preallocations in fsverity metadata
From: Andrey Albershteyn @ 2026-03-13 11:17 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Andrey Albershteyn, linux-xfs, fsverity, linux-fsdevel, ebiggers,
hch, linux-ext4, linux-f2fs-devel, linux-btrfs
In-Reply-To: <20260312145250.GE6069@frogsfrogsfrogs>
On 2026-03-12 07:52:50, Darrick J. Wong wrote:
> On Thu, Mar 12, 2026 at 02:50:57PM +0100, Andrey Albershteyn wrote:
> > On 2026-03-09 18:29:14, Darrick J. Wong wrote:
> > > > XFS preallocates spaces during writes. In normal I/O this space, if
> > > > unused, is removed by truncate. For files with fsverity, XFS does not
> > > > use truncate as fsverity metadata is stored past EOF.
> > > >
> > > > After we're done with writing fsverity metadata iterate over extents in
> > > > that region and remove any unwritten ones. These would be preallocation
> > > > leftovers in the merkle tree holes and past fsverity descriptor.
> > > >
> > > > Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> > > > ---
> >
> > > There's an upper limit on the number of blocks you can unmap/free in a
> > > single transaction. Maybe move the xfs_trans_{alloc,commit} into the
> > > loop body?
> > >
> > > Oh wait, you skip the written extents. Ok, so maybe just roll it after
> > > you've done a bunmapi.
> >
> > I see, I will add a rolling transaction. Btw, why skipping the
> > written extents let's us use rolling here?
>
> Hrmm. At first I thought: why can't xfs_fsverity_cancel_unwritten
> allocate (and commit) the transaction inside the loop body? Then I
> thought "well, it's only conditionally unmapping things, and it's sort
> of a pain to allocate a transaction only then to find out if you
> actually want to run one".
>
> OTOH there could be billions of extents in the data fork, so holding the
> ILOCK for that many transactions isn't a good thing because tr_write
> only preallocates space for a certain number of transaction rolls.
> fsverity already holds IOLOCK_EXCL so there can't be any other programs
> using the file.
>
> So now I arrive back at "The transaction allocation and commit/cancel
> should be inside the loop body, since crashing midway through wouldn't
> result in user-visible changes to the file data".
>
> > > Do you need to purge the cow fork too?
> >
> > hmm, what case are you thinking about here? The fsverity is written
> > in past EOF region, I don't see how COW extent could be left there.
> > Can they somehow be left mapped for blocks past i_size?
>
> I was talking about speculative cow fork preallocations below i_size.
> They'll eventually get purged by blockgc, but you could give the space
> back once you've committed enabling the fsverity file flag.
hmm, if they're purged by blockgc, then why to do it here? I'm
iterating and removing unwritten extents here only because
xfs_inode_free_eofblocks() will skip fsverity inodes, the check in
xfs_can_free_eofblocks():
if (IS_VERITY(VFS_I(ip)))
return false;
The skipping is done to not remove all the fsverity metadata
extents. With preallocations enabled there could be unwritten
extents left in the metadata.
Am I missing something?
--
- Andrey
^ permalink raw reply
* Re: [PATCH -next 0/8] Fix some issues about ext4-test
From: Ojaswin Mujoo @ 2026-03-13 10:34 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack
In-Reply-To: <abO55xo3MDyJ-NEi@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com>
On Fri, Mar 13, 2026 at 12:46:55PM +0530, Ojaswin Mujoo wrote:
> On Tue, Mar 10, 2026 at 09:04:04PM +0800, Ye Bin wrote:
> > From: Ye Bin <yebin10@huawei.com>
> >
> > Patch [1]-[2]:
> > Decoupled mballoc-test and extents-test from ext4. Patch [1] does not
> > have any changes compared to the previously released version, so the
> > reviewed-by is added.
> > Patch [3-7]:
> > Bugfix for extents-test.c.
> > Patch [8]:
> > Bugfix for mballoc-test.c.
> >
> > Ye Bin (8):
> > ext4: fix mballoc-test.c is not compiled when EXT4_KUNIT_TESTS=M
> > ext4: introduce EXPORT_SYMBOL_FOR_EXT4_TEST() helper
> > ext4: fix extents-test.c is not compiled when EXT4_KUNIT_TESTS=M
> > ext4: fix miss unlock 'sb->s_umount' in extents_kunit_init()
> > ext4: fix miss free super_block in extents_kunit_exit()
> > ext4: fix the error handling process in extents_kunit_init).
> > ext4: fix possible null-ptr-deref in extents_kunit_exit()
> > ext4: fix possible null-ptr-deref in mbt_kunit_exit()
> >
> > fs/ext4/Makefile | 5 +-
> > fs/ext4/ext4.h | 5 ++
> > fs/ext4/ext4_extents.h | 12 +++++
> > fs/ext4/extents-test.c | 59 ++++++++++++++++--------
> > fs/ext4/extents.c | 38 ++++++++++++---
> > fs/ext4/mballoc-test.c | 87 ++++++++++++++++++-----------------
> > fs/ext4/mballoc.c | 102 +++++++++++++++++++++++++++++++++++++++--
> > fs/ext4/mballoc.h | 30 ++++++++++++
> > 8 files changed, 268 insertions(+), 70 deletions(-)
>
> Hi Ye,
>
> Thanks for the fixes, seems like I completely forgot cleaning up
> resources in extents-test.c so thanks for taking that up. Sadly, my UML
> kunit runs didn't catch any of this :/
>
> Although the series look good to me, I have a request, can you please
> resend the fixes in patch 4 - 7 separate from patch 1 2 3. We need
> these fixes (specifically patch 4 and 5) to fix the breakage in
> linux-next [1], keeping them separate can help Ted just pickup the
> urgent fixes independent of patch 1,2,3.
>
> Also, for patch 5, it fixes an issue that Ritesh fixed in his patche
> [2]. I think your patch is a better approach since all cleanup is
> handled via deactivate_super(). However, his commit message better
> describes the issue. While resending, can you include his commit message
> as well. (I've checked with him so feel free to reuse it).
Hey Ye, sorry for the confusion but you can ignore the above paragraph.
We still need Ritesh's patches along with yours. More details here [1]
That being said, I still feel the fixes here can go separately. I'll try
to priortize the review of patch 4 - 7 so thaat we can get them in shape
to be merged.
[1] https://lore.kernel.org/linux-ext4/abPh6GX0t22m628D@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com/
Regards,
ojaswin
>
> Thanks again,
> Ojaswin
>
> [1] https://lore.kernel.org/all/0d21783c-5299-4488-9708-def0b1a4dcbe@sirena.org.uk/
> [2] https://lore.kernel.org/linux-ext4/5bb9041471dab8ce870c191c19cbe4df57473be8.1772381213.git.ritesh.list@gmail.com/
>
> >
> > --
> > 2.34.1
> >
^ permalink raw reply
* Re: [PATCH] ext4: kunit: extents-test: Fix percpu_counters list corruption
From: Ojaswin Mujoo @ 2026-03-13 10:07 UTC (permalink / raw)
To: Mark Brown; +Cc: Ritesh Harjani (IBM), linux-ext4
In-Reply-To: <abO6o3RFQM24dYK9@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com>
On Fri, Mar 13, 2026 at 12:50:03PM +0530, Ojaswin Mujoo wrote:
> On Thu, Mar 12, 2026 at 03:57:38PM +0000, Mark Brown wrote:
> > On Sun, Mar 01, 2026 at 09:44:26PM +0530, Ritesh Harjani (IBM) wrote:
> > > commit 82f80e2e3b23 ("ext4: add extent status cache support to kunit tests"),
> > > added ext4_es_register_shrinker() in extents_kunit_init() function but
> > > failed to add the unregister shrinker routine in extents_kunit_exit().
> >
> > This is breaking KUnit in -next which is causing problems for other
> > trees, can we please get it merged promptly? Discussion here:
> >
> > https://lore.kernel.org/r/abJe1PAf4JbwGGRI@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com
>
> @Ted, the fixes here [1] supersede this patch. I've asked Ye Bin to post
> the fixes independent of other patches. Once we have those, we can get
> that merged and it should fix linux-next
>
> [1] https://lore.kernel.org/linux-ext4/abO55xo3MDyJ-NEi@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com/
>
> Regards,
> ojaswin
Hi Ted, upon discussing internally with Ritesh, I believe we still need
this patch and [1] doesn't fix the issue of linux-next, just hides it.
Basically I was assuming that the patch 5 of Ye's patch will handle
unregistering the shrinker via:
deactivate_super()
ext4_kill_sb()
...
generic_super_shutdown()
sop->put_super()
ext4_put_super()
ext4_es_unregister_shrinker()
kfree(sbi)
However I missed the fact that in extents_kunit_init() we never set
sb->s_op so ext4_put_super() is never called. In Ye's series, the issue
became hidden because the kfree(sbi) was never called anymore (memory
leak) causing slab to never return the corrupted page.
Hence this patch is needed for the immediate fix of linux-next. And then
we can look into merging Ye's fixes separately, once the review is
complete.
Sorry for the confusion.
Regards,
ojaswin
[1] https://lore.kernel.org/linux-ext4/20260312131253.366296-6-yebin@huaweicloud.com/
^ permalink raw reply
* Re: [PATCH] ext4: kunit: extents-test: Fix percpu_counters list corruption
From: Ojaswin Mujoo @ 2026-03-13 7:20 UTC (permalink / raw)
To: Mark Brown; +Cc: Ritesh Harjani (IBM), linux-ext4
In-Reply-To: <f6d26b93-e21c-45bf-b377-1896010e59fe@sirena.org.uk>
On Thu, Mar 12, 2026 at 03:57:38PM +0000, Mark Brown wrote:
> On Sun, Mar 01, 2026 at 09:44:26PM +0530, Ritesh Harjani (IBM) wrote:
> > commit 82f80e2e3b23 ("ext4: add extent status cache support to kunit tests"),
> > added ext4_es_register_shrinker() in extents_kunit_init() function but
> > failed to add the unregister shrinker routine in extents_kunit_exit().
>
> This is breaking KUnit in -next which is causing problems for other
> trees, can we please get it merged promptly? Discussion here:
>
> https://lore.kernel.org/r/abJe1PAf4JbwGGRI@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com
@Ted, the fixes here [1] supersede this patch. I've asked Ye Bin to post
the fixes independent of other patches. Once we have those, we can get
that merged and it should fix linux-next
[1] https://lore.kernel.org/linux-ext4/abO55xo3MDyJ-NEi@li-dc0c254c-257c-11b2-a85c-98b6c1322444.ibm.com/
Regards,
ojaswin
^ permalink raw reply
* Re: [PATCH -next 0/8] Fix some issues about ext4-test
From: Ojaswin Mujoo @ 2026-03-13 7:16 UTC (permalink / raw)
To: Ye Bin; +Cc: tytso, adilger.kernel, linux-ext4, jack
In-Reply-To: <20260310130412.3156753-1-yebin@huaweicloud.com>
On Tue, Mar 10, 2026 at 09:04:04PM +0800, Ye Bin wrote:
> From: Ye Bin <yebin10@huawei.com>
>
> Patch [1]-[2]:
> Decoupled mballoc-test and extents-test from ext4. Patch [1] does not
> have any changes compared to the previously released version, so the
> reviewed-by is added.
> Patch [3-7]:
> Bugfix for extents-test.c.
> Patch [8]:
> Bugfix for mballoc-test.c.
>
> Ye Bin (8):
> ext4: fix mballoc-test.c is not compiled when EXT4_KUNIT_TESTS=M
> ext4: introduce EXPORT_SYMBOL_FOR_EXT4_TEST() helper
> ext4: fix extents-test.c is not compiled when EXT4_KUNIT_TESTS=M
> ext4: fix miss unlock 'sb->s_umount' in extents_kunit_init()
> ext4: fix miss free super_block in extents_kunit_exit()
> ext4: fix the error handling process in extents_kunit_init).
> ext4: fix possible null-ptr-deref in extents_kunit_exit()
> ext4: fix possible null-ptr-deref in mbt_kunit_exit()
>
> fs/ext4/Makefile | 5 +-
> fs/ext4/ext4.h | 5 ++
> fs/ext4/ext4_extents.h | 12 +++++
> fs/ext4/extents-test.c | 59 ++++++++++++++++--------
> fs/ext4/extents.c | 38 ++++++++++++---
> fs/ext4/mballoc-test.c | 87 ++++++++++++++++++-----------------
> fs/ext4/mballoc.c | 102 +++++++++++++++++++++++++++++++++++++++--
> fs/ext4/mballoc.h | 30 ++++++++++++
> 8 files changed, 268 insertions(+), 70 deletions(-)
Hi Ye,
Thanks for the fixes, seems like I completely forgot cleaning up
resources in extents-test.c so thanks for taking that up. Sadly, my UML
kunit runs didn't catch any of this :/
Although the series look good to me, I have a request, can you please
resend the fixes in patch 4 - 7 separate from patch 1 2 3. We need
these fixes (specifically patch 4 and 5) to fix the breakage in
linux-next [1], keeping them separate can help Ted just pickup the
urgent fixes independent of patch 1,2,3.
Also, for patch 5, it fixes an issue that Ritesh fixed in his patche
[2]. I think your patch is a better approach since all cleanup is
handled via deactivate_super(). However, his commit message better
describes the issue. While resending, can you include his commit message
as well. (I've checked with him so feel free to reuse it).
Thanks again,
Ojaswin
[1] https://lore.kernel.org/all/0d21783c-5299-4488-9708-def0b1a4dcbe@sirena.org.uk/
[2] https://lore.kernel.org/linux-ext4/5bb9041471dab8ce870c191c19cbe4df57473be8.1772381213.git.ritesh.list@gmail.com/
>
> --
> 2.34.1
>
^ permalink raw reply
* [PATCH v1] ext4: fix use-after-free in update_super_work when racing with umount
From: Jiayuan Chen @ 2026-03-13 6:52 UTC (permalink / raw)
To: linux-ext4
Cc: Jiayuan Chen, Theodore Ts'o, Andreas Dilger, Jan Kara,
Ritesh Harjani, Ye Bin, linux-kernel
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>
---
fs/ext4/ext4.h | 1 +
fs/ext4/ext4_jbd2.h | 45 ++++++++++++++++++++++++++++-----------------
fs/ext4/super.c | 34 +++++++++++++++++++++++-----------
fs/ext4/sysfs.c | 10 ++++++++++
4 files changed, 62 insertions(+), 28 deletions(-)
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index b76966dc06c0..a693365d224c 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3757,6 +3757,7 @@ extern const struct inode_operations ext4_fast_symlink_inode_operations;
/* sysfs.c */
extern void ext4_notify_error_sysfs(struct ext4_sb_info *sbi);
extern int ext4_register_sysfs(struct super_block *sb);
+extern void ext4_sb_release_proc(struct super_block *sb);
extern void ext4_unregister_sysfs(struct super_block *sb);
extern int __init ext4_init_sysfs(void);
extern void ext4_exit_sysfs(void);
diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index 63d17c5201b5..2b7d68b11578 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -430,32 +430,43 @@ static inline int ext4_should_dioread_nolock(struct inode *inode)
}
/*
- * Pass journal explicitly as it may not be cached in the sbi->s_journal in some
- * cases
+ * Stop new s_sb_upd_work from being queued and flush any pending work.
+ *
+ * At this point only two things can be operating on the journal:
+ * JBD2 thread performing transaction commit and s_sb_upd_work
+ * issuing sb update through the journal. Once we set
+ * EXT4_MF_JOURNAL_DESTROY, new ext4_handle_error() calls will not
+ * queue s_sb_upd_work and ext4_force_commit() makes sure any
+ * ext4_handle_error() calls from the running transaction commit are
+ * finished. Hence no new s_sb_upd_work can be queued after we
+ * flush it here.
*/
-static inline int ext4_journal_destroy(struct ext4_sb_info *sbi, journal_t *journal)
+static inline void ext4_journal_stop_work(struct ext4_sb_info *sbi)
{
- int err = 0;
-
- /*
- * At this point only two things can be operating on the journal.
- * JBD2 thread performing transaction commit and s_sb_upd_work
- * issuing sb update through the journal. Once we set
- * EXT4_JOURNAL_DESTROY, new ext4_handle_error() calls will not
- * queue s_sb_upd_work and ext4_force_commit() makes sure any
- * ext4_handle_error() calls from the running transaction commit are
- * finished. Hence no new s_sb_upd_work can be queued after we
- * flush it here.
- */
ext4_set_mount_flag(sbi->s_sb, EXT4_MF_JOURNAL_DESTROY);
-
ext4_force_commit(sbi->s_sb);
flush_work(&sbi->s_sb_upd_work);
+}
+
+/*
+ * Destroy the journal. Must be called after ext4_journal_stop_work().
+ * Pass journal explicitly as it may not be cached in the sbi->s_journal
+ * in some cases.
+ */
+static inline int ext4_journal_finish(struct ext4_sb_info *sbi,
+ journal_t *journal)
+{
+ int err;
err = jbd2_journal_destroy(journal);
sbi->s_journal = NULL;
-
return err;
}
+static inline int ext4_journal_destroy(struct ext4_sb_info *sbi, journal_t *journal)
+{
+ ext4_journal_stop_work(sbi);
+ return ext4_journal_finish(sbi, journal);
+}
+
#endif /* _EXT4_JBD2_H */
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 752f414aa06b..9bba783f44e1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1280,16 +1280,12 @@ static void ext4_put_super(struct super_block *sb)
int err;
/*
- * Unregister sysfs before destroying jbd2 journal.
- * Since we could still access attr_journal_task attribute via sysfs
- * path which could have sbi->s_journal->j_task as NULL
- * Unregister sysfs before flush sbi->s_sb_upd_work.
- * Since user may read /proc/fs/ext4/xx/mb_groups during umount, If
- * read metadata verify failed then will queue error work.
- * update_super_work will call start_this_handle may trigger
- * BUG_ON.
+ * Remove procfs entries before flush s_sb_upd_work. Since user may
+ * read /proc/fs/ext4/xx/mb_groups during umount, if read metadata
+ * verify failed then will queue error work. update_super_work will
+ * call start_this_handle which may trigger BUG_ON.
*/
- ext4_unregister_sysfs(sb);
+ ext4_sb_release_proc(sb);
if (___ratelimit(&ext4_mount_msg_ratelimit, "EXT4-fs unmount"))
ext4_msg(sb, KERN_INFO, "unmounting filesystem %pU.",
@@ -1301,14 +1297,30 @@ static void ext4_put_super(struct super_block *sb)
destroy_workqueue(sbi->rsv_conversion_wq);
ext4_release_orphan_info(sb);
+ /*
+ * Flush s_sb_upd_work before unregistering sysfs, since
+ * update_super_work calls ext4_notify_error_sysfs() which accesses
+ * the kobject's kernfs_node via sysfs_notify(). Unregistering sysfs
+ * before the flush could lead to a use-after-free on the
+ * kernfs_node.
+ *
+ * Also unregister sysfs before destroying jbd2 journal, since
+ * userspace could read the journal_task sysfs attribute while
+ * jbd2_journal_destroy() is killing the journal thread, leading to
+ * a NULL pointer dereference of j_task in journal_task_show().
+ */
if (sbi->s_journal) {
aborted = is_journal_aborted(sbi->s_journal);
- err = ext4_journal_destroy(sbi, sbi->s_journal);
+ ext4_journal_stop_work(sbi);
+ ext4_unregister_sysfs(sb);
+ err = ext4_journal_finish(sbi, sbi->s_journal);
if ((err < 0) && !aborted) {
ext4_abort(sb, -err, "Couldn't clean up the journal");
}
- } else
+ } else {
flush_work(&sbi->s_sb_upd_work);
+ ext4_unregister_sysfs(sb);
+ }
ext4_es_unregister_shrinker(sbi);
timer_shutdown_sync(&sbi->s_err_report);
diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index d2ecc1026c0c..f6947416c1e7 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -638,6 +638,16 @@ int ext4_register_sysfs(struct super_block *sb)
return 0;
}
+void ext4_sb_release_proc(struct super_block *sb)
+{
+ struct ext4_sb_info *sbi = EXT4_SB(sb);
+
+ if (sbi->s_proc) {
+ remove_proc_subtree(sb->s_id, ext4_proc_root);
+ sbi->s_proc = NULL;
+ }
+}
+
void ext4_unregister_sysfs(struct super_block *sb)
{
struct ext4_sb_info *sbi = EXT4_SB(sb);
--
2.43.0
^ permalink raw reply related
* Re: [RFC PATCH] ext4: handle wraparound when searching for blocks for indirect mapped blocks
From: Baokun Li @ 2026-03-13 2:00 UTC (permalink / raw)
To: Theodore Tso; +Cc: Jan Kara, Ext4 Developers List, libaokun
In-Reply-To: <20260312142345.GA4689@macsyma-wired.lan>
On 3/12/26 10:23 PM, Theodore Tso wrote:
> On Wed, Mar 11, 2026 at 10:38:20AM +0800, Baokun Li wrote:
>> Good spotting! ext4_find_goal() ensures that the goal block obtained for
>> indirect-block-based files will not exceed EXT4_MAX_BLOCK_FILE_PHYS.
>> However, on an ext4 filesystem where the two file formats are mixed,
>> it is indeed possible to get an excessively large goal group via stream
>> allocation.
> Well, I didn't spot it; an LLM AI noticed. :-) Arguably I should have
> noticed it when doing my review, but I didn't.
>
>> Since the mixed-format case is quite rare, I think we can simply validate
>> start in ext4_mb_scan_groups() and reset it to 0 when it exceeds the limit,
>> like this:
> No, I don't think that's enough. It's not just that we could have an
> excessively large goal group. The goal group could also just be, say,
> 2**32 - 5. If the next 5 groups are full, then when we do an
> optimized scan, we will end up beyond the 2**32 limit. That's why we
> need to add some kidn of wraparound logic to *any* caller to
> ext4_get_allocation_groups_count().
>
IIUC, ngroups/end here only depend on filesystem size and whether the inode
is extent-based, and both should stay unchanged during block allocation.
So doing the check once at the beginning should be sufficient. Am I missing
anything?
Cheers,
Baokun
^ permalink raw reply
* [PATCH 19/53] afs: use d_time instead of d_fsdata
From: NeilBrown @ 2026-03-12 21:12 UTC (permalink / raw)
To: Linus Torvalds, Alexander Viro, Christian Brauner, Jan Kara,
Jeff Layton, Trond Myklebust, Anna Schumaker, Carlos Maiolino,
Miklos Szeredi, Amir Goldstein, Jan Harkes, Hugh Dickins,
Baolin Wang, David Howells, Marc Dionne, Steve French,
Namjae Jeon, Sungjong Seo, Yuezhang Mo, Andreas Hindborg,
Breno Leitao, Theodore Ts'o, Andreas Dilger, Steven Rostedt,
Masami Hiramatsu, Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko,
Tyler Hicks, Andreas Gruenbacher, Richard Weinberger,
Anton Ivanov, Johannes Berg, Jeremy Kerr, Ard Biesheuvel
Cc: linux-fsdevel, linux-nfs, linux-xfs, linux-unionfs, coda,
linux-mm, linux-afs, linux-cifs, linux-ext4, linux-kernel,
linux-trace-kernel, ceph-devel, ecryptfs, gfs2, linux-um,
linux-efi
In-Reply-To: <20260312214330.3885211-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
afs uses ->d_fsdata to store version information for the parent
directory. ->d_time is arguably a better field to store this
information as the version is like a time stamp, and ->d_time is an
unsigned long, while ->d_fsdata is a void *.
This will leave ->d_fsdata free for a different use ... which
admittedly is also not a void*, but is certainly not at all a time.
Interesting the value stored in ->d_time or d_fsdata is u64 which is a
different size of 32 bit hosts. Maybe that doesn't matter.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/afs/dir.c | 18 +++++++++---------
fs/afs/internal.h | 8 ++++----
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 78caef3f1338..a0417292314c 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -808,7 +808,7 @@ static struct inode *afs_do_lookup(struct inode *dir, struct dentry *dentry)
afs_dir_iterate(dir, &cookie->ctx, NULL, &data_version);
}
- dentry->d_fsdata = (void *)(unsigned long)data_version;
+ dentry->d_time = (unsigned long)data_version;
/* Check to see if we already have an inode for the primary fid. */
inode = ilookup5(dir->i_sb, cookie->fids[1].vnode,
@@ -895,9 +895,9 @@ static struct inode *afs_do_lookup(struct inode *dir, struct dentry *dentry)
}
if (op->file[0].scb.have_status)
- dentry->d_fsdata = (void *)(unsigned long)op->file[0].scb.status.data_version;
+ dentry->d_time = (unsigned long)op->file[0].scb.status.data_version;
else
- dentry->d_fsdata = (void *)(unsigned long)op->file[0].dv_before;
+ dentry->d_time = (unsigned long)op->file[0].dv_before;
ret = afs_put_operation(op);
out:
kfree(cookie);
@@ -1010,7 +1010,7 @@ static struct dentry *afs_lookup(struct inode *dir, struct dentry *dentry,
_debug("splice %p", dentry->d_inode);
d = d_splice_alias(inode, dentry);
if (!IS_ERR_OR_NULL(d)) {
- d->d_fsdata = dentry->d_fsdata;
+ d->d_time = dentry->d_time;
trace_afs_lookup(dvnode, &d->d_name, &fid);
} else {
trace_afs_lookup(dvnode, &dentry->d_name, &fid);
@@ -1040,7 +1040,7 @@ static int afs_d_revalidate_rcu(struct afs_vnode *dvnode, struct dentry *dentry)
* version.
*/
dir_version = (long)READ_ONCE(dvnode->status.data_version);
- de_version = (long)READ_ONCE(dentry->d_fsdata);
+ de_version = (long)READ_ONCE(dentry->d_time);
if (de_version != dir_version) {
dir_version = (long)READ_ONCE(dvnode->invalid_before);
if (de_version - dir_version < 0)
@@ -1100,7 +1100,7 @@ static int afs_d_revalidate(struct inode *parent_dir, const struct qstr *name,
* version.
*/
dir_version = dir->status.data_version;
- de_version = (long)dentry->d_fsdata;
+ de_version = (long)dentry->d_time;
if (de_version == (long)dir_version)
goto out_valid_noupdate;
@@ -1161,7 +1161,7 @@ static int afs_d_revalidate(struct inode *parent_dir, const struct qstr *name,
}
out_valid:
- dentry->d_fsdata = (void *)(unsigned long)dir_version;
+ dentry->d_time = (unsigned long)dir_version;
out_valid_noupdate:
key_put(key);
_leave(" = 1 [valid]");
@@ -1931,7 +1931,7 @@ static void afs_rename_edit_dir(struct afs_operation *op)
spin_unlock(&new_inode->i_lock);
}
- /* Now we can update d_fsdata on the dentries to reflect their
+ /* Now we can update d_time on the dentries to reflect their
* new parent's data_version.
*/
afs_update_dentry_version(op, new_dvp, op->dentry);
@@ -2167,7 +2167,7 @@ static int afs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
}
/* This bit is potentially nasty as there's a potential race with
- * afs_d_revalidate{,_rcu}(). We have to change d_fsdata on the dentry
+ * afs_d_revalidate{,_rcu}(). We have to change d_time_ on the dentry
* to reflect it's new parent's new data_version after the op, but
* d_revalidate may see old_dentry between the op having taken place
* and the version being updated.
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 009064b8d661..106a7fe06b56 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -1746,17 +1746,17 @@ static inline struct inode *AFS_VNODE_TO_I(struct afs_vnode *vnode)
}
/*
- * Note that a dentry got changed. We need to set d_fsdata to the data version
+ * Note that a dentry got changed. We need to set d_time to the data version
* number derived from the result of the operation. It doesn't matter if
- * d_fsdata goes backwards as we'll just revalidate.
+ * d_time goes backwards as we'll just revalidate.
*/
static inline void afs_update_dentry_version(struct afs_operation *op,
struct afs_vnode_param *dir_vp,
struct dentry *dentry)
{
if (!op->cumul_error.error)
- dentry->d_fsdata =
- (void *)(unsigned long)dir_vp->scb.status.data_version;
+ dentry->d_time =
+ (unsigned long)dir_vp->scb.status.data_version;
}
/*
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH 20/53] afs: don't unhash/rehash dentries during unlink/rename
From: NeilBrown @ 2026-03-12 21:12 UTC (permalink / raw)
To: Linus Torvalds, Alexander Viro, Christian Brauner, Jan Kara,
Jeff Layton, Trond Myklebust, Anna Schumaker, Carlos Maiolino,
Miklos Szeredi, Amir Goldstein, Jan Harkes, Hugh Dickins,
Baolin Wang, David Howells, Marc Dionne, Steve French,
Namjae Jeon, Sungjong Seo, Yuezhang Mo, Andreas Hindborg,
Breno Leitao, Theodore Ts'o, Andreas Dilger, Steven Rostedt,
Masami Hiramatsu, Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko,
Tyler Hicks, Andreas Gruenbacher, Richard Weinberger,
Anton Ivanov, Johannes Berg, Jeremy Kerr, Ard Biesheuvel
Cc: linux-fsdevel, linux-nfs, linux-xfs, linux-unionfs, coda,
linux-mm, linux-afs, linux-cifs, linux-ext4, linux-kernel,
linux-trace-kernel, ceph-devel, ecryptfs, gfs2, linux-um,
linux-efi
In-Reply-To: <20260312214330.3885211-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
afs needs to block lookup of dentries during unlink and rename.
There are two reasons:
1/ If the target is to be removed, not silly-renamed, the subsequent
opens cannot be allowed as the file won't exist on the server.
2/ If the rename source is being moved between directories a lookup,
particularly d_revalidate, might change ->d_time asynchronously
with rename changing ->d_time with possible incorrect results.
afs current unhashes the dentry to force a lookup which will wait on the
directory lock, and rehashes afterwards. This is incompatible with
proposed changed to directory locking which will require a dentry to
remain hashed throughout rename/unlink/etc operations.
This patch copies a mechanism developed for NFS. ->d_fsdata which is
currently unused is now set to a non-NULL value when lookups must be
blocked. d_revalidate checks for this value, and waits for it to become
NULL.
->d_lock is used to ensure d_revalidate never updates ->d_time while
->d_fsdata is set.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/afs/afs.h | 7 ++++++
fs/afs/dir.c | 64 +++++++++++++++++++++++++++++------------------
fs/afs/internal.h | 5 +---
3 files changed, 47 insertions(+), 29 deletions(-)
diff --git a/fs/afs/afs.h b/fs/afs/afs.h
index ec3db00bd081..019e77b08458 100644
--- a/fs/afs/afs.h
+++ b/fs/afs/afs.h
@@ -26,6 +26,13 @@ typedef u64 afs_volid_t;
typedef u64 afs_vnodeid_t;
typedef u64 afs_dataversion_t;
+/* This is stored in ->d_fsdata to stop d_revalidate looking at,
+ * and possibly changing, ->d_time on a dentry which is being moved
+ * between directories, and to block lookup for dentry that is
+ * being removed without silly-rename.
+ */
+#define AFS_FSDATA_BLOCKED ((void*)1)
+
typedef enum {
AFSVL_RWVOL, /* read/write volume */
AFSVL_ROVOL, /* read-only volume */
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index a0417292314c..9c57614feccf 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -1034,6 +1034,10 @@ static int afs_d_revalidate_rcu(struct afs_vnode *dvnode, struct dentry *dentry)
if (!afs_check_validity(dvnode))
return -ECHILD;
+ /* A rename/unlink is pending */
+ if (dentry->d_fsdata)
+ return -ECHILD;
+
/* We only need to invalidate a dentry if the server's copy changed
* behind our back. If we made the change, it's no problem. Note that
* on a 32-bit system, we only have 32 bits in the dentry to store the
@@ -1069,6 +1073,10 @@ static int afs_d_revalidate(struct inode *parent_dir, const struct qstr *name,
if (flags & LOOKUP_RCU)
return afs_d_revalidate_rcu(dir, dentry);
+ /* Wait for rename/unlink to complete */
+wait_for_rename:
+ wait_var_event(&dentry->d_fsdata, dentry->d_fsdata == NULL);
+
if (d_really_is_positive(dentry)) {
vnode = AFS_FS_I(d_inode(dentry));
_enter("{v={%llx:%llu} n=%pd fl=%lx},",
@@ -1161,7 +1169,13 @@ static int afs_d_revalidate(struct inode *parent_dir, const struct qstr *name,
}
out_valid:
+ spin_lock(&dentry->d_lock);
+ if (dentry->d_fsdata) {
+ spin_unlock(&dentry->d_lock);
+ goto wait_for_rename;
+ }
dentry->d_time = (unsigned long)dir_version;
+ spin_unlock(&dentry->d_lock);
out_valid_noupdate:
key_put(key);
_leave(" = 1 [valid]");
@@ -1536,8 +1550,7 @@ static void afs_unlink_edit_dir(struct afs_operation *op)
static void afs_unlink_put(struct afs_operation *op)
{
_enter("op=%08x", op->debug_id);
- if (op->unlink.need_rehash && afs_op_error(op) < 0 && afs_op_error(op) != -ENOENT)
- d_rehash(op->dentry);
+ store_release_wake_up(&op->dentry->d_fsdata, NULL);
}
static const struct afs_operation_ops afs_unlink_operation = {
@@ -1591,11 +1604,7 @@ static int afs_unlink(struct inode *dir, struct dentry *dentry)
afs_op_set_error(op, afs_sillyrename(dvnode, vnode, dentry, op->key));
goto error;
}
- if (!d_unhashed(dentry)) {
- /* Prevent a race with RCU lookup. */
- __d_drop(dentry);
- op->unlink.need_rehash = true;
- }
+ dentry->d_fsdata = AFS_FSDATA_BLOCKED;
spin_unlock(&dentry->d_lock);
op->file[1].vnode = vnode;
@@ -1885,9 +1894,10 @@ static void afs_rename_edit_dir(struct afs_operation *op)
_enter("op=%08x", op->debug_id);
- if (op->rename.rehash) {
- d_rehash(op->rename.rehash);
- op->rename.rehash = NULL;
+ if (op->rename.unblock) {
+ /* Rename has finished, so unlocks lookups to target */
+ store_release_wake_up(&op->rename.unblock->d_fsdata, NULL);
+ op->rename.unblock = NULL;
}
fscache_begin_write_operation(&orig_cres, afs_vnode_cache(orig_dvnode));
@@ -1970,6 +1980,9 @@ static void afs_rename_exchange_edit_dir(struct afs_operation *op)
d_exchange(old_dentry, new_dentry);
up_write(&orig_dvnode->validate_lock);
+ /* dentry has been moved, so d_validate can safely proceed */
+ store_release_wake_up(&old_dentry->d_fsdata, NULL);
+
} else {
down_write(&orig_dvnode->validate_lock);
if (test_bit(AFS_VNODE_DIR_VALID, &orig_dvnode->flags) &&
@@ -2009,11 +2022,10 @@ static void afs_rename_exchange_edit_dir(struct afs_operation *op)
static void afs_rename_put(struct afs_operation *op)
{
_enter("op=%08x", op->debug_id);
- if (op->rename.rehash)
- d_rehash(op->rename.rehash);
+ if (op->rename.unblock)
+ store_release_wake_up(&op->rename.unblock->d_fsdata, NULL);
+ store_release_wake_up(&op->dentry->d_fsdata, NULL);
dput(op->rename.tmp);
- if (afs_op_error(op))
- d_rehash(op->dentry);
}
static const struct afs_operation_ops afs_rename_operation = {
@@ -2121,7 +2133,6 @@ static int afs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
op->ops = &afs_rename_noreplace_operation;
} else if (flags & RENAME_EXCHANGE) {
op->ops = &afs_rename_exchange_operation;
- d_drop(new_dentry);
} else {
/* If we might displace the target, we might need to do silly
* rename.
@@ -2135,14 +2146,12 @@ static int afs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
*/
if (d_is_positive(new_dentry) && !d_is_dir(new_dentry)) {
/* To prevent any new references to the target during
- * the rename, we unhash the dentry in advance.
+ * the rename, we set d_fsdata which afs_d_revalidate will wait for.
+ * d_lock ensures d_count() and ->d_fsdata are consistent.
*/
- if (!d_unhashed(new_dentry)) {
- d_drop(new_dentry);
- op->rename.rehash = new_dentry;
- }
-
+ spin_lock(&new_dentry->d_lock);
if (d_count(new_dentry) > 2) {
+ spin_unlock(&new_dentry->d_lock);
/* copy the target dentry's name */
op->rename.tmp = d_alloc(new_dentry->d_parent,
&new_dentry->d_name);
@@ -2160,8 +2169,12 @@ static int afs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
}
op->dentry_2 = op->rename.tmp;
- op->rename.rehash = NULL;
op->rename.new_negative = true;
+ } else {
+ /* Block any lookups to target until the rename completes */
+ new_dentry->d_fsdata = AFS_FSDATA_BLOCKED;
+ op->rename.unblock = new_dentry;
+ spin_unlock(&new_dentry->d_lock);
}
}
}
@@ -2172,10 +2185,11 @@ static int afs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
* d_revalidate may see old_dentry between the op having taken place
* and the version being updated.
*
- * So drop the old_dentry for now to make other threads go through
- * lookup instead - which we hold a lock against.
+ * So block revalidate on the old_dentry until the rename completes.
*/
- d_drop(old_dentry);
+ spin_lock(&old_dentry->d_lock);
+ old_dentry->d_fsdata = AFS_FSDATA_BLOCKED;
+ spin_unlock(&old_dentry->d_lock);
ret = afs_do_sync_operation(op);
if (ret == -ENOTSUPP)
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index 106a7fe06b56..f2898ce9c0e6 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -891,10 +891,7 @@ struct afs_operation {
const char *symlink;
} create;
struct {
- bool need_rehash;
- } unlink;
- struct {
- struct dentry *rehash;
+ struct dentry *unblock;
struct dentry *tmp;
unsigned int rename_flags;
bool new_negative;
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH 21/53] afs: use d_splice_alias() in afs_vnode_new_inode()
From: NeilBrown @ 2026-03-12 21:12 UTC (permalink / raw)
To: Linus Torvalds, Alexander Viro, Christian Brauner, Jan Kara,
Jeff Layton, Trond Myklebust, Anna Schumaker, Carlos Maiolino,
Miklos Szeredi, Amir Goldstein, Jan Harkes, Hugh Dickins,
Baolin Wang, David Howells, Marc Dionne, Steve French,
Namjae Jeon, Sungjong Seo, Yuezhang Mo, Andreas Hindborg,
Breno Leitao, Theodore Ts'o, Andreas Dilger, Steven Rostedt,
Masami Hiramatsu, Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko,
Tyler Hicks, Andreas Gruenbacher, Richard Weinberger,
Anton Ivanov, Johannes Berg, Jeremy Kerr, Ard Biesheuvel
Cc: linux-fsdevel, linux-nfs, linux-xfs, linux-unionfs, coda,
linux-mm, linux-afs, linux-cifs, linux-ext4, linux-kernel,
linux-trace-kernel, ceph-devel, ecryptfs, gfs2, linux-um,
linux-efi
In-Reply-To: <20260312214330.3885211-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
As afs supports the fhandle interfaces there is a theoretical possibility
that the inode created for mkdir could be found by open_by_handle_at()
and given a dentry before d_instantiate() is called. This would result
in two dentries for the one directory inode, which is not permitted.
So this patch changes afs_mkdir() to use d_splice_alias() and to
return the alternate dentry from ->mkdir() if appropriate.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/afs/dir.c | 14 ++++++++++----
fs/afs/internal.h | 1 +
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 9c57614feccf..1e472768e1f1 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -1248,7 +1248,7 @@ void afs_check_for_remote_deletion(struct afs_operation *op)
/*
* Create a new inode for create/mkdir/symlink
*/
-static void afs_vnode_new_inode(struct afs_operation *op)
+static struct dentry *afs_vnode_new_inode(struct afs_operation *op)
{
struct afs_vnode_param *dvp = &op->file[0];
struct afs_vnode_param *vp = &op->file[1];
@@ -1265,7 +1265,7 @@ static void afs_vnode_new_inode(struct afs_operation *op)
* the new directory on the server.
*/
afs_op_accumulate_error(op, PTR_ERR(inode), 0);
- return;
+ return NULL;
}
vnode = AFS_FS_I(inode);
@@ -1276,7 +1276,7 @@ static void afs_vnode_new_inode(struct afs_operation *op)
afs_init_new_symlink(vnode, op);
if (!afs_op_error(op))
afs_cache_permit(vnode, op->key, vnode->cb_break, &vp->scb);
- d_instantiate(op->dentry, inode);
+ return d_splice_alias(inode, op->dentry);
}
static void afs_create_success(struct afs_operation *op)
@@ -1285,7 +1285,7 @@ static void afs_create_success(struct afs_operation *op)
op->ctime = op->file[0].scb.status.mtime_client;
afs_vnode_commit_status(op, &op->file[0]);
afs_update_dentry_version(op, &op->file[0], op->dentry);
- afs_vnode_new_inode(op);
+ op->create.ret = afs_vnode_new_inode(op);
}
static void afs_create_edit_dir(struct afs_operation *op)
@@ -1356,6 +1356,12 @@ static struct dentry *afs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
op->ops = &afs_mkdir_operation;
ret = afs_do_sync_operation(op);
afs_dir_unuse_cookie(dvnode, ret);
+ if (op->create.ret) {
+ /* Alternate dentry */
+ if (ret == 0)
+ return op->create.ret;
+ dput(op->create.ret);
+ }
return ERR_PTR(ret);
}
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index f2898ce9c0e6..ce94f10a14c0 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -889,6 +889,7 @@ struct afs_operation {
int reason; /* enum afs_edit_dir_reason */
mode_t mode;
const char *symlink;
+ struct dentry *ret;
} create;
struct {
struct dentry *unblock;
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH 22/53] afs: use d_alloc_nonblock in afs_sillyrename()
From: NeilBrown @ 2026-03-12 21:12 UTC (permalink / raw)
To: Linus Torvalds, Alexander Viro, Christian Brauner, Jan Kara,
Jeff Layton, Trond Myklebust, Anna Schumaker, Carlos Maiolino,
Miklos Szeredi, Amir Goldstein, Jan Harkes, Hugh Dickins,
Baolin Wang, David Howells, Marc Dionne, Steve French,
Namjae Jeon, Sungjong Seo, Yuezhang Mo, Andreas Hindborg,
Breno Leitao, Theodore Ts'o, Andreas Dilger, Steven Rostedt,
Masami Hiramatsu, Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko,
Tyler Hicks, Andreas Gruenbacher, Richard Weinberger,
Anton Ivanov, Johannes Berg, Jeremy Kerr, Ard Biesheuvel
Cc: linux-fsdevel, linux-nfs, linux-xfs, linux-unionfs, coda,
linux-mm, linux-afs, linux-cifs, linux-ext4, linux-kernel,
linux-trace-kernel, ceph-devel, ecryptfs, gfs2, linux-um,
linux-efi
In-Reply-To: <20260312214330.3885211-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
Rather than performing a normal lookup (which will be awkward with future
locking changes) use d_alloc_noblock() to find a dentry for an
unused name, and use an open-coded lookup_slow() to see if it is free on
the server.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/afs/dir_silly.c | 51 ++++++++++++++++++++++++++++++----------------
1 file changed, 34 insertions(+), 17 deletions(-)
diff --git a/fs/afs/dir_silly.c b/fs/afs/dir_silly.c
index 982bb6ec15f0..699143b21cdd 100644
--- a/fs/afs/dir_silly.c
+++ b/fs/afs/dir_silly.c
@@ -112,7 +112,9 @@ int afs_sillyrename(struct afs_vnode *dvnode, struct afs_vnode *vnode,
struct dentry *dentry, struct key *key)
{
static unsigned int sillycounter;
- struct dentry *sdentry = NULL;
+ struct dentry *sdentry = NULL, *old;
+ struct inode *dir = dentry->d_parent->d_inode;
+ struct qstr qsilly;
unsigned char silly[16];
int ret = -EBUSY;
@@ -122,23 +124,38 @@ int afs_sillyrename(struct afs_vnode *dvnode, struct afs_vnode *vnode,
if (dentry->d_flags & DCACHE_NFSFS_RENAMED)
return -EBUSY;
- sdentry = NULL;
- do {
- dput(sdentry);
- sillycounter++;
-
- /* Create a silly name. Note that the ".__afs" prefix is
- * understood by the salvager and must not be changed.
- */
- scnprintf(silly, sizeof(silly), ".__afs%04X", sillycounter);
- sdentry = lookup_noperm(&QSTR(silly), dentry->d_parent);
+newname:
+ sillycounter++;
- /* N.B. Better to return EBUSY here ... it could be dangerous
- * to delete the file while it's in use.
- */
- if (IS_ERR(sdentry))
- goto out;
- } while (!d_is_negative(sdentry));
+ /* Create a silly name. Note that the ".__afs" prefix is
+ * understood by the salvager and must not be changed.
+ */
+ scnprintf(silly, sizeof(silly), ".__afs%04X", sillycounter);
+ qsilly = QSTR(silly);
+ sdentry = try_lookup_noperm(&qsilly, dentry->d_parent);
+ if (!sdentry)
+ sdentry = d_alloc_noblock(dentry->d_parent, &qsilly);
+ if (sdentry == ERR_PTR(-EWOULDBLOCK))
+ /* try another name */
+ goto newname;
+ /* N.B. Better to return EBUSY here ... it could be dangerous
+ * to delete the file while it's in use.
+ */
+ if (IS_ERR(sdentry))
+ goto out;
+ if (d_is_positive(sdentry)) {
+ dput(sdentry);
+ goto newname;
+ }
+ /* This name isn't known locally - check on server */
+ old = dir->i_op->lookup(dir, sdentry, 0);
+ d_lookup_done(sdentry);
+ if (old || d_is_positive(sdentry)) {
+ if (!IS_ERR(old))
+ dput(old);
+ dput(sdentry);
+ goto newname;
+ }
ihold(&vnode->netfs.inode);
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH 23/53] afs: lookup_atsys to drop and reclaim lock.
From: NeilBrown @ 2026-03-12 21:12 UTC (permalink / raw)
To: Linus Torvalds, Alexander Viro, Christian Brauner, Jan Kara,
Jeff Layton, Trond Myklebust, Anna Schumaker, Carlos Maiolino,
Miklos Szeredi, Amir Goldstein, Jan Harkes, Hugh Dickins,
Baolin Wang, David Howells, Marc Dionne, Steve French,
Namjae Jeon, Sungjong Seo, Yuezhang Mo, Andreas Hindborg,
Breno Leitao, Theodore Ts'o, Andreas Dilger, Steven Rostedt,
Masami Hiramatsu, Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko,
Tyler Hicks, Andreas Gruenbacher, Richard Weinberger,
Anton Ivanov, Johannes Berg, Jeremy Kerr, Ard Biesheuvel
Cc: linux-fsdevel, linux-nfs, linux-xfs, linux-unionfs, coda,
linux-mm, linux-afs, linux-cifs, linux-ext4, linux-kernel,
linux-trace-kernel, ceph-devel, ecryptfs, gfs2, linux-um,
linux-efi
In-Reply-To: <20260312214330.3885211-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
If afs is asked to lookup a name ending with @sys, it needs to look up
a different name for which is allocates a dentry with
d_alloc_parallel().
This is done while the parent lock is held which will be a problem in
a future patch where the ordering of the parent lock and
d_alloc_parallel() locking is reversed.
There is no actual need to hold the lock while d_alloc_parallel() is
called, so with this patch we drop the lock and reclaim it.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/afs/dir.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index 1e472768e1f1..c195ee851191 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -908,12 +908,14 @@ static struct inode *afs_do_lookup(struct inode *dir, struct dentry *dentry)
/*
* Look up an entry in a directory with @sys substitution.
*/
-static struct dentry *afs_lookup_atsys(struct inode *dir, struct dentry *dentry)
+static struct dentry *afs_lookup_atsys(struct inode *dir, struct dentry *dentry,
+ unsigned int flags)
{
struct afs_sysnames *subs;
struct afs_net *net = afs_i2net(dir);
struct dentry *ret;
char *buf, *p, *name;
+ struct qstr nm;
int len, i;
_enter("");
@@ -933,6 +935,13 @@ static struct dentry *afs_lookup_atsys(struct inode *dir, struct dentry *dentry)
refcount_inc(&subs->usage);
read_unlock(&net->sysnames_lock);
+ /* Calling d_alloc_parallel() while holding parent locked is undesirable.
+ * We don't really need the lock any more.
+ */
+ if (flags & LOOKUP_SHARED)
+ inode_unlock_shared(dir);
+ else
+ inode_unlock(dir);
for (i = 0; i < subs->nr; i++) {
name = subs->subs[i];
len = dentry->d_name.len - 4 + strlen(name);
@@ -942,7 +951,10 @@ static struct dentry *afs_lookup_atsys(struct inode *dir, struct dentry *dentry)
}
strcpy(p, name);
- ret = lookup_noperm(&QSTR(buf), dentry->d_parent);
+ nm = QSTR(buf);
+ ret = try_lookup_noperm(&nm, dentry->d_parent);
+ if (!ret)
+ ret = d_alloc_parallel(dentry->d_parent, &nm);
if (IS_ERR(ret) || d_is_positive(ret))
goto out_s;
dput(ret);
@@ -953,6 +965,10 @@ static struct dentry *afs_lookup_atsys(struct inode *dir, struct dentry *dentry)
*/
ret = NULL;
out_s:
+ if (flags & LOOKUP_SHARED)
+ inode_lock_shared(dir);
+ else
+ inode_lock_nested(dir, I_MUTEX_PARENT);
afs_put_sysnames(subs);
kfree(buf);
out_p:
@@ -998,7 +1014,7 @@ static struct dentry *afs_lookup(struct inode *dir, struct dentry *dentry,
dentry->d_name.name[dentry->d_name.len - 3] == 's' &&
dentry->d_name.name[dentry->d_name.len - 2] == 'y' &&
dentry->d_name.name[dentry->d_name.len - 1] == 's')
- return afs_lookup_atsys(dir, dentry);
+ return afs_lookup_atsys(dir, dentry, flags);
afs_stat_v(dvnode, n_lookup);
inode = afs_do_lookup(dir, dentry);
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH 24/53] afs: use d_duplicate()
From: NeilBrown @ 2026-03-12 21:12 UTC (permalink / raw)
To: Linus Torvalds, Alexander Viro, Christian Brauner, Jan Kara,
Jeff Layton, Trond Myklebust, Anna Schumaker, Carlos Maiolino,
Miklos Szeredi, Amir Goldstein, Jan Harkes, Hugh Dickins,
Baolin Wang, David Howells, Marc Dionne, Steve French,
Namjae Jeon, Sungjong Seo, Yuezhang Mo, Andreas Hindborg,
Breno Leitao, Theodore Ts'o, Andreas Dilger, Steven Rostedt,
Masami Hiramatsu, Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko,
Tyler Hicks, Andreas Gruenbacher, Richard Weinberger,
Anton Ivanov, Johannes Berg, Jeremy Kerr, Ard Biesheuvel
Cc: linux-fsdevel, linux-nfs, linux-xfs, linux-unionfs, coda,
linux-mm, linux-afs, linux-cifs, linux-ext4, linux-kernel,
linux-trace-kernel, ceph-devel, ecryptfs, gfs2, linux-um,
linux-efi
In-Reply-To: <20260312214330.3885211-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
To prepare for d_alloc_parallel() being permitted without a directory
lock, use d_duplicate() when duplicating a dentry in order to create a
whiteout.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/afs/dir.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index c195ee851191..b5c593f50079 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -2047,6 +2047,8 @@ static void afs_rename_put(struct afs_operation *op)
if (op->rename.unblock)
store_release_wake_up(&op->rename.unblock->d_fsdata, NULL);
store_release_wake_up(&op->dentry->d_fsdata, NULL);
+ if (op->rename.tmp)
+ d_lookup_done(op->rename.tmp);
dput(op->rename.tmp);
}
@@ -2175,8 +2177,7 @@ static int afs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
if (d_count(new_dentry) > 2) {
spin_unlock(&new_dentry->d_lock);
/* copy the target dentry's name */
- op->rename.tmp = d_alloc(new_dentry->d_parent,
- &new_dentry->d_name);
+ op->rename.tmp = d_duplicate(new_dentry);
if (!op->rename.tmp) {
afs_op_nomem(op);
goto error;
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH 25/53] smb/client: use d_time to store a timestamp in dentry, not d_fsdata
From: NeilBrown @ 2026-03-12 21:12 UTC (permalink / raw)
To: Linus Torvalds, Alexander Viro, Christian Brauner, Jan Kara,
Jeff Layton, Trond Myklebust, Anna Schumaker, Carlos Maiolino,
Miklos Szeredi, Amir Goldstein, Jan Harkes, Hugh Dickins,
Baolin Wang, David Howells, Marc Dionne, Steve French,
Namjae Jeon, Sungjong Seo, Yuezhang Mo, Andreas Hindborg,
Breno Leitao, Theodore Ts'o, Andreas Dilger, Steven Rostedt,
Masami Hiramatsu, Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko,
Tyler Hicks, Andreas Gruenbacher, Richard Weinberger,
Anton Ivanov, Johannes Berg, Jeremy Kerr, Ard Biesheuvel
Cc: linux-fsdevel, linux-nfs, linux-xfs, linux-unionfs, coda,
linux-mm, linux-afs, linux-cifs, linux-ext4, linux-kernel,
linux-trace-kernel, ceph-devel, ecryptfs, gfs2, linux-um,
linux-efi
In-Reply-To: <20260312214330.3885211-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
smb/client uses d_fsdata is exactly the way that d_time is intended to
be used. It previous used d_time but this was changed in
Commit: a00be0e31f8d ("cifs: don't use ->d_time")
without any reason being given.
This patch effectively reverts that patch (though it doesn't remove the
helpers) so that d_fsdata can be used for something more generic.
Cc: Miklos Szeredi <miklos@szeredi.hu>
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/smb/client/cifsfs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/smb/client/cifsfs.h b/fs/smb/client/cifsfs.h
index e320d39b01f5..5153e811c50b 100644
--- a/fs/smb/client/cifsfs.h
+++ b/fs/smb/client/cifsfs.h
@@ -30,12 +30,12 @@ cifs_uniqueid_to_ino_t(u64 fileid)
static inline void cifs_set_time(struct dentry *dentry, unsigned long time)
{
- dentry->d_fsdata = (void *) time;
+ dentry->d_time = time;
}
static inline unsigned long cifs_get_time(struct dentry *dentry)
{
- return (unsigned long) dentry->d_fsdata;
+ return dentry->d_time;
}
extern struct file_system_type cifs_fs_type, smb3_fs_type;
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH 26/53] smb/client: don't unhashed and rehash to prevent new opens.
From: NeilBrown @ 2026-03-12 21:12 UTC (permalink / raw)
To: Linus Torvalds, Alexander Viro, Christian Brauner, Jan Kara,
Jeff Layton, Trond Myklebust, Anna Schumaker, Carlos Maiolino,
Miklos Szeredi, Amir Goldstein, Jan Harkes, Hugh Dickins,
Baolin Wang, David Howells, Marc Dionne, Steve French,
Namjae Jeon, Sungjong Seo, Yuezhang Mo, Andreas Hindborg,
Breno Leitao, Theodore Ts'o, Andreas Dilger, Steven Rostedt,
Masami Hiramatsu, Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko,
Tyler Hicks, Andreas Gruenbacher, Richard Weinberger,
Anton Ivanov, Johannes Berg, Jeremy Kerr, Ard Biesheuvel
Cc: linux-fsdevel, linux-nfs, linux-xfs, linux-unionfs, coda,
linux-mm, linux-afs, linux-cifs, linux-ext4, linux-kernel,
linux-trace-kernel, ceph-devel, ecryptfs, gfs2, linux-um,
linux-efi
In-Reply-To: <20260312214330.3885211-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
smb/client needs to block new opens of the target of unlink and rename
while the operation is progressing. This stablises d_count() and allows
a determination of whether a "silly-rename" is required.
It currently unhashes the dentry which will cause lookup to block on
the parent directory i_rwsem. Proposed changes to locking will cause
this approach to stop working and the exclusivity will be provided for
the dentry only, and only while it is hashed.
So we introduce a new machanism similar to that used by nfs and afs.
->d_fsdata (currently unused by smb/client) is set to a non-NULL
value when lookups need to be blocked. ->d_revalidate checks for this
and blocks. This might still allow d_count() to increment, but once it
has been tested as 1, there can be no new opens completed.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/smb/client/dir.c | 3 +++
fs/smb/client/inode.c | 48 +++++++++++++++++--------------------------
2 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index cb10088197d2..cecbc0cce5c5 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -790,6 +790,9 @@ cifs_d_revalidate(struct inode *dir, const struct qstr *name,
if (flags & LOOKUP_RCU)
return -ECHILD;
+ /* Wait for pending rename/unlink */
+ wait_var_event(&direntry->d_fsdata, direntry->d_fsdata == NULL);
+
if (d_really_is_positive(direntry)) {
int rc;
struct inode *inode = d_inode(direntry);
diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index d4d3cfeb6c90..3549605fa9c2 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -28,6 +28,13 @@
#include "cached_dir.h"
#include "reparse.h"
+/* This is stored in ->d_fsdata to block d_revalidate on a
+ * file dentry that is being removed - unlink or rename target.
+ * This causes any open attempt to block. There may be existing opens
+ * but they can be detected by checking d_count() under ->d_lock.
+ */
+#define CIFS_FSDATA_BLOCKED ((void *)1)
+
/*
* Set parameters for the netfs library
*/
@@ -1946,27 +1953,21 @@ static int __cifs_unlink(struct inode *dir, struct dentry *dentry, bool sillyren
__u32 dosattr = 0, origattr = 0;
struct TCP_Server_Info *server;
struct iattr *attrs = NULL;
- bool rehash = false;
cifs_dbg(FYI, "cifs_unlink, dir=0x%p, dentry=0x%p\n", dir, dentry);
if (unlikely(cifs_forced_shutdown(cifs_sb)))
return smb_EIO(smb_eio_trace_forced_shutdown);
- /* Unhash dentry in advance to prevent any concurrent opens */
- spin_lock(&dentry->d_lock);
- if (!d_unhashed(dentry)) {
- __d_drop(dentry);
- rehash = true;
- }
- spin_unlock(&dentry->d_lock);
-
tlink = cifs_sb_tlink(cifs_sb);
if (IS_ERR(tlink))
return PTR_ERR(tlink);
tcon = tlink_tcon(tlink);
server = tcon->ses->server;
+ /* Set d_fsdata to prevent any concurrent opens */
+ dentry->d_fsdata = CIFS_FSDATA_BLOCKED;
+
xid = get_xid();
page = alloc_dentry_path();
@@ -2083,8 +2084,9 @@ static int __cifs_unlink(struct inode *dir, struct dentry *dentry, bool sillyren
kfree(attrs);
free_xid(xid);
cifs_put_tlink(tlink);
- if (rehash)
- d_rehash(dentry);
+
+ /* Allow lookups */
+ store_release_wake_up(&dentry->d_fsdata, NULL);
return rc;
}
@@ -2501,7 +2503,6 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
struct cifs_sb_info *cifs_sb;
struct tcon_link *tlink;
struct cifs_tcon *tcon;
- bool rehash = false;
unsigned int xid;
int rc, tmprc;
int retry_count = 0;
@@ -2517,23 +2518,15 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
if (unlikely(cifs_forced_shutdown(cifs_sb)))
return smb_EIO(smb_eio_trace_forced_shutdown);
- /*
- * Prevent any concurrent opens on the target by unhashing the dentry.
- * VFS already unhashes the target when renaming directories.
- */
- if (d_is_positive(target_dentry) && !d_is_dir(target_dentry)) {
- if (!d_unhashed(target_dentry)) {
- d_drop(target_dentry);
- rehash = true;
- }
- }
-
tlink = cifs_sb_tlink(cifs_sb);
if (IS_ERR(tlink))
return PTR_ERR(tlink);
tcon = tlink_tcon(tlink);
server = tcon->ses->server;
+ /* Set d_fsdata to prevent any concurrent opens */
+ target_dentry->d_fsdata = CIFS_FSDATA_BLOCKED;
+
page1 = alloc_dentry_path();
page2 = alloc_dentry_path();
xid = get_xid();
@@ -2570,8 +2563,6 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
}
}
- if (!rc)
- rehash = false;
/*
* No-replace is the natural behavior for CIFS, so skip unlink hacks.
*/
@@ -2662,8 +2653,6 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
}
rc = cifs_do_rename(xid, source_dentry, from_name,
target_dentry, to_name);
- if (!rc)
- rehash = false;
}
}
@@ -2671,8 +2660,9 @@ cifs_rename2(struct mnt_idmap *idmap, struct inode *source_dir,
CIFS_I(source_dir)->time = CIFS_I(target_dir)->time = 0;
cifs_rename_exit:
- if (rehash)
- d_rehash(target_dentry);
+ /* Allow lookups */
+ store_release_wake_up(&target_dentry->d_fsdata, NULL);
+
kfree(info_buf_source);
free_dentry_path(page2);
free_dentry_path(page1);
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
* [PATCH 27/53] smb/client: use d_splice_alias() in atomic_open
From: NeilBrown @ 2026-03-12 21:12 UTC (permalink / raw)
To: Linus Torvalds, Alexander Viro, Christian Brauner, Jan Kara,
Jeff Layton, Trond Myklebust, Anna Schumaker, Carlos Maiolino,
Miklos Szeredi, Amir Goldstein, Jan Harkes, Hugh Dickins,
Baolin Wang, David Howells, Marc Dionne, Steve French,
Namjae Jeon, Sungjong Seo, Yuezhang Mo, Andreas Hindborg,
Breno Leitao, Theodore Ts'o, Andreas Dilger, Steven Rostedt,
Masami Hiramatsu, Ilya Dryomov, Alex Markuze, Viacheslav Dubeyko,
Tyler Hicks, Andreas Gruenbacher, Richard Weinberger,
Anton Ivanov, Johannes Berg, Jeremy Kerr, Ard Biesheuvel
Cc: linux-fsdevel, linux-nfs, linux-xfs, linux-unionfs, coda,
linux-mm, linux-afs, linux-cifs, linux-ext4, linux-kernel,
linux-trace-kernel, ceph-devel, ecryptfs, gfs2, linux-um,
linux-efi
In-Reply-To: <20260312214330.3885211-1-neilb@ownmail.net>
From: NeilBrown <neil@brown.name>
atomic_open can be called with a hashed-negative dentry or an in-lookup
dentry. Rather than d_drop() and d_add() we can use d_splice_alias()
which keeps the dentry hashed - important for proposed locking changes.
Signed-off-by: NeilBrown <neil@brown.name>
---
fs/smb/client/dir.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index cecbc0cce5c5..361a20987927 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -439,8 +439,7 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned
goto out_err;
}
- d_drop(direntry);
- d_add(direntry, newinode);
+ d_splice_alias(newinode, direntry);
out:
free_dentry_path(page);
--
2.50.0.107.gf914562f5916.dirty
^ permalink raw reply related
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