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

[Dongliang Mu]

On Fri, Mar 11, 2022 at 12:56 PM Gao Xiang <hsiangkao at linux.alibaba.com> wrote:
>
> On Fri, Mar 11, 2022 at 12:08:43PM +0800, Dongliang Mu wrote:
> > 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;
>
> bool s_sysfs_inited;  ?

I agree with this name.

>
> >   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 for the patch. I think you could do some fault injection (e.g.
> return err forcely in erofs_register_sysfs() )  to verify that.

Let me try this since I already observed the fault injection in the log file.

>
> If possible, would you mind submitting a formal patch then?
>
> Thanks,
> Gao Xiang
>
>
> > >
> > > >
> > > > Thanks,
> > > > Gao Xiang
> > > >
> > > > >
> > > > > --
> > > > > My best regards to you.
> > > > >
> > > > >      No System Is Safe!
> > > > >      Dongliang Mu