How to fix the bug in "WARNING: kobject bug in erofs_unregister_sysfs"?

[Dongliang Mu]

On Thu, Mar 10, 2022 at 10:04 PM Dongliang Mu <mudongliangabcd at gmail.com> wrote:
>
> On Thu, Mar 10, 2022 at 8:39 PM Gao Xiang <hsiangkao at linux.alibaba.com> wrote:
> >
> > Hi Dongliang,
> >
> > (+ cc Jianan.)
> >
> > On Thu, Mar 10, 2022 at 06:15:20PM +0800, Dongliang Mu wrote:
> > > Hi kernel developers,
> > >
> > > I am writing to kindly ask for some suggestions on fixing "WARNING:
> > > kobject bug in erofs_unregister_sysfs".
> > >
> > > The underlying issue is in the following,
> > >
> > > erofs_fc_get_tree
> > > -> get_tree_bdev
> > >   -> fill_super
> > >     -> erofs_fc_fill_super
> > >
> >
> > Thanks for the report!
> >
> > > When erofs_register_sysfs fails in the calling kobject_init_and_add,
> > > it just returned an error code and the parent function will call
> > > deactivate_locked_super to do clean up.
> >
> > Yes, in that way we don't need to rewrite another error path (actually
> > once we had another duplicated error path but Al suggested the current
> > shape...)
> >
> > >
> > > In the following stack trace, it finally calls erofs_unregister_sysfs
> > > without knowing the execution status of erofs_register_sysfs, which
> > > leads to the kobject bug.
> > >
> > >  erofs_unregister_sysfs+0x46/0x60 fs/erofs/sysfs.c:225
> > >  erofs_put_super+0x37/0xb0 fs/erofs/super.c:771
> > >  generic_shutdown_super+0x14c/0x400 fs/super.c:465
> > >  kill_block_super+0x97/0xf0 fs/super.c:1397
> > >  erofs_kill_sb+0x60/0x190 fs/erofs/super.c:752
> > >  deactivate_locked_super+0x94/0x160 fs/super.c:335
> > >  get_tree_bdev+0x573/0x760 fs/super.c:1297
> > >
> > > I am not sure how to fix this bug. Any suggestion is appreciated.
> >
> > I think a simple way is to introduce a `sysfs_inited' boolean to
> > sbi to indicate that. Or some better suggestion is welcomed.
>
> Yes, it's an idea to use a boolean to indicate the execution status.
> Let me first try to craft a drafted patch and test it on Syzbot.

I've drafted one patch and sent it here.

diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h
index 5aa2cf2c2f80..ba2db2e9e3b7 100644
--- a/fs/erofs/internal.h
+++ b/fs/erofs/internal.h
@@ -144,6 +144,7 @@ struct erofs_sb_info {
  u32 feature_incompat;

  /* sysfs support */
+ bool sysfs_inited;
  struct kobject s_kobj; /* /sys/fs/erofs/<devname> */
  struct completion s_kobj_unregister;
 };
diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c
index dac252bc9228..795273c15c42 100644
--- a/fs/erofs/sysfs.c
+++ b/fs/erofs/sysfs.c
@@ -209,6 +209,7 @@ int erofs_register_sysfs(struct super_block *sb)
     "%s", sb->s_id);
  if (err)
  goto put_sb_kobj;
+ sbi->sysfs_inited = true;
  return 0;

 put_sb_kobj:
@@ -221,8 +222,11 @@ void erofs_unregister_sysfs(struct super_block *sb)
 {
  struct erofs_sb_info *sbi = EROFS_SB(sb);

- kobject_del(&sbi->s_kobj);
- kobject_put(&sbi->s_kobj);
+ if (sbi->sysfs_inited) {
+ kobject_del(&sbi->s_kobj);
+ kobject_put(&sbi->s_kobj);
+ sbi->sysfs_inited = false;
+ }
  wait_for_completion(&sbi->s_kobj_unregister);
 }

The compilation is fine. However, since the crash report does not have
any reproducer, I cannot test it in Syzbot or my local instance.

>
> >
> > Thanks,
> > Gao Xiang
> >
> > >
> > > --
> > > My best regards to you.
> > >
> > >      No System Is Safe!
> > >      Dongliang Mu