linux-kernel - Re: [PATCH 3/3] zonefs: convert to use kobject_is

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8ca8c138-67fd-73ed-1ce5-c090d49f31e9@fastmail.com>
Date:   Thu, 6 Apr 2023 19:13:38 +0900
From:   Damien Le Moal <dlemoal@...tmail.com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Yangtao Li <frank.li@...o.com>
Cc:     xiang@...nel.org, chao@...nel.org, huyue2@...lpad.com,
        jefflexu@...ux.alibaba.com, damien.lemoal@...nsource.wdc.com,
        naohiro.aota@....com, jth@...nel.org, rafael@...nel.org,
        linux-erofs@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 3/3] zonefs: convert to use kobject_is_added()

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