Re: [PATCH] fs: fix s_fs_info leak when setup_bdev_super() fails

[hanzhijian]

Hi Al and Christian,

syzbot reported a memory leak in adfs [1], which I traced to the
interaction between sget_fc() and setup_bdev_super().  My first attempt
at a fix (this patch) turned out to be wrong and I've retracted it, so
I'd like to understand the intended way to fix this.

The leak path is:

  adfs_init_fs_context() allocates a struct adfs_sb_info and stores it
  in fc->s_fs_info (fs/adfs/super.c:443,463).

  sget_fc() transfers fc->s_fs_info to s->s_fs_info and clears
  fc->s_fs_info once set() succeeds (fs/super.c:895,905).

  get_tree_bdev_flags() then calls setup_bdev_super().  If that fails
  (the block device cannot be opened, is read-only, or is frozen),
  fill_super() never runs, s->s_root stays NULL, and the error path
  calls deactivate_locked_super() -> kill_block_super() ->
  generic_shutdown_super().  Since generic_shutdown_super() only calls
  put_super() inside its "if (s->s_root)" block, the adfs_sb_info
  allocated by init_fs_context() is never released.

My patch tried to hand s_fs_info back to the fs_context:

        fc->s_fs_info = s->s_fs_info;
        s->s_fs_info = NULL;
        deactivate_locked_super(s);

but syzbot's CI then reported general protection faults in
erofs_kill_sb(), fuse_kill_sb_blk() and xfs_kill_sb() [2], because those
filesystems dereference s->s_fs_info in their custom kill_sb() handlers.

I notice sget_fc()'s own set()-failure path does the same "hand back to
fc" (s->s_fs_info = NULL at fs/super.c:899) but pairs it with
destroy_unused_super(), which does not call kill_sb(), whereas after
sget_fc() returns the superblock is already published and must be torn
down with deactivate_locked_super().  What is the intended way to
release s->s_fs_info in this situation?

Thanks,
Han Zhijian

[1] https://syzkaller.appspot.com/bug?extid=1c70732df5fd4f0e4fbb
[2] https://ci.syzbot.org/series/1ef55243-f163-4d98-bbd1-41b579082704