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.
