On Tue, Sep 26, 2023 at 10:25:15PM +0100, Al Viro wrote: > Before your patch: foo_kill_super() calls kill_anon_super(), > which calls kill_super_notify(), which removes the sucker from > the list, then frees ->s_fs_info. After your patch: > removal from the lists happens via the call of kill_super_notify() > *after* both of your methods had been called, while freeing > ->s_fs_info happens from the method call. IOW, you've restored > the situation prior to "super: ensure valid info". The whole > point of that commit had been to make sure that we have nothing > in the lists with ->s_fs_info pointing to a freed object. More detailed example: take a look at NFS. We have ->get_tree() there call sget_fc() with nfs_compare_super() as possible 'test' callback. It does look at ->s_fs_info of the superblocks found on the list of instances for fs type in question. Moreover, it proceeds to call nfs_compare_mount_options(), which chases pointers from that (at the very least fetch ->client in nfs_server instance ->s_fs_info points to and dereferences that). We really, really do not want nfs_free_server() happen while the superblock is visible in the instances list. Now, in your tree nfs_free_sb() call nfs_free_server(). *Without* having called kill_super_notify() first - you do that only after the call of ->free_sb(). So with this series applied we have UAF on race between mount and umount. For NFS. No block devices involved. Old logics had been "after generic_shutdown_super() the private parts of superblock belong to filesystem alone; they might be accessed by methods called from RCU pathwalk, but that's it". I still don't see any clear rules for the new one. And the more I'm looking, the more sceptical I get about the approach you've taken, TBH...
