linux-ext4 - Re: [PATCH] ubifs: remove unnecessary kobject

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>] [day] [month] [year] [list]

Date:   Thu, 6 Apr 2023 11:36:47 +0800
From:   Zhihao Cheng <chengzhihao1@...wei.com>
To:     Yangtao Li <frank.li@...o.com>, Richard Weinberger <richard@....at>
CC:     <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <linux-ext4@...r.kernel.org>, <tytso@....edu>, <jack@...e.com>,
        yangerkun <yangerkun@...wei.com>
Subject: Re: [PATCH] ubifs: remove unnecessary kobject_del()

Hi, Yangtao
> kobject_put() actually covers kobject removal automatically, which is
> single stage removal. So it is safe to kill kobject_del() directly.
> 
> Signed-off-by: Yangtao Li <frank.li@...o.com>
> ---
>   fs/ubifs/sysfs.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/fs/ubifs/sysfs.c b/fs/ubifs/sysfs.c
> index 1c958148bb87..1ffdc3c9b340 100644
> --- a/fs/ubifs/sysfs.c
> +++ b/fs/ubifs/sysfs.c
> @@ -130,7 +130,6 @@ int ubifs_sysfs_register(struct ubifs_info *c)
>   
>   void ubifs_sysfs_unregister(struct ubifs_info *c)
>   {
> -	kobject_del(&c->kobj);
>   	kobject_put(&c->kobj);
>   	wait_for_completion(&c->kobj_unregister);
>   
> 

This patch looks harmless at the view of ubifs, kobject_cleanup() 
finally invokes __kobject_del and releases parent just like 
kobject_del() does. A difference is that the releasing sequence of 
kobj's parent is put after releasing kobj.

About the use case of kobject_del(), we may refer to other filesystems' 
implementations, eg. ext4_put_super(). Firstly ext4_unregister_sysfs() 
removes all sysfs interfaces, then jbd2_journal_destroy() destroys 
sbi->s_journal, finally sbi->s_kobj is released. Here kobject_del() 
stops user accessing sbi->s_journal by sysfs interface 
journal_task，because sbi->s_journal will be released soon before 
kobject_put().

I think we should still reserve the 'redundant' kobject_del(), removing 
it won't bring any performance improvement.

BTW, in ext4_put_super(), flush_stashed_error_work(which could access 
sbi->s_kobj) is flushed after kobject removed. is it okay to replace 
kobject_del(&sbi->s_kobj) with kobject_put(&sbi->s_kobj)?

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index a22417d113ca..9e3744099d1e 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1296,8 +1296,6 @@ static void ext4_put_super(struct super_block *sb)
          * Now that we are completely done shutting down the
          * superblock, we need to actually destroy the kobject.
          */
-       kobject_put(&sbi->s_kobj);
-       wait_for_completion(&sbi->s_kobj_unregister);
         if (sbi->s_chksum_driver)
                 crypto_free_shash(sbi->s_chksum_driver);
         kfree(sbi->s_blockgroup_lock);
diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
index 12d6252e3e22..be92a09bb8a0 100644
--- a/fs/ext4/sysfs.c
+++ b/fs/ext4/sysfs.c
@@ -562,7 +562,8 @@ void ext4_unregister_sysfs(struct super_block *sb)

         if (sbi->s_proc)
                 remove_proc_subtree(sb->s_id, ext4_proc_root);
-       kobject_del(&sbi->s_kobj);
+       kobject_put(&sbi->s_kobj);
+       wait_for_completion(&sbi->s_kobj_unregister);
  }

  int __init ext4_init_sysfs(void)