linux-kernel - Re: [PATCH v5 1/2] kernfs: use hashed mutex and spinlock in place of global ones.

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]

Date:   Mon, 14 Feb 2022 23:13:02 +1100
From:   Imran Khan <imran.f.khan@...cle.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     tj@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/2] kernfs: use hashed mutex and spinlock in place of
 global ones.

Hello Greg,
Thanks for reviewing this.

On 8/2/22 10:27 pm, Greg KH wrote:
> On Sun, Feb 06, 2022 at 12:09:24PM +1100, Imran Khan wrote:
>> diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
>> index f9cc912c31e1b..cc49a6cd94154 100644
>> --- a/fs/kernfs/kernfs-internal.h
>> +++ b/fs/kernfs/kernfs-internal.h
>> @@ -31,6 +31,7 @@ struct kernfs_iattrs {
>>  	atomic_t		user_xattr_size;
>>  };
>>  
>> +
>>  /* +1 to avoid triggering overflow warning when negating it */
>>  #define KN_DEACTIVATED_BIAS		(INT_MIN + 1)
>>  
> 
> Nit, the above change isn't needed :)

This has been removed in version 6 of patch at [1].
> 
>> @@ -147,4 +148,54 @@ void kernfs_drain_open_files(struct kernfs_node *kn);
>>   */
>>  extern const struct inode_operations kernfs_symlink_iops;
>>  
[...]
>> +}
> 
> Can't you use kernfs_open_node_lock_ptr() in kernfs_open_node_lock() to
> make this use less duplicated code?
> 

Yes. I have removed duplicate code in version 6 of patch at [1].
> 
>> +

[...]
>> +	return lock;
>> +}
> 
> Same thing here.
> 
> 
Done.

> 
>> +
>>  #endif	/* __KERNFS_INTERNAL_H */
>> diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
>> index 861c4f0f8a29f..5bf9f02ce9dce 100644
>> --- a/include/linux/kernfs.h
>> +++ b/include/linux/kernfs.h
>> @@ -18,6 +18,8 @@
>>  #include <linux/uidgid.h>
>>  #include <linux/wait.h>
>>  #include <linux/rwsem.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/cache.h>
>>  
>>  struct file;
>>  struct dentry;
>> @@ -34,6 +36,40 @@ struct kernfs_fs_context;
>>  struct kernfs_open_node;
>>  struct kernfs_iattrs;
>>  
>> +/*
[...]
>> @@ -90,6 +126,7 @@ enum kernfs_root_flag {
>>  	KERNFS_ROOT_SUPPORT_USER_XATTR		= 0x0008,
>>  };
>>  
>> +
>>  /* type-specific structures for kernfs_node union members */
>>  struct kernfs_elem_dir {
>>  	unsigned long		subdirs;
>> @@ -201,6 +238,8 @@ struct kernfs_root {
>>  
>>  	wait_queue_head_t	deactivate_waitq;
>>  	struct rw_semaphore	kernfs_rwsem;
>> +	struct kernfs_open_node_lock open_node_locks[NR_KERNFS_LOCKS];
>> +	struct kernfs_open_file_mutex open_file_mutex[NR_KERNFS_LOCKS];
>>  };
> 
> I think struct kernfs_root can be declared locally inside fs/kernfs/ to
> keep other subsystems/files from having to see this structure, right?
> That would make this a bit less of a "rebuild the world" type of change
> and could be done in a patch before this one.

Tejun has suggested to avoid using per-fs hash table and take hash table
outside kernfs_root. So this patch is not changing kernfs_root and I
have left it in it's current place.
Please let me know if moving kernfs_root is still needed.

Thanks,
-- Imran

[1]:
https://lore.kernel.org/lkml/20220214120322.2402628-1-imran.f.khan@oracle.com/

> 
> Overall, this looks sane to me, nice work.
> 
> Tejun, any comments?
> 
> thanks,
> 
> greg k-h