| Message ID | 20230907-vfs-ntfs3-kill_sb-v1-1-ef4397dd941d@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ntfs3: drop inode references in ntfs_put_super() | expand |
On Thu, 7 Sept 2023 at 09:04, Christian Brauner <brauner@kernel.org> wrote: > > Hey Linus, > > Would you mind applying this patch directly? It's a simple fix for an > ntfs3 bug that was introduced by some refactoring we did in mainline > only. Done. Thanks, Linus
On Thu, Sep 07, 2023 at 06:03:40PM +0200, Christian Brauner wrote: > Recently we moved most cleanup from ntfs_put_super() into > ntfs3_kill_sb() as part of a bigger cleanup. This accidently also moved > dropping inode references stashed in ntfs3's sb->s_fs_info from > @sb->put_super() to @sb->kill_sb(). But generic_shutdown_super() > verifies that there are no busy inodes past sb->put_super(). Fix this > and disentangle dropping inode references from freeing @sb->s_fs_info. Sorry for the delay, I've been travelling. Wouldn't it make more sense to just free it in ->kill_sb before calling kill_block_super? Either way the fix looks good, and as Linus has already applied it it's probably not worth arguing..
On Fri, Sep 08, 2023 at 10:12:13AM +0200, Christoph Hellwig wrote: > On Thu, Sep 07, 2023 at 06:03:40PM +0200, Christian Brauner wrote: > > Recently we moved most cleanup from ntfs_put_super() into > > ntfs3_kill_sb() as part of a bigger cleanup. This accidently also moved > > dropping inode references stashed in ntfs3's sb->s_fs_info from > > @sb->put_super() to @sb->kill_sb(). But generic_shutdown_super() > > verifies that there are no busy inodes past sb->put_super(). Fix this > > and disentangle dropping inode references from freeing @sb->s_fs_info. > > Sorry for the delay, I've been travelling. Wouldn't it make more > sense to just free it in ->kill_sb before calling kill_block_super? ntfs3 has ntfs_evict_inodes() which might depend on the info in sbi->s_fs_info to be valid. So calling it before kill_block_super() risks putting resources that ntfs_evict_inodes() might depend on. Doing it in ntfs_put_super() will prevent this from becoming an issue without having to wade through all callchains. And since we can't get rid of ntfs_put_super() anyway why bother risking that.
diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c index 5fffddea554f..cfec5e0c7f66 100644 --- a/fs/ntfs3/super.c +++ b/fs/ntfs3/super.c @@ -571,12 +571,8 @@ static void init_once(void *foo) /* * Noinline to reduce binary size. */ -static noinline void ntfs3_free_sbi(struct ntfs_sb_info *sbi) +static noinline void ntfs3_put_sbi(struct ntfs_sb_info *sbi) { - kfree(sbi->new_rec); - kvfree(ntfs_put_shared(sbi->upcase)); - kfree(sbi->def_table); - wnd_close(&sbi->mft.bitmap); wnd_close(&sbi->used.bitmap); @@ -601,6 +597,13 @@ static noinline void ntfs3_free_sbi(struct ntfs_sb_info *sbi) indx_clear(&sbi->security.index_sdh); indx_clear(&sbi->reparse.index_r); indx_clear(&sbi->objid.index_o); +} + +static void ntfs3_free_sbi(struct ntfs_sb_info *sbi) +{ + kfree(sbi->new_rec); + kvfree(ntfs_put_shared(sbi->upcase)); + kfree(sbi->def_table); kfree(sbi->compress.lznt); #ifdef CONFIG_NTFS3_LZX_XPRESS xpress_free_decompressor(sbi->compress.xpress); @@ -625,6 +628,7 @@ static void ntfs_put_super(struct super_block *sb) /* Mark rw ntfs as clear, if possible. */ ntfs_set_state(sbi, NTFS_DIRTY_CLEAR); + ntfs3_put_sbi(sbi); } static int ntfs_statfs(struct dentry *dentry, struct kstatfs *buf) @@ -1644,8 +1648,10 @@ static void ntfs_fs_free(struct fs_context *fc) struct ntfs_mount_options *opts = fc->fs_private; struct ntfs_sb_info *sbi = fc->s_fs_info; - if (sbi) + if (sbi) { + ntfs3_put_sbi(sbi); ntfs3_free_sbi(sbi); + } if (opts) put_mount_options(opts);
Recently we moved most cleanup from ntfs_put_super() into ntfs3_kill_sb() as part of a bigger cleanup. This accidently also moved dropping inode references stashed in ntfs3's sb->s_fs_info from @sb->put_super() to @sb->kill_sb(). But generic_shutdown_super() verifies that there are no busy inodes past sb->put_super(). Fix this and disentangle dropping inode references from freeing @sb->s_fs_info. Fixes: a4f64a300a29 ("ntfs3: free the sbi in ->kill_sb") # mainline only Reported-by: Guenter Roeck <linux@roeck-us.net> Tested-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Christian Brauner <brauner@kernel.org> --- Hey Linus, Would you mind applying this patch directly? It's a simple fix for an ntfs3 bug that was introduced by some refactoring we did in mainline only. Guenter reported it and both he and I verified that this patch fixes the issue. I can also resend it as a PR tomorrow if you prefer. Thanks! Christian --- fs/ntfs3/super.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) --- base-commit: 7ba2090ca64ea1aa435744884124387db1fac70f change-id: 20230907-vfs-ntfs3-kill_sb-52bb6f5230d4