On Fri 09-10-26 23:51:50, Christian Brauner wrote:
> While working on eliding the grace period in namespace_unlock() I went
> through what filesystems free when they're shut down and what a pathwalk
> in rcu mode can still read at that point. Since 66a97c2ec953's rcu
> pathwalk fixes that's the filesystem's job. Whatever ->permission(),
> ->d_revalidate(), ->get_link(), ->d_hash() and ->d_compare() look at in
> rcu mode has to be freed after a grace period. A few filesystems don't
> do that.
>
> Most of it is hidden by umount(2). namespace_unlock() waits for a grace
> period before it drops the mounts, so a walker that was inside the
> filesystem is gone by the time the filesystem is shut down.
>
> But the last reference to a mount doesn't have to go away in umount(2).
> A chdir() out of a lazily unmounted directory or the close() of the last
> file on it drops it without a grace period. A walker can then still be
> in the filesystem while ->put_super() and ->kill_sb() free what it
> reads.
Good catches! But I find it very counter-intuitive that pathname lookups
can still be running against a filesystem while its ->put_super is called.
People will keep forgetting about that and as a result these UAF bugs will
be introduced again. Isn't it feasible we'd guarantee to the filesystem
->put_super() gets called only after all lookups are done (i.e., after RCU
grace period expires)?
I understand this is a hairy bussiness because this has userspace visible
implications (like when filesystem's sysfs files disappear, when the bdev
really gets unused, etc.) but keeping all filesystems safe to run RCU
lookup after ->put_super() will be IMHO a headache.
Honza
>
> With KASAN on vfs.fixes:
>
> - ext4_get_link() reads sb->s_fs_info and the extent status counters in
> rcu mode for a symlink that keeps its target in a data block.
>
> ext4_put_super() destroys the counters, clears s_fs_info and frees the
> sbi right away. That's a NULL dereference, a use-after-free of the sbi
> or a fault on a destroyed percpu counter, depending on where the
> walker sits.
>
> - The unicode map of casefolded ext4, f2fs and tmpfs is freed from
> ->put_super() while generic_ci_d_hash() and generic_ci_d_compare() use
> it.
>
> - ntfs3 with nocase frees its sbi and mount options under ntfs_d_hash()
> and ntfs_d_compare(). A remount frees the old mount options right away
> too, that one needs no unmount at all.
>
> - eventfs frees an eventfs_inode after an SRCU grace period only, but
> tracefs_d_revalidate() reads it under rcu_read_lock(). No unmount
> needed either.
>
> The ipc patch is a different kind of bug. create_ipc_ns() leaks the
> mqueue mount and its superblock when it fails after mq_init_ns(). I ran
> into it while looking at the kern_unmount() users.
>
> afs and 9p look just as broken but nothing can reach it today. Both
> have a private bdi, and generic_shutdown_super() calls bdi_unregister(),
> which waits for a grace period before ->kill_sb() frees their private
> data. That grace period is there for the rcu readers of bdi_list and
> covers afs and 9p by accident. I don't want them to depend on that, so
> they free their private data after a grace period of their own as well.
>
> I appended the reproducers. The race reproducers hold a pathwalk inside
> the filesystem's callback for two seconds with a kprobe while the other
> side frees what it reads. For the ntfs3 remount the window is a single
> load and its dereference, so that one needs an mdelay() in the kernel.
> ext4 with a journal needs a kernel without lockdep since
> jbd2_journal_destroy() calls lockdep_unregister_key(), which waits for a
> grace period too.
>
> Clean this all up.
>
> Signed-off-by: Christian Brauner (Amutable) <[email protected]>
> ---
> Christian Brauner (8):
> ext4: free the sbi after a grace period
> afs: free the super info after a grace period
> 9p: free the session after a grace period
> eventfs: free the eventfs_inode after an rcu grace period too
> ipc: drop the mqueue mount with kern_unmount() on setup failure
> unicode: free the map after a grace period
> ntfs3: free the sbi after a grace period
> ntfs3: free replaced mount options after a grace period
>
> fs/9p/v9fs.h | 1 +
> fs/9p/vfs_super.c | 4 ++--
> fs/afs/internal.h | 2 ++
> fs/afs/super.c | 2 +-
> fs/ext4/ext4.h | 2 ++
> fs/ext4/extents-test.c | 2 ++
> fs/ext4/extents.c | 1 +
> fs/ext4/extents_status.c | 7 ++++++-
> fs/ext4/extents_status.h | 1 +
> fs/ext4/super.c | 13 +++++++++++--
> fs/ntfs3/dir.c | 13 +++++++++++--
> fs/ntfs3/ntfs_fs.h | 3 +++
> fs/ntfs3/super.c | 49
> +++++++++++++++++++++++++++++++++++-------------
> fs/tracefs/event_inode.c | 10 +++++++++-
> fs/unicode/utf8-core.c | 21 +++++++++++++++++----
> include/linux/unicode.h | 1 +
> ipc/namespace.c | 2 +-
> 17 files changed, 107 insertions(+), 27 deletions(-)
> ---
> base-commit: 3d399224425573875b6f6f1181bd8cf9a28cc7d1
> change-id: 20261009-work-fixes-rcu-cd9c1eec0769
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
