On Thu, Apr 06, 2023 at 06:55:40PM +0800, Gao Xiang wrote: > > > On 2023/4/6 18:27, Greg KH wrote: > > On Thu, Apr 06, 2023 at 06:13:05PM +0800, Gao Xiang wrote: > > > Hi Greg, > > > > > > On 2023/4/6 18:03, Greg KH wrote: > > > > On Thu, Apr 06, 2023 at 05:30:55PM +0800, Yangtao Li wrote: > > > > > Use kobject_is_added() instead of directly accessing the internal > > > > > variables of kobject. BTW kill kobject_del() directly, because > > > > > kobject_put() actually covers kobject removal automatically. > > > > > > > > > > Signed-off-by: Yangtao Li <[email protected]> > > > > > --- > > > > > fs/erofs/sysfs.c | 3 +-- > > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > > > diff --git a/fs/erofs/sysfs.c b/fs/erofs/sysfs.c > > > > > index 435e515c0792..daac23e32026 100644 > > > > > --- a/fs/erofs/sysfs.c > > > > > +++ b/fs/erofs/sysfs.c > > > > > @@ -240,8 +240,7 @@ void erofs_unregister_sysfs(struct super_block > > > > > *sb) > > > > > { > > > > > struct erofs_sb_info *sbi = EROFS_SB(sb); > > > > > - if (sbi->s_kobj.state_in_sysfs) { > > > > > - kobject_del(&sbi->s_kobj); > > > > > + if (kobject_is_added(&sbi->s_kobj)) { > > > > > > > > I do not understand why this check is even needed, I do not think it > > > > should be there at all as obviously the kobject was registered if it now > > > > needs to not be registered. > > > > > > I think Yangtao sent a new patchset which missed the whole previous > > > background discussions as below: > > > https://lore.kernel.org/r/[email protected] > > > > > > It's needed because once a syzbot complaint as below: > > > https://lore.kernel.org/r/CAD-N9QXNx=p3-qowzk6pcznf32czy8km3vvo8mamfzz9cpu...@mail.gmail.com > > > > > > I'd suggest including the previous backgrounds at least in the newer > > > patchset, > > > otherwise it makes me explain again and again... > > > > That would be good, as I do not think this is correct, it should be > > fixed in a different way, see my response to the zonefs patch in this > > series as a much simpler method to use. > > Yes, but here (sbi->s_kobj) is not a kobject pointer (also at a quick > glance it seems that zonefs has similar code), and also we couldn't > just check the sbi is NULL or not here only, since sbi is already > non-NULL in this path and there are some others in sbi to free in > other functions. > > s_kobj could be changed into a pointer if needed. I'm all fine with > either way since as you said, it's a boilerplate filesystem kobject > logic duplicated from somewhere. Hopefully Yangtao could help take > this task since he sent me patches about this multiple times.
I made the same mistake with the zonefs code. If the kobject in this structure controls the lifespan of it (which makes it not a pointer, my mistake), then that whole memory chunk can't be valid anymore if the kobject registering function failed so you need to get rid of it then, not later. thanks, greg k-h
