sb->s_fs_info freeing fixes

sb->s_fs_info freeing fixes

[Christoph Hellwig]

sb->s_fs_info should only be freed after the superblock has been marked
inactive in generic_shutdown_super, which means either in ->put_super or
in ->kill_sb after generic_shutdown_super has returned.  Fix the
instances where that is not the case.

Diffstat
 arch/s390/hypfs/inode.c      |    3 +--
 fs/devpts/inode.c            |    2 +-
 fs/ramfs/inode.c             |    2 +-
 security/selinux/selinuxfs.c |    5 +----
 4 files changed, 4 insertions(+), 8 deletions(-)

[PATCH 1/4] ramfs: free sb->s_fs_info after shutting down the super block

[Christoph Hellwig]

sb->s_fs_info can only be safely freed after generic_shutdown_super was
called and all access to the super_block has stopped.

Thus only free the private data after calling kill_litter_super, which
calls generic_shutdown_super internally.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/ramfs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
index 18e8387cab4148..0f37ecbae59dad 100644
--- a/fs/ramfs/inode.c
+++ b/fs/ramfs/inode.c
@@ -280,8 +280,8 @@ int ramfs_init_fs_context(struct fs_context *fc)
 
 void ramfs_kill_sb(struct super_block *sb)
 {
-	kfree(sb->s_fs_info);
 	kill_litter_super(sb);
+	kfree(sb->s_fs_info);
 }
 
 static struct file_system_type ramfs_fs_type = {
-- 
2.39.2

Re: [PATCH 1/4] ramfs: free sb->s_fs_info after shutting down the super block

[Al Viro]

On Thu, Aug 31, 2023 at 07:31:54AM +0200, Christoph Hellwig wrote:
> sb->s_fs_info can only be safely freed after generic_shutdown_super was
> called and all access to the super_block has stopped.
> 
> Thus only free the private data after calling kill_litter_super, which
> calls generic_shutdown_super internally.

Take a look at what ramfs uses that thing for.  Remount and ->show_options().
Neither is an issue in ->kill_sb().  I don't really hate that patch, but
commit message is flat-out incorrect in this case.

[PATCH 2/4] devpts: free sb->s_fs_info after shutting down the super block

[Christoph Hellwig]

sb->s_fs_info can only be safely freed after generic_shutdown_super was
called and all access to the super_block has stopped.

Thus only free the private data after calling kill_litter_super, which
calls generic_shutdown_super internally.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/devpts/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 299c295a27a03e..d46cea36c026ad 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -491,10 +491,10 @@ static void devpts_kill_sb(struct super_block *sb)
 {
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 
+	kill_litter_super(sb);
 	if (fsi)
 		ida_destroy(&fsi->allocated_ptys);
 	kfree(fsi);
-	kill_litter_super(sb);
 }
 
 static struct file_system_type devpts_fs_type = {
-- 
2.39.2

Re: [PATCH 2/4] devpts: free sb->s_fs_info after shutting down the super block

[Al Viro]

On Thu, Aug 31, 2023 at 07:31:55AM +0200, Christoph Hellwig wrote:
> sb->s_fs_info can only be safely freed after generic_shutdown_super was
> called and all access to the super_block has stopped.

Similar to ramfs case; nothing in dentry/inode eviction codepaths is
using anything in ->s_fs_info.  And references in tty are holding
an active ref to superblock in question, preventing ->kill_sb(), so
nothing from the outside is going to play with e.g. devpts_{new,kill}_index()
under us.

[PATCH 3/4] selinuxfs: free sb->s_fs_info after shutting down the super block

[Christoph Hellwig]

sb->s_fs_info can only be safely freed after generic_shutdown_super was
called and all access to the super_block has stopped.

Thus only free the private data after calling kill_litter_super, which
calls generic_shutdown_super internally.

Also remove the duplicate freeing in the sel_fill_super error path given
that ->kіll_sb is also called on ->fill_super failure.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 security/selinux/selinuxfs.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 9dafb6ff110d26..8a8a532be8e767 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -2100,9 +2100,6 @@ static int sel_fill_super(struct super_block *sb, struct fs_context *fc)
 err:
 	pr_err("SELinux: %s:  failed while creating inodes\n",
 		__func__);
-
-	selinux_fs_info_free(sb);
-
 	return ret;
 }
 
@@ -2123,8 +2120,8 @@ static int sel_init_fs_context(struct fs_context *fc)
 
 static void sel_kill_sb(struct super_block *sb)
 {
-	selinux_fs_info_free(sb);
 	kill_litter_super(sb);
+	selinux_fs_info_free(sb);
 }
 
 static struct file_system_type sel_fs_type = {
-- 
2.39.2

Re: [PATCH 3/4] selinuxfs: free sb->s_fs_info after shutting down the super block

[Al Viro]

On Thu, Aug 31, 2023 at 07:31:56AM +0200, Christoph Hellwig wrote:
> sb->s_fs_info can only be safely freed after generic_shutdown_super was
> called and all access to the super_block has stopped.
> 
> Thus only free the private data after calling kill_litter_super, which
> calls generic_shutdown_super internally.

Same as for ramfs, AFAICS.

> Also remove the duplicate freeing in the sel_fill_super error path given
> that ->kіll_sb is also called on ->fill_super failure.

Reasonable cleanup, that.

[PATCH 4/4] hypfs: free sb->s_fs_info after shutting down the super block

[Christoph Hellwig]

sb->s_fs_info can only be safely freed after generic_shutdown_super was
called and all access to the super_block has stopped.

Thus only free the private data after calling kill_litter_super, which
calls generic_shutdown_super internally.

Also remove the pointless clearing of sb->s_fs_info as the super_block
can't be accessed at this point and will be freed immediately.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/s390/hypfs/inode.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/s390/hypfs/inode.c b/arch/s390/hypfs/inode.c
index ada83149932fec..dbe8a7dcafa922 100644
--- a/arch/s390/hypfs/inode.c
+++ b/arch/s390/hypfs/inode.c
@@ -329,9 +329,8 @@ static void hypfs_kill_super(struct super_block *sb)
 		hypfs_delete_tree(sb->s_root);
 	if (sb_info && sb_info->update_file)
 		hypfs_remove(sb_info->update_file);
-	kfree(sb->s_fs_info);
-	sb->s_fs_info = NULL;
 	kill_litter_super(sb);
+	kfree(sb->s_fs_info);
 }
 
 static struct dentry *hypfs_create_file(struct dentry *parent, const char *name,
-- 
2.39.2

Re: [PATCH 4/4] hypfs: free sb->s_fs_info after shutting down the super block

[Al Viro]

On Thu, Aug 31, 2023 at 07:31:57AM +0200, Christoph Hellwig wrote:
> sb->s_fs_info can only be safely freed after generic_shutdown_super was
> called and all access to the super_block has stopped.
> 
> Thus only free the private data after calling kill_litter_super, which
> calls generic_shutdown_super internally.

Same as for ramfs.  We *do* use ->s_fs_info there, but only for
operations that require an active reference.  They can't overlap
with ->kill_sb().

Re: sb->s_fs_info freeing fixes

[Christian Brauner]

On Thu, Aug 31, 2023 at 07:31:53AM +0200, Christoph Hellwig wrote:
> sb->s_fs_info should only be freed after the superblock has been marked
> inactive in generic_shutdown_super, which means either in ->put_super or
> in ->kill_sb after generic_shutdown_super has returned.  Fix the
> instances where that is not the case.

Funny, I had looked at all those filesystems a while back to
double-check that things are sane and that's why I didn't change them.

>  arch/s390/hypfs/inode.c      |    3 +--

get_tree_single() -> single instance
=> doesn't match on sb->s_fs_info

>  fs/devpts/inode.c            |    2 +-

get_tree_nodev() -> each mount is a new instance
=> We don't match on sb->s_fs_info

>  fs/ramfs/inode.c             |    2 +-

get_tree_nodev() -> each mount is a new instance
=> We don't match on sb->s_fs_info

>  security/selinux/selinuxfs.c |    5 +----

get_tree_single() -> single instance
=> doesn't match on sb->s_fs_info

Al roughly said the same thing afaict.

I still think that there's no need to deviate from the basic logic:

(1) call generic kill_*() helper
(2) wipe sb->s_fs_info

So I think that's a cleanup we should do. Just change the rationale to
say that this deviation is pointless and just means the reader of the
code has to sanity check against the superblock helper that's used.

This btw, has confused me before too and I basically had a version of
the exchange we're now having:

https://lore.kernel.org/all/YvzUS%2F7bd1mm6c%2FV@kroah.com/T/#mee634e46448d6c88244c0c5d33f935ffdb60cb12

Re: sb->s_fs_info freeing fixes

[Christian Brauner]

On Thu, Aug 31, 2023 at 12:20:32PM +0200, Christian Brauner wrote:
> On Thu, Aug 31, 2023 at 07:31:53AM +0200, Christoph Hellwig wrote:
> > sb->s_fs_info should only be freed after the superblock has been marked
> > inactive in generic_shutdown_super, which means either in ->put_super or
> > in ->kill_sb after generic_shutdown_super has returned.  Fix the
> > instances where that is not the case.
> 
> Funny, I had looked at all those filesystems a while back to
> double-check that things are sane and that's why I didn't change them.
> 
> >  arch/s390/hypfs/inode.c      |    3 +--
> 
> get_tree_single() -> single instance
> => doesn't match on sb->s_fs_info
> 
> >  fs/devpts/inode.c            |    2 +-
> 
> get_tree_nodev() -> each mount is a new instance
> => We don't match on sb->s_fs_info
> 
> >  fs/ramfs/inode.c             |    2 +-
> 
> get_tree_nodev() -> each mount is a new instance
> => We don't match on sb->s_fs_info
> 
> >  security/selinux/selinuxfs.c |    5 +----
> 
> get_tree_single() -> single instance
> => doesn't match on sb->s_fs_info
> 
> Al roughly said the same thing afaict.
> 
> I still think that there's no need to deviate from the basic logic:
> 
> (1) call generic kill_*() helper
> (2) wipe sb->s_fs_info
> 
> So I think that's a cleanup we should do. Just change the rationale to
> say that this deviation is pointless and just means the reader of the
> code has to sanity check against the superblock helper that's used.

I changed the commit messages to say:

"Since ramfs/devpts uses get_tree_nodev() it doesn't rely on
sb->s_fs_info. So there's no use after free risk as with other
filesystems.

But there's no need to deviate from the standard cleanup logic and cause
reviewers to verify whether that is safe or not."

and similar for the other two:

"Since hypfs/selinuxfs uses get_tree_single() it doesn't rely on
sb->s_fs_info. So there's no use after free risk as with other
filesystems.

But there's no need to deviate from the standard cleanup logic and cause
reviewers to verify whether that is safe or not."

If that is good enough for people then I can grab it.

Re: sb->s_fs_info freeing fixes

[Christoph Hellwig]

On Thu, Aug 31, 2023 at 12:29:11PM +0200, Christian Brauner wrote:
> "Since ramfs/devpts uses get_tree_nodev() it doesn't rely on
> sb->s_fs_info. So there's no use after free risk as with other
> filesystems.
> 
> But there's no need to deviate from the standard cleanup logic and cause
> reviewers to verify whether that is safe or not."
> 
> and similar for the other two:
> 
> "Since hypfs/selinuxfs uses get_tree_single() it doesn't rely on
> sb->s_fs_info. So there's no use after free risk as with other
> filesystems.
> 
> But there's no need to deviate from the standard cleanup logic and cause
> reviewers to verify whether that is safe or not."
> 
> If that is good enough for people then I can grab it.

Fine with me.  And yes, I'd rather not have private data freed before
SB_ACTIVE is cleared even if it is fine right now.  It's just a bug
waiting to happen.

Re: sb->s_fs_info freeing fixes

[Christian Brauner]

> > If that is good enough for people then I can grab it.
> 
> Fine with me.  And yes, I'd rather not have private data freed before
> SB_ACTIVE is cleared even if it is fine right now.  It's just a bug
> waiting to happen.

Applied to the vfs.super branch of the vfs/vfs.git tree.
Patches in the vfs.super branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.super

[1/4] ramfs: free sb->s_fs_info after shutting down the super block
      https://git.kernel.org/vfs/vfs/c/c5725dff056d
[2/4] devpts: free sb->s_fs_info after shutting down the super block
      https://git.kernel.org/vfs/vfs/c/fee7516be512
[3/4] selinuxfs: free sb->s_fs_info after shutting down the super block
      https://git.kernel.org/vfs/vfs/c/3105b94e7d62
[4/4] hypfs: free sb->s_fs_info after shutting down the super block
      https://git.kernel.org/vfs/vfs/c/993d214eb394

Re: sb->s_fs_info freeing fixes

[Paul Moore]

On Thu, Aug 31, 2023 at 9:11 AM Christian Brauner <brauner@kernel.org> wrote:
> > > If that is good enough for people then I can grab it.
> >
> > Fine with me.  And yes, I'd rather not have private data freed before
> > SB_ACTIVE is cleared even if it is fine right now.  It's just a bug
> > waiting to happen.
>
> Applied to the vfs.super branch of the vfs/vfs.git tree.
> Patches in the vfs.super branch should appear in linux-next soon.
>
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
>
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
>
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs.super
>
> [1/4] ramfs: free sb->s_fs_info after shutting down the super block
>       https://git.kernel.org/vfs/vfs/c/c5725dff056d
> [2/4] devpts: free sb->s_fs_info after shutting down the super block
>       https://git.kernel.org/vfs/vfs/c/fee7516be512
> [3/4] selinuxfs: free sb->s_fs_info after shutting down the super block
>       https://git.kernel.org/vfs/vfs/c/3105b94e7d62
> [4/4] hypfs: free sb->s_fs_info after shutting down the super block
>       https://git.kernel.org/vfs/vfs/c/993d214eb394

No need to change anything in this case, but in the future if there
are no patch dependency or ordering issues can you let me take the
SELinux patches via the SELinux tree?  It helps prevent merge
conflicts during the next merge window and quiets the daily automated
checks I have in place to detect SELinux changes outside of the
SELinux tree.

Thanks.

-- 
paul-moore.com

Re: sb->s_fs_info freeing fixes

[Christoph Hellwig]

On Thu, Aug 31, 2023 at 10:38:09AM -0400, Paul Moore wrote:
> No need to change anything in this case, but in the future if there
> are no patch dependency or ordering issues can you let me take the
> SELinux patches via the SELinux tree?  It helps prevent merge
> conflicts during the next merge window and quiets the daily automated
> checks I have in place to detect SELinux changes outside of the
> SELinux tree.

Even if this goes into the next merge window we'd need it in the vfs
tree as it is preparation for other work on VFS interfaces.

Re: sb->s_fs_info freeing fixes

[Paul Moore]

On Thu, Aug 31, 2023 at 10:39 AM Christoph Hellwig <hch@lst.de> wrote:
> On Thu, Aug 31, 2023 at 10:38:09AM -0400, Paul Moore wrote:
> > No need to change anything in this case, but in the future if there
> > are no patch dependency or ordering issues can you let me take the
> > SELinux patches via the SELinux tree?  It helps prevent merge
> > conflicts during the next merge window and quiets the daily automated
> > checks I have in place to detect SELinux changes outside of the
> > SELinux tree.
>
> Even if this goes into the next merge window we'd need it in the vfs
> tree as it is preparation for other work on VFS interfaces.

That's fine, I would consider that part of the "patch dependency" case
I mentioned above.

My motivation for commenting on this is that I've seen a few recent
cases where some other subsystems have been quick to merge SELinux,
etc. patches and I just wanted to ask that some consideration be made
as it causes us (me) some level of pain.  If there is a good reason
for it, that's fine, being a maintainer is a life of pain ;) but if it
is simply to just get the patch merged into an upstream tree a day or
two earlier I would please ask you to exercise patience and let it be
merged via the appropriate tree.

-- 
paul-moore.com