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

[Gao Xiang]

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;  ?

>   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.

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