linux-kernel - Re: [PATCH v2] proc: faster open/read/close with "permanent" files

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:   Wed, 19 Feb 2020 13:06:00 -0800
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Alexey Dobriyan <adobriyan@...il.com>
Cc:     linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2] proc: faster open/read/close with "permanent" files

On Wed, 19 Feb 2020 22:11:27 +0300 Alexey Dobriyan <adobriyan@...il.com> wrote:

> Now that "struct proc_ops" exist we can start putting there stuff which
> could not fly with VFS "struct file_operations"...
> 
> Most of fs/proc/inode.c file is dedicated to make open/read/.../close reliable
> in the event of disappearing /proc entries which usually happens if module is
> getting removed. Files like /proc/cpuinfo which never disappear simply do not
> need such protection.
> 
> Save 2 atomic ops, 1 allocation, 1 free per open/read/close sequence for such
> "permanent" files.
> 
> Enable "permanent" flag for
> 
> 	/proc/cpuinfo
> 	/proc/kmsg
> 	/proc/modules
> 	/proc/slabinfo
> 	/proc/stat
> 	/proc/sysvipc/*
> 	/proc/swaps
> 
> More will come once I figure out foolproof way to prevent out module
> authors from marking their stuff "permanent" for performance reasons
> when it is not.
> 
> This should help with scalability: benchmark is "read /proc/cpuinfo R times
> by N threads scattered over the system".
> 
> 	N	R	t, s (before)	t, s (after)
> 	-----------------------------------------------------
> 	64	4096	1.582458	1.530502	-3.2%
> 	256	4096	6.371926	6.125168	-3.9%
> 	1024	4096	25.64888	24.47528	-4.6%

I guess that's significant.

> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -61,6 +61,7 @@ struct proc_dir_entry {
>  	struct rb_node subdir_node;
>  	char *name;
>  	umode_t mode;
> +	u8 flags;

Add a comment describing what this is?

>  	u8 namelen;
>  	char inline_name[];
>  } __randomize_layout;
>
> ...
>
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -5,6 +5,7 @@
>  #ifndef _LINUX_PROC_FS_H
>  #define _LINUX_PROC_FS_H
>  
> +#include <linux/compiler.h>
>  #include <linux/types.h>
>  #include <linux/fs.h>
>  
> @@ -12,7 +13,21 @@ struct proc_dir_entry;
>  struct seq_file;
>  struct seq_operations;
>  
> +enum {
> +	/*
> +	 * All /proc entries using this ->proc_ops instance are never removed.
> +	 *
> +	 * If in doubt, ignore this flag.
> +	 */
> +#ifdef MODULE
> +	PROC_ENTRY_PERMANENT = 0U,
> +#else
> +	PROC_ENTRY_PERMANENT = 1U << 0,
> +#endif
> +};

That feels quite hacky.  Is it really needed?  Any module which uses
this is simply buggy?

Can we just leave this undefined if MODULE and break the build?

>  struct proc_ops {
> +	unsigned int proc_flags;
>  	int	(*proc_open)(struct inode *, struct file *);
>  	ssize_t	(*proc_read)(struct file *, char __user *, size_t, loff_t *);
>  	ssize_t	(*proc_write)(struct file *, const char __user *, size_t, loff_t *);
> @@ -25,7 +40,7 @@ struct proc_ops {
>  #endif
>  	int	(*proc_mmap)(struct file *, struct vm_area_struct *);
>  	unsigned long (*proc_get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
> -};
> +} __randomize_layout;

Unchangelogged, unrelated?

>  #ifdef CONFIG_PROC_FS
>