[3/3] zonefs: convert to use kobject_is_added()

  Use kobject_is_added() instead of local `s_sysfs_registered` variables.
BTW kill kobject_del() directly, because kobject_put() actually covers
kobject removal automatically.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/zonefs/sysfs.c  | 11 +++++------
 fs/zonefs/zonefs.h |  1 -
 2 files changed, 5 insertions(+), 7 deletions(-)
  

On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
> BTW kill kobject_del() directly, because kobject_put() actually covers
> kobject removal automatically.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  fs/zonefs/sysfs.c  | 11 +++++------
>  fs/zonefs/zonefs.h |  1 -
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
> index 8ccb65c2b419..f0783bf7a25c 100644
> --- a/fs/zonefs/sysfs.c
> +++ b/fs/zonefs/sysfs.c
> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
>  		return ret;
>  	}
>  
> -	sbi->s_sysfs_registered = true;

You know this, why do you need to have a variable tell you this or not?

> -
>  	return 0;
>  }
>  
> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
>  {
>  	struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
>  
> -	if (!sbi || !sbi->s_sysfs_registered)

How can either of these ever be true?  Note, sbi should be passed here
to this function, not the super block as that is now unregistered from
the system.  Looks like no one has really tested this codepath that much
:(

> +	if (!sbi)
>  		return;

this can not ever be true, right?


>  
> -	kobject_del(&sbi->s_kobj);
> -	kobject_put(&sbi->s_kobj);
> -	wait_for_completion(&sbi->s_kobj_unregister);
> +	if (kobject_is_added(&sbi->s_kobj)) {

Again, not needed.

thanks,

greg k-h
  

On 4/6/23 19:05, Greg KH wrote:
> On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
>> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
>> BTW kill kobject_del() directly, because kobject_put() actually covers
>> kobject removal automatically.
>>
>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>> ---
>>  fs/zonefs/sysfs.c  | 11 +++++------
>>  fs/zonefs/zonefs.h |  1 -
>>  2 files changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
>> index 8ccb65c2b419..f0783bf7a25c 100644
>> --- a/fs/zonefs/sysfs.c
>> +++ b/fs/zonefs/sysfs.c
>> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
>>  		return ret;
>>  	}
>>  
>> -	sbi->s_sysfs_registered = true;
> 
> You know this, why do you need to have a variable tell you this or not?

If kobject_init_and_add() fails, zonefs_sysfs_register() returns an error and
fill_super will also return that error. vfs will then call kill_super, which
calls zonefs_sysfs_unregister(). For that case, we need to know that we actually
added the kobj.

> 
>> -
>>  	return 0;
>>  }
>>  
>> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
>>  {
>>  	struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
>>  
>> -	if (!sbi || !sbi->s_sysfs_registered)
> 
> How can either of these ever be true?  Note, sbi should be passed here
> to this function, not the super block as that is now unregistered from
> the system.  Looks like no one has really tested this codepath that much
> :(
> 
>> +	if (!sbi)
>>  		return;
> 
> this can not ever be true, right?

Yes it can, if someone attempt to mount a non zoned device. In that case,
fill_super returns early without setting sb->s_fs_info but vfs still calls
kill_super.

> 
> 
>>  
>> -	kobject_del(&sbi->s_kobj);
>> -	kobject_put(&sbi->s_kobj);
>> -	wait_for_completion(&sbi->s_kobj_unregister);
>> +	if (kobject_is_added(&sbi->s_kobj)) {
> 
> Again, not needed.
> 
> thanks,
> 
> greg k-h
  

On Thu, Apr 06, 2023 at 07:13:38PM +0900, Damien Le Moal wrote:
> On 4/6/23 19:05, Greg KH wrote:
> > On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
> >> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
> >> BTW kill kobject_del() directly, because kobject_put() actually covers
> >> kobject removal automatically.
> >>
> >> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> >> ---
> >>  fs/zonefs/sysfs.c  | 11 +++++------
> >>  fs/zonefs/zonefs.h |  1 -
> >>  2 files changed, 5 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
> >> index 8ccb65c2b419..f0783bf7a25c 100644
> >> --- a/fs/zonefs/sysfs.c
> >> +++ b/fs/zonefs/sysfs.c
> >> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
> >>  		return ret;
> >>  	}
> >>  
> >> -	sbi->s_sysfs_registered = true;
> > 
> > You know this, why do you need to have a variable tell you this or not?
> 
> If kobject_init_and_add() fails, zonefs_sysfs_register() returns an error and
> fill_super will also return that error. vfs will then call kill_super, which
> calls zonefs_sysfs_unregister(). For that case, we need to know that we actually
> added the kobj.

Ok, but then why not just 0 out the kobject pointer here instead?  That
way you will always know if it's a valid pointer or not and you don't
have to rely on some other variable?  Use the one that you have already :)

And you really don't even need to check anything, just pass in NULL to
kobject_del() and friends, it should handle it.

> >> -
> >>  	return 0;
> >>  }
> >>  
> >> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
> >>  {
> >>  	struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
> >>  
> >> -	if (!sbi || !sbi->s_sysfs_registered)
> > 
> > How can either of these ever be true?  Note, sbi should be passed here
> > to this function, not the super block as that is now unregistered from
> > the system.  Looks like no one has really tested this codepath that much
> > :(
> > 
> >> +	if (!sbi)
> >>  		return;
> > 
> > this can not ever be true, right?
> 
> Yes it can, if someone attempt to mount a non zoned device. In that case,
> fill_super returns early without setting sb->s_fs_info but vfs still calls
> kill_super.

But you already had a sbi pointer in the place that this was called, so
you "know" if you need to even call into here or not.  You are having to
look up the same pointer multiple times in this call chain, there's no
need for that.

thanks,

greg k-h
  

On 4/6/23 19:26, Greg KH wrote:
> On Thu, Apr 06, 2023 at 07:13:38PM +0900, Damien Le Moal wrote:
>> On 4/6/23 19:05, Greg KH wrote:
>>> On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
>>>> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
>>>> BTW kill kobject_del() directly, because kobject_put() actually covers
>>>> kobject removal automatically.
>>>>
>>>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>>>> ---
>>>>  fs/zonefs/sysfs.c  | 11 +++++------
>>>>  fs/zonefs/zonefs.h |  1 -
>>>>  2 files changed, 5 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
>>>> index 8ccb65c2b419..f0783bf7a25c 100644
>>>> --- a/fs/zonefs/sysfs.c
>>>> +++ b/fs/zonefs/sysfs.c
>>>> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> -	sbi->s_sysfs_registered = true;
>>>
>>> You know this, why do you need to have a variable tell you this or not?
>>
>> If kobject_init_and_add() fails, zonefs_sysfs_register() returns an error and
>> fill_super will also return that error. vfs will then call kill_super, which
>> calls zonefs_sysfs_unregister(). For that case, we need to know that we actually
>> added the kobj.
> 
> Ok, but then why not just 0 out the kobject pointer here instead?  That
> way you will always know if it's a valid pointer or not and you don't
> have to rely on some other variable?  Use the one that you have already :)

but sbi->s_kobj is the kobject itself, not a pointer. I can still zero it out in
case of error to avoid using the added s_sysfs_registered bool. I would need to
check a field of s_kobj though, which is not super clean and makes the code
dependent on kobject internals. Not super nice in my opinion, unless I am
missing something.

> And you really don't even need to check anything, just pass in NULL to
> kobject_del() and friends, it should handle it.>
>>>> -
>>>>  	return 0;
>>>>  }
>>>>  
>>>> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
>>>>  {
>>>>  	struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
>>>>  
>>>> -	if (!sbi || !sbi->s_sysfs_registered)
>>>
>>> How can either of these ever be true?  Note, sbi should be passed here
>>> to this function, not the super block as that is now unregistered from
>>> the system.  Looks like no one has really tested this codepath that much
>>> :(
>>>
>>>> +	if (!sbi)
>>>>  		return;
>>>
>>> this can not ever be true, right?
>>
>> Yes it can, if someone attempt to mount a non zoned device. In that case,
>> fill_super returns early without setting sb->s_fs_info but vfs still calls
>> kill_super.
> 
> But you already had a sbi pointer in the place that this was called, so
> you "know" if you need to even call into here or not.  You are having to
> look up the same pointer multiple times in this call chain, there's no
> need for that.

I am not following here. Either we check that we have sbi here in
zonefs_sysfs_unregister(), or we conditionally call this function in
zonefs_kill_super() with a "if (sbi)". Either way, we need to check since sbi
can be NULL.

> 
> thanks,
> 
> greg k-h
  

On Thu, Apr 06, 2023 at 07:58:38PM +0900, Damien Le Moal wrote:
> On 4/6/23 19:26, Greg KH wrote:
> > On Thu, Apr 06, 2023 at 07:13:38PM +0900, Damien Le Moal wrote:
> >> On 4/6/23 19:05, Greg KH wrote:
> >>> On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
> >>>> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
> >>>> BTW kill kobject_del() directly, because kobject_put() actually covers
> >>>> kobject removal automatically.
> >>>>
> >>>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> >>>> ---
> >>>>  fs/zonefs/sysfs.c  | 11 +++++------
> >>>>  fs/zonefs/zonefs.h |  1 -
> >>>>  2 files changed, 5 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
> >>>> index 8ccb65c2b419..f0783bf7a25c 100644
> >>>> --- a/fs/zonefs/sysfs.c
> >>>> +++ b/fs/zonefs/sysfs.c
> >>>> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
> >>>>  		return ret;
> >>>>  	}
> >>>>  
> >>>> -	sbi->s_sysfs_registered = true;
> >>>
> >>> You know this, why do you need to have a variable tell you this or not?
> >>
> >> If kobject_init_and_add() fails, zonefs_sysfs_register() returns an error and
> >> fill_super will also return that error. vfs will then call kill_super, which
> >> calls zonefs_sysfs_unregister(). For that case, we need to know that we actually
> >> added the kobj.
> > 
> > Ok, but then why not just 0 out the kobject pointer here instead?  That
> > way you will always know if it's a valid pointer or not and you don't
> > have to rely on some other variable?  Use the one that you have already :)
> 
> but sbi->s_kobj is the kobject itself, not a pointer.

Then it should not be there if the kobject is not valid as it should
have been freed when the kobject_init_and_add() call failed, right?

> I can still zero it out in
> case of error to avoid using the added s_sysfs_registered bool. I would need to
> check a field of s_kobj though, which is not super clean and makes the code
> dependent on kobject internals. Not super nice in my opinion, unless I am
> missing something.

See above, if a kobject fails to be registered, just remove the whole
object as it's obviously "dead" now and you can not trust it.

> > And you really don't even need to check anything, just pass in NULL to
> > kobject_del() and friends, it should handle it.>
> >>>> -
> >>>>  	return 0;
> >>>>  }
> >>>>  
> >>>> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
> >>>>  {
> >>>>  	struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
> >>>>  
> >>>> -	if (!sbi || !sbi->s_sysfs_registered)
> >>>
> >>> How can either of these ever be true?  Note, sbi should be passed here
> >>> to this function, not the super block as that is now unregistered from
> >>> the system.  Looks like no one has really tested this codepath that much
> >>> :(
> >>>
> >>>> +	if (!sbi)
> >>>>  		return;
> >>>
> >>> this can not ever be true, right?
> >>
> >> Yes it can, if someone attempt to mount a non zoned device. In that case,
> >> fill_super returns early without setting sb->s_fs_info but vfs still calls
> >> kill_super.
> > 
> > But you already had a sbi pointer in the place that this was called, so
> > you "know" if you need to even call into here or not.  You are having to
> > look up the same pointer multiple times in this call chain, there's no
> > need for that.
> 
> I am not following here. Either we check that we have sbi here in
> zonefs_sysfs_unregister(), or we conditionally call this function in
> zonefs_kill_super() with a "if (sbi)". Either way, we need to check since sbi
> can be NULL.

In zonefs_kill_super() you have get the spi at the top of the function,
so use that, don't make zonefs_sysfs_unregister() have to compute it
again.

But again, if the kobject fails to be registered, you have to treat the
memory contained there as not valid and get rid of it as soon as
possible.

thanks,

greg k-h
  

On 4/6/23 20:18, Greg KH wrote:
> On Thu, Apr 06, 2023 at 07:58:38PM +0900, Damien Le Moal wrote:
>> On 4/6/23 19:26, Greg KH wrote:
>>> On Thu, Apr 06, 2023 at 07:13:38PM +0900, Damien Le Moal wrote:
>>>> On 4/6/23 19:05, Greg KH wrote:
>>>>> On Thu, Apr 06, 2023 at 05:30:56PM +0800, Yangtao Li wrote:
>>>>>> Use kobject_is_added() instead of local `s_sysfs_registered` variables.
>>>>>> BTW kill kobject_del() directly, because kobject_put() actually covers
>>>>>> kobject removal automatically.
>>>>>>
>>>>>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>>>>>> ---
>>>>>>  fs/zonefs/sysfs.c  | 11 +++++------
>>>>>>  fs/zonefs/zonefs.h |  1 -
>>>>>>  2 files changed, 5 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
>>>>>> index 8ccb65c2b419..f0783bf7a25c 100644
>>>>>> --- a/fs/zonefs/sysfs.c
>>>>>> +++ b/fs/zonefs/sysfs.c
>>>>>> @@ -101,8 +101,6 @@ int zonefs_sysfs_register(struct super_block *sb)
>>>>>>  		return ret;
>>>>>>  	}
>>>>>>  
>>>>>> -	sbi->s_sysfs_registered = true;
>>>>>
>>>>> You know this, why do you need to have a variable tell you this or not?
>>>>
>>>> If kobject_init_and_add() fails, zonefs_sysfs_register() returns an error and
>>>> fill_super will also return that error. vfs will then call kill_super, which
>>>> calls zonefs_sysfs_unregister(). For that case, we need to know that we actually
>>>> added the kobj.
>>>
>>> Ok, but then why not just 0 out the kobject pointer here instead?  That
>>> way you will always know if it's a valid pointer or not and you don't
>>> have to rely on some other variable?  Use the one that you have already :)
>>
>> but sbi->s_kobj is the kobject itself, not a pointer.
> 
> Then it should not be there if the kobject is not valid as it should
> have been freed when the kobject_init_and_add() call failed, right?

What do you mean freed ? the kboject itself is a field of zonefs sbi. So the
kobject gets freed together with sbi.

>> I can still zero it out in
>> case of error to avoid using the added s_sysfs_registered bool. I would need to
>> check a field of s_kobj though, which is not super clean and makes the code
>> dependent on kobject internals. Not super nice in my opinion, unless I am
>> missing something.
> 
> See above, if a kobject fails to be registered, just remove the whole
> object as it's obviously "dead" now and you can not trust it.

Well yes, that is what s_sysfs_registered indicates, that the kobject is not
valid. I do not understand what you mean with "just remove the whole object".

>>> And you really don't even need to check anything, just pass in NULL to
>>> kobject_del() and friends, it should handle it.>
>>>>>> -
>>>>>>  	return 0;
>>>>>>  }
>>>>>>  
>>>>>> @@ -110,12 +108,13 @@ void zonefs_sysfs_unregister(struct super_block *sb)
>>>>>>  {
>>>>>>  	struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
>>>>>>  
>>>>>> -	if (!sbi || !sbi->s_sysfs_registered)
>>>>>
>>>>> How can either of these ever be true?  Note, sbi should be passed here
>>>>> to this function, not the super block as that is now unregistered from
>>>>> the system.  Looks like no one has really tested this codepath that much
>>>>> :(
>>>>>
>>>>>> +	if (!sbi)
>>>>>>  		return;
>>>>>
>>>>> this can not ever be true, right?
>>>>
>>>> Yes it can, if someone attempt to mount a non zoned device. In that case,
>>>> fill_super returns early without setting sb->s_fs_info but vfs still calls
>>>> kill_super.
>>>
>>> But you already had a sbi pointer in the place that this was called, so
>>> you "know" if you need to even call into here or not.  You are having to
>>> look up the same pointer multiple times in this call chain, there's no
>>> need for that.
>>
>> I am not following here. Either we check that we have sbi here in
>> zonefs_sysfs_unregister(), or we conditionally call this function in
>> zonefs_kill_super() with a "if (sbi)". Either way, we need to check since sbi
>> can be NULL.
> 
> In zonefs_kill_super() you have get the spi at the top of the function,
> so use that, don't make zonefs_sysfs_unregister() have to compute it
> again.

That I can do, yes.

> 
> But again, if the kobject fails to be registered, you have to treat the
> memory contained there as not valid and get rid of it as soon as
> possible.

If the kobject add failed, we never touch it thanks to s_sysfs_registered. I
still do not see the issue here.

> 
> thanks,
> 
> greg k-h