introduce proc_inode->pid_entry

RFC PATCH In review Oleg Nesterov 3 participants 12 messages Aug 8, 2014 – Aug 10, 2014

Subsystems (4)

VFS Proc filesystem Memory mapping Proc sysctl

lore

Share & tools

lore

Oleg Nesterov

author

2014-08-08 18:57 UTC #1 lore raw

RFC PATCH 1/5 proc: intoduce proc_inode->pid_entry and is_tgid_pid_entry()

In reply to Oleg Nesterov

Add the new "struct pid_entry *" member into struct proc_inode, and the new helper, is_tgid_pid_entry(), it can be used by *base_stuff to figure out whether we need tgid or tid info.

Note: this obviously blows struct proc_inode, but I think it is simple to put ->fd, ->sysctl/sysctl_entry, and the new member in the union, the only complication is that proc_evict_inode() should be carefull with ->sysctl.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/proc/base.c     |   16 +++++++++++++++-
 fs/proc/inode.c    |    1 +
 fs/proc/internal.h |    4 ++++
 3 files changed, 20 insertions(+), 1 deletions(-)

fs/proc/base.c +15 -1

index 9aef9ac..ea11bf1 100644
@@ -2124,6 +2124,9 @@ static int proc_pident_instantiate(struct inode *dir,
 		goto out;
 
 	ei = PROC_I(inode);
+	ei->pid_entry = p;
+	ei->op = p->op;
+
 	inode->i_mode = p->mode;
 	if (S_ISDIR(inode->i_mode))
 		set_nlink(inode, 2);	/* Use getattr to fix if necessary */

@@ -2131,7 +2134,7 @@ static int proc_pident_instantiate(struct inode *dir,
 		inode->i_op = p->iop;
 	if (p->fop)
 		inode->i_fop = p->fop;
-	ei->op = p->op;
+
 	d_set_d_op(dentry, &pid_dentry_operations);
 	d_add(dentry, inode);
 	/* Close the race of the process dying before we return the dentry */

@@ -2978,6 +2981,17 @@ static const struct pid_entry tid_base_stuff[] = {
 #endif
 };
 
+bool is_tgid_pid_entry(const struct pid_entry *p)
+{
+	if (p >= tgid_base_stuff &&
+	    p <  tgid_base_stuff + ARRAY_SIZE(tgid_base_stuff))
+		return true;
+	if (p >= tid_base_stuff &&
+	    p <  tid_base_stuff + ARRAY_SIZE(tid_base_stuff))
+		return false;
+	BUG();
+}
+
 static int proc_tid_base_readdir(struct file *file, struct dir_context *ctx)
 {
 	return proc_pident_readdir(file, ctx,

fs/proc/inode.c +1 -0

index 0adbc02..45ad621 100644
@@ -73,6 +73,7 @@ static struct inode *proc_alloc_inode(struct super_block *sb)
 	ei->pde = NULL;
 	ei->sysctl = NULL;
 	ei->sysctl_entry = NULL;
+	ei->pid_entry = NULL;
 	ei->ns.ns = NULL;
 	ei->ns.ns_ops = NULL;
 	inode = &ei->vfs_inode;

fs/proc/internal.h +4 -0

index 78784cd..2e310b6 100644
@@ -15,6 +15,7 @@
 #include <linux/atomic.h>
 #include <linux/binfmts.h>
 
+struct pid_entry;
 struct ctl_table_header;
 struct mempolicy;

@@ -65,6 +66,7 @@ struct proc_inode {
 	struct proc_dir_entry *pde;
 	struct ctl_table_header *sysctl;
 	struct ctl_table *sysctl_entry;
+	const struct pid_entry *pid_entry;
 	struct proc_ns ns;
 	struct inode vfs_inode;
 };

@@ -147,6 +149,8 @@ out:
  */
 extern const struct file_operations proc_tid_children_operations;
 
+extern bool is_tgid_pid_entry(const struct pid_entry *p);
+
 extern int proc_tid_stat(struct seq_file *, struct pid_namespace *,
 			 struct pid *, struct task_struct *);
 extern int proc_tgid_stat(struct seq_file *, struct pid_namespace *,

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-08-08 18:57 UTC #5 lore raw

RFC PATCH 2/5 proc: unify proc_tgid_stat() and proc_tid_stat()

In reply to Oleg Nesterov

Now that we have is_tgid_pid_entry() we do not need 2 different helpers for ONE("stat"), we can kill proc_tgid_stat() and proc_tid_stat() and turn do_task_stat() into proc_pid_stat().

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/proc/array.c    |   17 +++--------------
 fs/proc/base.c     |    4 ++--
 fs/proc/internal.h |    4 +---
 3 files changed, 6 insertions(+), 19 deletions(-)

fs/proc/array.c +3 ~~-14~~

index 64db2bc..d096bd6 100644
@@ -377,9 +377,10 @@ int proc_pid_status(struct seq_file *m, struct pid_namespace *ns,
 	return 0;
 }
 
-static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
-			struct pid *pid, struct task_struct *task, int whole)
+int proc_pid_stat(struct seq_file *m, struct pid_namespace *ns,
+			struct pid *pid, struct task_struct *task)
 {
+	bool whole = is_tgid_pid_entry(PROC_I(m->private)->pid_entry);
 	unsigned long vsize, eip, esp, wchan = ~0UL;
 	int priority, nice;
 	int tty_pgrp = -1, tty_nr = 0;

@@ -550,18 +551,6 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns,
 	return 0;
 }
 
-int proc_tid_stat(struct seq_file *m, struct pid_namespace *ns,
-			struct pid *pid, struct task_struct *task)
-{
-	return do_task_stat(m, ns, pid, task, 0);
-}
-
-int proc_tgid_stat(struct seq_file *m, struct pid_namespace *ns,
-			struct pid *pid, struct task_struct *task)
-{
-	return do_task_stat(m, ns, pid, task, 1);
-}
-
 int proc_pid_statm(struct seq_file *m, struct pid_namespace *ns,
 			struct pid *pid, struct task_struct *task)
 {

fs/proc/base.c +2 -2

index ea11bf1..9d47fe0 100644
@@ -2581,7 +2581,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	INF("syscall",    S_IRUSR, proc_pid_syscall),
 #endif
 	INF("cmdline",    S_IRUGO, proc_pid_cmdline),
-	ONE("stat",       S_IRUGO, proc_tgid_stat),
+	ONE("stat",       S_IRUGO, proc_pid_stat),
 	ONE("statm",      S_IRUGO, proc_pid_statm),
 	REG("maps",       S_IRUGO, proc_pid_maps_operations),
 #ifdef CONFIG_NUMA

@@ -2917,7 +2917,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	INF("syscall",   S_IRUSR, proc_pid_syscall),
 #endif
 	INF("cmdline",   S_IRUGO, proc_pid_cmdline),
-	ONE("stat",      S_IRUGO, proc_tid_stat),
+	ONE("stat",      S_IRUGO, proc_pid_stat),
 	ONE("statm",     S_IRUGO, proc_pid_statm),
 	REG("maps",      S_IRUGO, proc_tid_maps_operations),
 #ifdef CONFIG_CHECKPOINT_RESTORE

fs/proc/internal.h +1 -3

index 2e310b6..c7bb0e3 100644
@@ -151,10 +151,8 @@ extern const struct file_operations proc_tid_children_operations;
 
 extern bool is_tgid_pid_entry(const struct pid_entry *p);
 
-extern int proc_tid_stat(struct seq_file *, struct pid_namespace *,
+extern int proc_pid_stat(struct seq_file *, struct pid_namespace *,
 			 struct pid *, struct task_struct *);
-extern int proc_tgid_stat(struct seq_file *, struct pid_namespace *,
-			  struct pid *, struct task_struct *);
 extern int proc_pid_status(struct seq_file *, struct pid_namespace *,
 			   struct pid *, struct task_struct *);
 extern int proc_pid_statm(struct seq_file *, struct pid_namespace *,

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-08-08 18:57 UTC #6 lore raw

RFC PATCH 3/5 proc: kill *_tid_*maps* stuff

In reply to Oleg Nesterov

Kill *_tid_*maps* data/functions. proc_maps_open() can use is_tgid_pid_entry() and initialize priv->is_tgid for ->show() methods.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/proc/base.c     |    6 +-
 fs/proc/internal.h |    4 +-
 fs/proc/task_mmu.c |  140 ++++++++--------------------------------------------
 3 files changed, 25 insertions(+), 125 deletions(-)

fs/proc/base.c +3 -3

index 9d47fe0..368f6fe 100644
@@ -2919,12 +2919,12 @@ static const struct pid_entry tid_base_stuff[] = {
 	INF("cmdline",   S_IRUGO, proc_pid_cmdline),
 	ONE("stat",      S_IRUGO, proc_pid_stat),
 	ONE("statm",     S_IRUGO, proc_pid_statm),
-	REG("maps",      S_IRUGO, proc_tid_maps_operations),
+	REG("maps",      S_IRUGO, proc_pid_maps_operations),
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	REG("children",  S_IRUGO, proc_tid_children_operations),
 #endif
 #ifdef CONFIG_NUMA
-	REG("numa_maps", S_IRUGO, proc_tid_numa_maps_operations),
+	REG("numa_maps", S_IRUGO, proc_pid_numa_maps_operations),
 #endif
 	REG("mem",       S_IRUSR|S_IWUSR, proc_mem_operations),
 	LNK("cwd",       proc_cwd_link),

@@ -2934,7 +2934,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	REG("mountinfo",  S_IRUGO, proc_mountinfo_operations),
 #ifdef CONFIG_PROC_PAGE_MONITOR
 	REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
-	REG("smaps",     S_IRUGO, proc_tid_smaps_operations),
+	REG("smaps",     S_IRUGO, proc_pid_smaps_operations),
 	REG("pagemap",    S_IRUSR, proc_pagemap_operations),
 #endif
 #ifdef CONFIG_SECURITY

fs/proc/internal.h +1 -3

index c7bb0e3..46500af 100644
@@ -268,6 +268,7 @@ extern int proc_remount(struct super_block *, int *, char *);
  */
 struct proc_maps_private {
 	struct pid *pid;
+	bool is_tgid;
 	struct task_struct *task;
 	struct mm_struct *mm;
 #ifdef CONFIG_MMU

@@ -281,11 +282,8 @@ struct proc_maps_private {
 struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode);
 
 extern const struct file_operations proc_pid_maps_operations;
-extern const struct file_operations proc_tid_maps_operations;
 extern const struct file_operations proc_pid_numa_maps_operations;
-extern const struct file_operations proc_tid_numa_maps_operations;
 extern const struct file_operations proc_pid_smaps_operations;
-extern const struct file_operations proc_tid_smaps_operations;
 extern const struct file_operations proc_clear_refs_operations;
 extern const struct file_operations proc_pagemap_operations;

fs/proc/task_mmu.c +21 ~~-119~~

255 lines not on this page · raw message

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-08-08 18:57 UTC #7 lore raw

RFC PATCH 4/5 proc: introduce pid_entry_name()

In reply to Oleg Nesterov

Yes, this is ugly. I think we should simply export struct pid_entry, or make is_tgid_pid_entry/pid_entry_name "struct inode *". Or something else. Lets discuss this later.

See the next patch which explains why.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/proc/base.c     |    7 +++++++
 fs/proc/internal.h |    1 +
 2 files changed, 8 insertions(+), 0 deletions(-)

fs/proc/base.c +7 -0

index 368f6fe..81d372c 100644
@@ -2992,6 +2992,13 @@ bool is_tgid_pid_entry(const struct pid_entry *p)
 	BUG();
 }
 
+// THIS IS UGLY, JUST TO DISCUSS RFC
+const char *pid_entry_name(const struct pid_entry *p)
+{
+	is_tgid_pid_entry(p);	// trigger BUG_ON() if not valid
+	return p->name;
+}
+
 static int proc_tid_base_readdir(struct file *file, struct dir_context *ctx)
 {
 	return proc_pident_readdir(file, ctx,

fs/proc/internal.h +1 -0

index 46500af..c162db2 100644
@@ -150,6 +150,7 @@ out:
 extern const struct file_operations proc_tid_children_operations;
 
 extern bool is_tgid_pid_entry(const struct pid_entry *p);
+extern const char *pid_entry_name(const struct pid_entry *p);
 
 extern int proc_pid_stat(struct seq_file *, struct pid_namespace *,
 			 struct pid *, struct task_struct *);

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-08-08 18:58 UTC #8 lore raw

RFC PATCH 5/5 proc: unify proc_pid_*maps* stuff

In reply to Oleg Nesterov

A single seq_operations/file_operations pair can handle maps, smaps, and numa_maps files. Just the generic ->open() needs to look at pid_entry_name() and initialize priv->show() accordingly.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/proc/base.c     |    8 ++--
 fs/proc/internal.h |    3 +-
 fs/proc/task_mmu.c |  123 +++++++++++++++++++++-------------------------------
 3 files changed, 54 insertions(+), 80 deletions(-)

fs/proc/base.c +4 -4

index 81d372c..4018a86 100644
@@ -2585,7 +2585,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	ONE("statm",      S_IRUGO, proc_pid_statm),
 	REG("maps",       S_IRUGO, proc_pid_maps_operations),
 #ifdef CONFIG_NUMA
-	REG("numa_maps",  S_IRUGO, proc_pid_numa_maps_operations),
+	REG("numa_maps",  S_IRUGO, proc_pid_maps_operations),
 #endif
 	REG("mem",        S_IRUSR|S_IWUSR, proc_mem_operations),
 	LNK("cwd",        proc_cwd_link),

@@ -2596,7 +2596,7 @@ static const struct pid_entry tgid_base_stuff[] = {
 	REG("mountstats", S_IRUSR, proc_mountstats_operations),
 #ifdef CONFIG_PROC_PAGE_MONITOR
 	REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
-	REG("smaps",      S_IRUGO, proc_pid_smaps_operations),
+	REG("smaps",      S_IRUGO, proc_pid_maps_operations),
 	REG("pagemap",    S_IRUSR, proc_pagemap_operations),
 #endif
 #ifdef CONFIG_SECURITY

@@ -2924,7 +2924,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	REG("children",  S_IRUGO, proc_tid_children_operations),
 #endif
 #ifdef CONFIG_NUMA
-	REG("numa_maps", S_IRUGO, proc_pid_numa_maps_operations),
+	REG("numa_maps", S_IRUGO, proc_pid_maps_operations),
 #endif
 	REG("mem",       S_IRUSR|S_IWUSR, proc_mem_operations),
 	LNK("cwd",       proc_cwd_link),

@@ -2934,7 +2934,7 @@ static const struct pid_entry tid_base_stuff[] = {
 	REG("mountinfo",  S_IRUGO, proc_mountinfo_operations),
 #ifdef CONFIG_PROC_PAGE_MONITOR
 	REG("clear_refs", S_IWUSR, proc_clear_refs_operations),
-	REG("smaps",     S_IRUGO, proc_pid_smaps_operations),
+	REG("smaps",     S_IRUGO, proc_pid_maps_operations),
 	REG("pagemap",    S_IRUSR, proc_pagemap_operations),
 #endif
 #ifdef CONFIG_SECURITY

fs/proc/internal.h +1 -2

index c162db2..17091c9 100644
@@ -270,6 +270,7 @@ extern int proc_remount(struct super_block *, int *, char *);
 struct proc_maps_private {
 	struct pid *pid;
 	bool is_tgid;
+	int (*show)(struct seq_file *m, void *v);
 	struct task_struct *task;
 	struct mm_struct *mm;
 #ifdef CONFIG_MMU

@@ -283,8 +284,6 @@ struct proc_maps_private {
 struct mm_struct *proc_mem_open(struct inode *inode, unsigned int mode);
 
 extern const struct file_operations proc_pid_maps_operations;
-extern const struct file_operations proc_pid_numa_maps_operations;
-extern const struct file_operations proc_pid_smaps_operations;
 extern const struct file_operations proc_clear_refs_operations;
 extern const struct file_operations proc_pagemap_operations;

fs/proc/task_mmu.c +49 ~~-74~~

162 lines not on this page · raw message

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2014-08-08 18:59 UTC #0 lore raw

cover introduce proc_inode->pid_entry

Hello,

Obviously not for inclusion. The patches are horrible, break task_nommu.c, untested, etc. Only to explain what I mean and discuss the intent, at least. On top of recent /proc/pid/*maps* cleanups I sent.

To me it looks a bit annoying that task_mmu.c needs 6 seq_operations's and 6 file_operations's to handle /proc/pid/*maps*. And _only_ because ->show() differs.

Eric, et al, what do you think? At least something like 1-3 looks like a good cleanup imho. And afaics we can do more cleanups on top.

Oleg.

 fs/proc/array.c    |   17 +---
 fs/proc/base.c     |   37 +++++++--
 fs/proc/inode.c    |    1 +
 fs/proc/internal.h |   16 ++--
 fs/proc/task_mmu.c |  237 +++++++++++++---------------------------------------
 5 files changed, 98 insertions(+), 210 deletions(-)

Cyrill Gorcunov

2014-08-08 20:05 UTC #2 lore raw

Re: [RFC PATCH 1/5] proc: intoduce proc_inode->pid_entry and is_tgid_pid_entry()

In reply to Oleg Nesterov

On Fri, Aug 08, 2014 at 08:57:47PM +0200, Oleg Nesterov wrote:
...

fs/proc/inode.c static struct inode *proc_alloc_inode(struct super_block *sb) · line 79

Quoted 9 lines

+++ b/fs/proc/inode.c

fs/proc/inode.c +1 -0

@@ -73,6 +73,7 @@ static struct inode *proc_alloc_inode(struct super_block *sb)
 	ei->pde = NULL;
 	ei->sysctl = NULL;
 	ei->sysctl_entry = NULL;
+	ei->pid_entry = NULL;
 	ei->ns.ns = NULL;
 	ei->ns.ns_ops = NULL;
 	inode = &ei->vfs_inode;

Hi Oleg, sorry for not responding to previous emails, will try to review this things tomorrow (from a glance looks quite good!). Btw, this moment strike my eyes -- why don't we use kmem_cache_zalloc here but do assign nils again and again, maybe worth to address as well?

Eric W. Biederman

2014-08-08 22:07 UTC #9 lore raw

In reply to Oleg Nesterov

Quoted 6 lines · Oleg Nesterov

Hello,
Obviously not for inclusion. The patches are horrible, break task_nommu.c, untested, etc. Only to explain what I mean and discuss the intent, at least. On top of recent /proc/pid/*maps* cleanups I sent.

Quoted 6 lines

To me it looks a bit annoying that task_mmu.c needs 6 seq_operations's and 6 file_operations's to handle /proc/pid/*maps*. And _only_ because ->show() differs.
Eric, et al, what do you think? At least something like 1-3 looks like a good cleanup imho. And afaics we can do more cleanups on top.

I see where you are getting annoyed.

Taking a quick look at task_mmu.c It looks like the tgid vs pid logic to decided which stack or stacks to display is simply incorrect.

tgid vs pid is all about do we perform the per thread group rollups or not. Because we have /proc/<tid>/ directories that need the rollups but are per thread.

At a practical level moving pid_entry into the proc inode is ugly especially for the hack that is is_tgid_pid_entry.

That test could be implemented more easily by looking at the parent directories inode operations and seeing if they are proc_root_inode_operations.

Similarly you can get the names out of the dentry, although comparing on the dentry name feels like a real hack.

Given where you are starting I think tack_mmu.c code that decides when/which stack deserves a serious audit.

Eric

Eric W. Biederman

2014-08-08 22:15 UTC #10 lore raw

Quoted 32 lines · [email protected] (Eric W. Biederman) writes:

Quoted 6 lines · Oleg Nesterov
Hello,
Obviously not for inclusion. The patches are horrible, break task_nommu.c, untested, etc. Only to explain what I mean and discuss the intent, at least. On top of recent /proc/pid/*maps* cleanups I sent.
Quoted 6 lines
To me it looks a bit annoying that task_mmu.c needs 6 seq_operations's and 6 file_operations's to handle /proc/pid/*maps*. And _only_ because ->show() differs.
Eric, et al, what do you think? At least something like 1-3 looks like a good cleanup imho. And afaics we can do more cleanups on top.
I see where you are getting annoyed.
Taking a quick look at task_mmu.c It looks like the tgid vs pid logic to decided which stack or stacks to display is simply incorrect.
tgid vs pid is all about do we perform the per thread group rollups or not. Because we have /proc/<tid>/ directories that need the rollups but are per thread.
At a practical level moving pid_entry into the proc inode is ugly especially for the hack that is is_tgid_pid_entry.
That test could be implemented more easily by looking at the parent directories inode operations and seeing if they are proc_root_inode_operations.

Quoted 5 lines

Similarly you can get the names out of the dentry, although comparing on the dentry name feels like a real hack.
Given where you are starting I think tack_mmu.c code that decides when/which stack deserves a serious audit.

On a slightly larger scale it is probably about time to step back and look at /proc and see what structural cleanups can be done.

Since last time I was looking deeply a bunch of work has been done in the automount area, and it would be ever so nice if /proc/<pid>/net because an automount.

Similarly /proc/sys/ really needs to become at least a symlink into /proc/<pid>/sys and quite possibly an automount itself.

And arguably it would be nice to split /proc/<pid> from /proc generic, so we would actually have a pure proc.

Shrug. At least if those are ideas worth thinking about and some of them have the potential to cleanup some nasty interactions with the vfs.

Eric

Oleg Nesterov

author

2014-08-09 14:31 UTC #3 lore raw

Re: [RFC PATCH 1/5] proc: intoduce proc_inode->pid_entry and is_tgid_pid_entry()

In reply to Cyrill Gorcunov

Quoted 17 lines · Cyrill Gorcunov

On Fri, Aug 08, 2014 at 08:57:47PM +0200, Oleg Nesterov wrote:
...
fs/proc/inode.c static struct inode *proc_alloc_inode(struct super_block *sb) · line 79
Quoted 9 lines
+++ b/fs/proc/inode.c
fs/proc/inode.c +1 -0
@@ -73,6 +73,7 @@ static struct inode *proc_alloc_inode(struct super_block *sb)
 	ei->pde = NULL;
 	ei->sysctl = NULL;
 	ei->sysctl_entry = NULL;
+	ei->pid_entry = NULL;
 	ei->ns.ns = NULL;
 	ei->ns.ns_ops = NULL;
 	inode = &ei->vfs_inode;
Hi Oleg, sorry for not responding to previous emails, will try to review this things tomorrow (from a glance looks quite good!). Btw, this moment strike my eyes -- why don't we use kmem_cache_zalloc here but do assign nils again and again, maybe worth to address as well?

Probably because this doesn't make sense to nullify proc_inode->vfs_inode, it will be reinitialized at least by inode_init_always() anyway.

Oleg.

Cyrill Gorcunov

2014-08-10 07:16 UTC #4 lore raw

Re: [RFC PATCH 1/5] proc: intoduce proc_inode->pid_entry and is_tgid_pid_entry()

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines
Hi Oleg, sorry for not responding to previous emails, will try to review this things tomorrow (from a glance looks quite good!). Btw, this moment strike my eyes -- why don't we use kmem_cache_zalloc here but do assign nils again and again, maybe worth to address as well?
Probably because this doesn't make sense to nullify proc_inode->vfs_inode, it will be reinitialized at least by inode_init_always() anyway.

Yeah, thanks. I managed to fogeth that we call inode_init_always after inode
get allocated. As to series -- looks like a good start, and I think Eric is a way more confident in this area than me so i've nothing to add up to his comments.

Oleg Nesterov

author

2014-08-10 19:25 UTC #11 lore raw

In reply to Eric W. Biederman

Quoted 7 lines · Eric W. Biederman

Quoted 4 lines · Oleg Nesterov
To me it looks a bit annoying that task_mmu.c needs 6 seq_operations's and 6 file_operations's to handle /proc/pid/*maps*. And _only_ because ->show() differs.

Quoted 8 lines

Quoted 2 lines
Eric, et al, what do you think? At least something like 1-3 looks like a good cleanup imho. And afaics we can do more cleanups on top.
I see where you are getting annoyed.
Taking a quick look at task_mmu.c It looks like the tgid vs pid logic to decided which stack or stacks to display is simply incorrect.

Yes, probably, but please forget this for now. Because,

Quoted 2 lines

Given where you are starting I think tack_mmu.c code that decides when/which stack deserves a serious audit.

Yes. And more. At least this code needs more cleanups. ->task should die or at least we should avoid get/put_task_struct, ->pid can die too, hold_task_mempolicy() doesn't look correct (at least the "prevent changing our mempolicy while show_numa_maps" comment and down_write() in do_set_mempolicy(). I am going to try to cleanup this a bit after I change task_nommu.c to avoid mm_access() in m_start().

But this is off-topic,

Quoted 2 lines

At a practical level moving pid_entry into the proc inode is ugly especially for the hack that is is_tgid_pid_entry.

Well, I am not going to argue with maintainer, mostly simply because I do not understand this code enough ;)

But let me say that I disagree. We already have ->fd, and ->sysctl*. I see nothing wrong if *id_base_stuff files have more info for free.

And imo proc_inode->sysctl* is much worse. Simply because they are not private to fs/proc/proc_sysctl.c.

Could you please look into the attached mbox? I am just curious if we can do something like this in a clean way. In particular, please look at "Note:" comment in 3/3. Perhaps proc_sys_init() can do proc_get_inode(), initialize/instantiate it...

To clarify, of course it is not that I want to shrink sizeof(proc_inode). Just to me it doesn't look clean that proc_evict_inode() has to do sysctl_head_put(), grab_header() assumes that ->sysctl == NULL means sysctl_table_root.*, etc.

Quoted 3 lines

That test could be implemented more easily by looking at the parent directories inode operations and seeing if they are proc_root_inode_operations.

Hmm. Looking at inode? How?

Oleg.

Quoted 1 line

From a4c94461ae18b2d6cc2e8a9cfb042d0b6cc46e86 Mon Sep 17 00:00:00 2001

From: Oleg Nesterov <[email protected]>
Date: Sat, 9 Aug 2014 17:25:53 +0200
Subject: [PATCH 1/3] proc: introduce proc_sys_evict_inode()

Preparation. Shift the sysctl_head_put() code from proc_evict_inode() into the new trivial helper, make sysctl_head_put() static.

---
 fs/proc/inode.c       |    8 ++------
 fs/proc/internal.h    |    4 ++--
 fs/proc/proc_sysctl.c |   13 ++++++++++++-
 3 files changed, 16 insertions(+), 9 deletions(-)

fs/proc/inode.c +2 -6

index 0adbc02..9f6715a 100644
@@ -31,7 +31,6 @@
 static void proc_evict_inode(struct inode *inode)
 {
 	struct proc_dir_entry *de;
-	struct ctl_table_header *head;
 	const struct proc_ns_operations *ns_ops;
 	void *ns;

@@ -45,11 +44,8 @@ static void proc_evict_inode(struct inode *inode)
 	de = PROC_I(inode)->pde;
 	if (de)
 		pde_put(de);
-	head = PROC_I(inode)->sysctl;
-	if (head) {
-		RCU_INIT_POINTER(PROC_I(inode)->sysctl, NULL);
-		sysctl_head_put(head);
-	}
+
+	proc_sys_evict_inode(inode);
 	/* Release any associated namespace */
 	ns_ops = PROC_I(inode)->ns.ns_ops;
 	ns = PROC_I(inode)->ns.ns;

fs/proc/internal.h +2 -2

index 78784cd..7ce3262 100644
@@ -238,10 +238,10 @@ extern int proc_setup_self(struct super_block *);
  */
 #ifdef CONFIG_PROC_SYSCTL
 extern int proc_sys_init(void);
-extern void sysctl_head_put(struct ctl_table_header *);
+extern void proc_sys_evict_inode(struct inode *inode);
 #else
 static inline void proc_sys_init(void) { }
-static inline void sysctl_head_put(struct ctl_table_header *head) { }
+static inline void proc_sys_evict_inode(struct inode *inode) { }
 #endif
 
 /*

fs/proc/proc_sysctl.c +12 -1

index 7129046..2fa67e7 100644
@@ -256,7 +256,7 @@ static void sysctl_head_get(struct ctl_table_header *head)
 	spin_unlock(&sysctl_lock);
 }
 
-void sysctl_head_put(struct ctl_table_header *head)
+static void sysctl_head_put(struct ctl_table_header *head)
 {
 	spin_lock(&sysctl_lock);
 	if (!--head->count)

@@ -264,6 +264,17 @@ void sysctl_head_put(struct ctl_table_header *head)
 	spin_unlock(&sysctl_lock);
 }
 
+void proc_sys_evict_inode(struct inode *inode)
+{
+	struct ctl_table_header *head;
+
+	head = PROC_I(inode)->sysctl;
+	if (head) {
+		RCU_INIT_POINTER(PROC_I(inode)->sysctl, NULL);
+		sysctl_head_put(head);
+	}
+}
+
 static struct ctl_table_header *sysctl_head_grab(struct ctl_table_header *head)
 {
 	BUG_ON(!head);

Signature · 1 line

1.5.5.1

Quoted 1 line

From e0b42f4770cd76069e4e70e48e4e7bfa84fab4bf Mon Sep 17 00:00:00 2001

From: Oleg Nesterov <[email protected]>
Date: Sat, 9 Aug 2014 17:37:31 +0200
Subject: [PATCH 2/3] proc: change proc_sys_evict_inode() to check inode->i_op

Change proc_sys_evict_inode() to verify that this inode is really a /proc/sys inode before using ->sysctl.

---
 fs/proc/proc_sysctl.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

fs/proc/proc_sysctl.c +4 -0

index 2fa67e7..863aaee 100644
@@ -268,6 +268,10 @@ void proc_sys_evict_inode(struct inode *inode)
 {
 	struct ctl_table_header *head;
 
+	if (inode->i_op != &proc_sys_inode_operations &&
+	    inode->i_op != &proc_sys_dir_operations)
+		return;
+
 	head = PROC_I(inode)->sysctl;
 	if (head) {
 		RCU_INIT_POINTER(PROC_I(inode)->sysctl, NULL);

Signature · 1 line

1.5.5.1

Quoted 1 line

From 9c8e98fc45f091d5d2a4bfc6dcd86b67275160a1 Mon Sep 17 00:00:00 2001

From: Oleg Nesterov <[email protected]>
Date: Sun, 10 Aug 2014 17:37:25 +0200
Subject: [PATCH 3/3] proc: place proc_inode->sysctl* and proc_inode->fd into a union

->sysctl* and ->fd members can share the memory, add a union.

Note: unfortunately proc_alloc_inode() still has to initialize ->sysctl* members. And the only (afaics) reason is that proc_mkdir("sys") relies on ->sysctl == NULL which means sysctl_table_root in grab_header(). It would be nice to initialize these members after proc_get_inode("sys") somehow and remove the special "NULL means global root" case in proc_sys paths somehow, but I do not see how.

---
 fs/proc/internal.h |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

fs/proc/internal.h +8 -4

index 7ce3262..8d35ac0 100644
@@ -60,11 +60,15 @@ union proc_op {
 
 struct proc_inode {
 	struct pid *pid;
-	int fd;
-	union proc_op op;
 	struct proc_dir_entry *pde;
-	struct ctl_table_header *sysctl;
-	struct ctl_table *sysctl_entry;
+	union proc_op op;
+	union {
+		struct {
+			struct ctl_table_header *sysctl;
+			struct ctl_table *sysctl_entry;
+		};
+		int fd;
+	};
 	struct proc_ns ns;
 	struct inode vfs_inode;
 };

Signature · 1 line

1.5.5.1