[PATCH -mm] proc: faster open/close of files without ->release hook

[PATCH -mm] proc: faster open/close of files without ->release hook

[Alexey Dobriyan]

The whole point of code in fs/proc/inode.c is to make sure ->release
hook is called either at close() or at rmmod time.

All if it is unnecessary if there is no ->release hook.

Save allocation+list manipulations under spinlock in that case.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 fs/proc/inode.c |   41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -342,31 +342,36 @@ static int proc_reg_open(struct inode *inode, struct file *file)
 	 *
 	 * Save every "struct file" with custom ->release hook.
 	 */
-	pdeo = kmalloc(sizeof(struct pde_opener), GFP_KERNEL);
-	if (!pdeo)
-		return -ENOMEM;
-
-	if (!use_pde(pde)) {
-		kfree(pdeo);
+	if (!use_pde(pde))
 		return -ENOENT;
-	}
-	open = pde->proc_fops->open;
+
 	release = pde->proc_fops->release;
+	if (release) {
+		pdeo = kmalloc(sizeof(struct pde_opener), GFP_KERNEL);
+		if (!pdeo) {
+			rv = -ENOMEM;
+			goto out_unuse;
+		}
+	}
 
+	open = pde->proc_fops->open;
 	if (open)
 		rv = open(inode, file);
 
-	if (rv == 0 && release) {
-		/* To know what to release. */
-		pdeo->file = file;
-		pdeo->closing = false;
-		pdeo->c = NULL;
-		spin_lock(&pde->pde_unload_lock);
-		list_add(&pdeo->lh, &pde->pde_openers);
-		spin_unlock(&pde->pde_unload_lock);
-	} else
-		kfree(pdeo);
+	if (release) {
+		if (rv == 0) {
+			/* To know what to release. */
+			pdeo->file = file;
+			pdeo->closing = false;
+			pdeo->c = NULL;
+			spin_lock(&pde->pde_unload_lock);
+			list_add(&pdeo->lh, &pde->pde_openers);
+			spin_unlock(&pde->pde_unload_lock);
+		} else
+			kfree(pdeo);
+	}
 
+out_unuse:
 	unuse_pde(pde);
 	return rv;
 }

[PATCH -mm 1/3] proc: randomize "struct pde_opener"

[Alexey Dobriyan]

The more the merrier.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 fs/proc/internal.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -177,7 +177,7 @@ struct pde_opener {
 	struct list_head lh;
 	bool closing;
 	struct completion *c;
-};
+} __randomize_layout;
 extern const struct inode_operations proc_link_inode_operations;
 
 extern const struct inode_operations proc_pid_link_inode_operations;

[PATCH -mm 2/3] proc: move "struct pde_opener" to kmem cache

[Alexey Dobriyan]

"struct pde_opener" is fixed size and we can have more granular approach
to debugging.

For those who don't know, per cache SLUB poisoning and red zoning
don't work if there is at least one object allocated which is hopeless
in case of kmalloc-64 but not in case of standalone cache.
Although systemd opens 2 files from the get go, so it is hopeless after
all.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 fs/proc/inode.c    |   12 ++++++++----
 fs/proc/internal.h |    2 +-
 fs/proc/root.c     |    2 +-
 3 files changed, 10 insertions(+), 6 deletions(-)

--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -54,6 +54,7 @@ static void proc_evict_inode(struct inode *inode)
 }
 
 static struct kmem_cache *proc_inode_cachep __ro_after_init;
+static struct kmem_cache *pde_opener_cache __ro_after_init;
 
 static struct inode *proc_alloc_inode(struct super_block *sb)
 {
@@ -92,7 +93,7 @@ static void init_once(void *foo)
 	inode_init_once(&ei->vfs_inode);
 }
 
-void __init proc_init_inodecache(void)
+void __init proc_init_kmemcache(void)
 {
 	proc_inode_cachep = kmem_cache_create("proc_inode_cache",
 					     sizeof(struct proc_inode),
@@ -100,6 +101,9 @@ void __init proc_init_inodecache(void)
 						SLAB_MEM_SPREAD|SLAB_ACCOUNT|
 						SLAB_PANIC),
 					     init_once);
+	pde_opener_cache =
+		kmem_cache_create("pde_opener", sizeof(struct pde_opener), 0,
+				  SLAB_PANIC, NULL);
 }
 
 static int proc_show_options(struct seq_file *seq, struct dentry *root)
@@ -172,7 +176,7 @@ static void close_pdeo(struct proc_dir_entry *pde, struct pde_opener *pdeo)
 		spin_unlock(&pde->pde_unload_lock);
 		if (unlikely(c))
 			complete(c);
-		kfree(pdeo);
+		kmem_cache_free(pde_opener_cache, pdeo);
 	}
 }
 
@@ -347,7 +351,7 @@ static int proc_reg_open(struct inode *inode, struct file *file)
 
 	release = pde->proc_fops->release;
 	if (release) {
-		pdeo = kmalloc(sizeof(struct pde_opener), GFP_KERNEL);
+		pdeo = kmem_cache_alloc(pde_opener_cache, GFP_KERNEL);
 		if (!pdeo) {
 			rv = -ENOMEM;
 			goto out_unuse;
@@ -368,7 +372,7 @@ static int proc_reg_open(struct inode *inode, struct file *file)
 			list_add(&pdeo->lh, &pde->pde_openers);
 			spin_unlock(&pde->pde_unload_lock);
 		} else
-			kfree(pdeo);
+			kmem_cache_free(pde_opener_cache, pdeo);
 	}
 
 out_unuse:
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -182,7 +182,7 @@ extern const struct inode_operations proc_link_inode_operations;
 
 extern const struct inode_operations proc_pid_link_inode_operations;
 
-extern void proc_init_inodecache(void);
+void proc_init_kmemcache(void);
 void set_proc_pid_nlink(void);
 extern struct inode *proc_get_inode(struct super_block *, struct proc_dir_entry *);
 extern int proc_fill_super(struct super_block *, void *data, int flags);
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -125,7 +125,7 @@ void __init proc_root_init(void)
 {
 	int err;
 
-	proc_init_inodecache();
+	proc_init_kmemcache();
 	set_proc_pid_nlink();
 	err = register_filesystem(&proc_fs_type);
 	if (err)

[PATCH -mm 3/3] proc: account "struct pde_opener"

[Alexey Dobriyan]

The allocation is persistent in fact as any fool can open a file in
/proc and sit on it.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 fs/proc/inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -103,7 +103,7 @@ void __init proc_init_kmemcache(void)
 					     init_once);
 	pde_opener_cache =
 		kmem_cache_create("pde_opener", sizeof(struct pde_opener), 0,
-				  SLAB_PANIC, NULL);
+				  SLAB_ACCOUNT|SLAB_PANIC, NULL);
 }
 
 static int proc_show_options(struct seq_file *seq, struct dentry *root)

Re: [PATCH -mm 1/3] proc: randomize "struct pde_opener"

[Al Viro]

On Wed, Feb 14, 2018 at 11:19:35AM +0300, Alexey Dobriyan wrote:

> The more the merrier.

ITYM "Sanity is overrated anyway."

Re: [PATCH -mm 1/3] proc: randomize "struct pde_opener"

[Alexey Dobriyan]

On Thu, Feb 15, 2018 at 07:07:13PM +0000, Al Viro wrote:
> On Wed, Feb 14, 2018 at 11:19:35AM +0300, Alexey Dobriyan wrote:
> 
> > The more the merrier.
> 
> ITYM "Sanity is overrated anyway."

If you view annotations as debugging option the thing is not that bad.

Re: [PATCH -mm 1/3] proc: randomize "struct pde_opener"

[Al Viro]

On Fri, Feb 16, 2018 at 12:41:13AM +0300, Alexey Dobriyan wrote:
> On Thu, Feb 15, 2018 at 07:07:13PM +0000, Al Viro wrote:
> > On Wed, Feb 14, 2018 at 11:19:35AM +0300, Alexey Dobriyan wrote:
> > 
> > > The more the merrier.
> > 
> > ITYM "Sanity is overrated anyway."
> 
> If you view annotations as debugging option the thing is not that bad.

Yes, if your goal is to debug gcc.  Look, randomize_layout is a bad idea,
with worse implementation.  It's security theatre with no real benefits,
it makes for much harder kernel debugging, it buggers cachelines without
noticing *AND* it triggers gcc version-dependent miscompiles that
cheerfully cause memory corruption.

IMO we should remove that misfeature from at least the core VFS data
structures.  Sure, it's one of the "if you are dumb enough to enable
it, you get to pay the price" things, but that kind of garbage tends
to leak into distro builds.

I hoped that Kees would get a clue and remove the particularly bad
instances himself, but since that hasn't happened yet, I'm removing
the VFS ones this cycle.

And gcc bugs are not the only problem here.  Look at this:
struct dentry {
        /* RCU lookup touched fields */
        unsigned int d_flags;           /* protected by d_lock */
        seqcount_t d_seq;               /* per dentry seqlock */
        struct hlist_bl_node d_hash;    /* lookup hash list */
        struct dentry *d_parent;        /* parent directory */
        struct qstr d_name;
        struct inode *d_inode;          /* Where the name belongs to - NULL is
                                         * negative */
        unsigned char d_iname[DNAME_INLINE_LEN];        /* small names */

        /* Ref lookup also touches following */
        struct lockref d_lockref;       /* per-dentry lock and refcount */
        const struct dentry_operations *d_op;
        struct super_block *d_sb;       /* The root of the dentry tree */
        unsigned long d_time;           /* used by d_revalidate */
        void *d_fsdata;                 /* fs-specific data */

        union {
                struct list_head d_lru;         /* LRU list */
                wait_queue_head_t *d_wait;      /* in-lookup ones only */
        };
        struct list_head d_child;       /* child of parent list */
        struct list_head d_subdirs;     /* our children */
        /*
         * d_alias and d_rcu can share memory
         */
        union {
                struct hlist_node d_alias;      /* inode alias list */
                struct hlist_bl_node d_in_lookup_hash;  /* only for in-lookup ones */
                struct rcu_head d_rcu;
        } d_u;
} __randomize_layout;

Guess what happens to cache footprint of dcache lookups if the bunch in the
beginning gets spread over the entire thing?  Right...  And that's besides the
outright miscompiles.

Re: [PATCH -mm 1/3] proc: randomize "struct pde_opener"

[Kees Cook]

On Wed, Feb 14, 2018 at 12:19 AM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> The more the merrier.

What made you choose this structure, BTW?

> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>
>  fs/proc/internal.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -177,7 +177,7 @@ struct pde_opener {
>         struct list_head lh;
>         bool closing;
>         struct completion *c;
> -};
> +} __randomize_layout;
>  extern const struct inode_operations proc_link_inode_operations;
>
>  extern const struct inode_operations proc_pid_link_inode_operations;

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH -mm 1/3] proc: randomize "struct pde_opener"

[Kees Cook]

On Thu, Feb 15, 2018 at 4:13 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> Guess what happens to cache footprint of dcache lookups if the bunch in the
> beginning gets spread over the entire thing?  Right...  And that's besides the
> outright miscompiles.

Mentioned in private communication, but just for posterity:
GCC_PLUGIN_RANDSTRUCT_PERFORMANCE exists specifically to address those
kinds of performance concerns.

As to removing the markings: please don't. Instead, you'd mentioned
wanting to add a TAINT flag for this, and I think that sounds entirely
reasonable. We have TAINT flags for considerably less insane things.
:) I can send that patch.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH -mm 1/3] proc: randomize "struct pde_opener"

[Alexey Dobriyan]

On Thu, Feb 15, 2018 at 04:53:29PM -0800, Kees Cook wrote:
> On Wed, Feb 14, 2018 at 12:19 AM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > The more the merrier.
> 
> What made you choose this structure, BTW?

Nothing depends on its layout. No funky memcpy/memset...
It is strange you didn't annotate like 95% of structs.

> > @@ -177,7 +177,7 @@ struct pde_opener {
> >         struct list_head lh;
> >         bool closing;
> >         struct completion *c;
> > -};
> > +} __randomize_layout;

Re: [PATCH -mm 1/3] proc: randomize "struct pde_opener"

[Alexey Dobriyan]

On Fri, Feb 16, 2018 at 12:13:17AM +0000, Al Viro wrote:
> On Fri, Feb 16, 2018 at 12:41:13AM +0300, Alexey Dobriyan wrote:
> > On Thu, Feb 15, 2018 at 07:07:13PM +0000, Al Viro wrote:
> > > On Wed, Feb 14, 2018 at 11:19:35AM +0300, Alexey Dobriyan wrote:
> > > 
> > > > The more the merrier.
> > > 
> > > ITYM "Sanity is overrated anyway."
> > 
> > If you view annotations as debugging option the thing is not that bad.
> 
> Yes, if your goal is to debug gcc.  Look, randomize_layout is a bad idea,
> with worse implementation.  It's security theatre with no real benefits,
> it makes for much harder kernel debugging, it buggers cachelines without
> noticing *AND* it triggers gcc version-dependent miscompiles that
> cheerfully cause memory corruption.

So at least gcc people should all enable it. :^)