[RESEND,v9,3/8] proc: move hide_pid, pid_gid from pid_namespace to proc_fs_info
diff mbox series

Message ID 20200324204449.7263-4-gladkov.alexey@gmail.com
State New
Headers show
Series
  • proc: modernize proc to support multiple private instances
Related show

Commit Message

Alexey Gladkov March 24, 2020, 8:44 p.m. UTC
Move hide_pid and pid_gid parameters inside procfs fs_info struct
instead of making them per pid namespace. Since we have a multiple
procfs instances per pid namespace we need to make sure that all
proc-specific parameters are also per-superblock.

Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
 fs/proc/base.c                | 18 +++++++++---------
 fs/proc/inode.c               |  9 ++++-----
 fs/proc/root.c                |  4 ++--
 include/linux/pid_namespace.h |  8 --------
 include/linux/proc_fs.h       |  9 +++++++++
 5 files changed, 24 insertions(+), 24 deletions(-)

Comments

Linus Torvalds March 24, 2020, 9:21 p.m. UTC | #1
On Tue, Mar 24, 2020 at 1:46 PM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
>
> +/* definitions for hide_pid field */
> +enum {
> +       HIDEPID_OFF       = 0,
> +       HIDEPID_NO_ACCESS = 1,
> +       HIDEPID_INVISIBLE = 2,
> +};

Should this enum be named...

>  struct proc_fs_info {
>         struct pid_namespace *pid_ns;
>         struct dentry *proc_self;        /* For /proc/self */
>         struct dentry *proc_thread_self; /* For /proc/thread-self */
> +       kgid_t pid_gid;
> +       int hide_pid;
>  };

.. and then used here instead of "int"?

Same goes for 'struct proc_fs_context' too, for that matter?

And maybe in the function declarations and definitions too? In things
like 'has_pid_permissions()' (the series adds some other cases later,
like hidepid2str() etc)

Yeah, enums and ints are kind of interchangeable in C, but even if it
wouldn't give us any more typechecking (except perhaps with sparse if
you mark it so), it would be documenting the use.

Or am I missing something?

Anyway, I continue to think the series looks fine, bnut would love to
see it in -next and perhaps comments from Al and Alexey Dobriyan..

            Linus
Alexey Dobriyan March 25, 2020, 6 p.m. UTC | #2
On Tue, Mar 24, 2020 at 02:21:59PM -0700, Linus Torvalds wrote:
> On Tue, Mar 24, 2020 at 1:46 PM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
> >
> > +/* definitions for hide_pid field */
> > +enum {
> > +       HIDEPID_OFF       = 0,
> > +       HIDEPID_NO_ACCESS = 1,
> > +       HIDEPID_INVISIBLE = 2,
> > +};
> 
> Should this enum be named...
> 
> >  struct proc_fs_info {
> >         struct pid_namespace *pid_ns;
> >         struct dentry *proc_self;        /* For /proc/self */
> >         struct dentry *proc_thread_self; /* For /proc/thread-self */
> > +       kgid_t pid_gid;
> > +       int hide_pid;
> >  };
> 
> .. and then used here instead of "int"?
> 
> Same goes for 'struct proc_fs_context' too, for that matter?
> 
> And maybe in the function declarations and definitions too? In things
> like 'has_pid_permissions()' (the series adds some other cases later,
> like hidepid2str() etc)
> 
> Yeah, enums and ints are kind of interchangeable in C, but even if it
> wouldn't give us any more typechecking (except perhaps with sparse if
> you mark it so), it would be documenting the use.
> 
> Or am I missing something?
> 
> Anyway, I continue to think the series looks fine, bnut would love to
> see it in -next and perhaps comments from Al and Alexey Dobriyan..

Patches are OK, except the part where "pid" is named "pidfs" and
the suffix doesn't convey any information.

	mount -t proc -o subset=pid,sysctl,misc

Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>
Alexey Gladkov March 25, 2020, 7:04 p.m. UTC | #3
On Wed, Mar 25, 2020 at 09:00:15PM +0300, Alexey Dobriyan wrote:
> On Tue, Mar 24, 2020 at 02:21:59PM -0700, Linus Torvalds wrote:
> > On Tue, Mar 24, 2020 at 1:46 PM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
> > >
> > > +/* definitions for hide_pid field */
> > > +enum {
> > > +       HIDEPID_OFF       = 0,
> > > +       HIDEPID_NO_ACCESS = 1,
> > > +       HIDEPID_INVISIBLE = 2,
> > > +};
> > 
> > Should this enum be named...
> > 
> > >  struct proc_fs_info {
> > >         struct pid_namespace *pid_ns;
> > >         struct dentry *proc_self;        /* For /proc/self */
> > >         struct dentry *proc_thread_self; /* For /proc/thread-self */
> > > +       kgid_t pid_gid;
> > > +       int hide_pid;
> > >  };
> > 
> > .. and then used here instead of "int"?
> > 
> > Same goes for 'struct proc_fs_context' too, for that matter?
> > 
> > And maybe in the function declarations and definitions too? In things
> > like 'has_pid_permissions()' (the series adds some other cases later,
> > like hidepid2str() etc)
> > 
> > Yeah, enums and ints are kind of interchangeable in C, but even if it
> > wouldn't give us any more typechecking (except perhaps with sparse if
> > you mark it so), it would be documenting the use.
> > 
> > Or am I missing something?
> > 
> > Anyway, I continue to think the series looks fine, bnut would love to
> > see it in -next and perhaps comments from Al and Alexey Dobriyan..
> 
> Patches are OK, except the part where "pid" is named "pidfs" and
> the suffix doesn't convey any information.

I will fix this in the final version.

> 	mount -t proc -o subset=pid,sysctl,misc

I have not yet figured out how to implement this. I mean subset=meminfo,misc.

Patch
diff mbox series

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 9d2e425338c8..984b97bb634b 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -697,13 +697,13 @@  int proc_setattr(struct dentry *dentry, struct iattr *attr)
  * May current process learn task's sched/cmdline info (for hide_pid_min=1)
  * or euid/egid (for hide_pid_min=2)?
  */
-static bool has_pid_permissions(struct pid_namespace *pid,
+static bool has_pid_permissions(struct proc_fs_info *fs_info,
 				 struct task_struct *task,
 				 int hide_pid_min)
 {
-	if (pid->hide_pid < hide_pid_min)
+	if (fs_info->hide_pid < hide_pid_min)
 		return true;
-	if (in_group_p(pid->pid_gid))
+	if (in_group_p(fs_info->pid_gid))
 		return true;
 	return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
 }
@@ -711,18 +711,18 @@  static bool has_pid_permissions(struct pid_namespace *pid,
 
 static int proc_pid_permission(struct inode *inode, int mask)
 {
-	struct pid_namespace *pid = proc_pid_ns(inode);
+	struct proc_fs_info *fs_info = proc_sb_info(inode->i_sb);
 	struct task_struct *task;
 	bool has_perms;
 
 	task = get_proc_task(inode);
 	if (!task)
 		return -ESRCH;
-	has_perms = has_pid_permissions(pid, task, HIDEPID_NO_ACCESS);
+	has_perms = has_pid_permissions(fs_info, task, HIDEPID_NO_ACCESS);
 	put_task_struct(task);
 
 	if (!has_perms) {
-		if (pid->hide_pid == HIDEPID_INVISIBLE) {
+		if (fs_info->hide_pid == HIDEPID_INVISIBLE) {
 			/*
 			 * Let's make getdents(), stat(), and open()
 			 * consistent with each other.  If a process
@@ -1897,7 +1897,7 @@  int pid_getattr(const struct path *path, struct kstat *stat,
 		u32 request_mask, unsigned int query_flags)
 {
 	struct inode *inode = d_inode(path->dentry);
-	struct pid_namespace *pid = proc_pid_ns(inode);
+	struct proc_fs_info *fs_info = proc_sb_info(inode->i_sb);
 	struct task_struct *task;
 
 	generic_fillattr(inode, stat);
@@ -1907,7 +1907,7 @@  int pid_getattr(const struct path *path, struct kstat *stat,
 	rcu_read_lock();
 	task = pid_task(proc_pid(inode), PIDTYPE_PID);
 	if (task) {
-		if (!has_pid_permissions(pid, task, HIDEPID_INVISIBLE)) {
+		if (!has_pid_permissions(fs_info, task, HIDEPID_INVISIBLE)) {
 			rcu_read_unlock();
 			/*
 			 * This doesn't prevent learning whether PID exists,
@@ -3402,7 +3402,7 @@  int proc_pid_readdir(struct file *file, struct dir_context *ctx)
 		unsigned int len;
 
 		cond_resched();
-		if (!has_pid_permissions(ns, iter.task, HIDEPID_INVISIBLE))
+		if (!has_pid_permissions(fs_info, iter.task, HIDEPID_INVISIBLE))
 			continue;
 
 		len = snprintf(name, sizeof(name), "%u", iter.tgid);
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 6e4c6728338b..91fe4896fa85 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -168,12 +168,11 @@  void proc_invalidate_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock
 static int proc_show_options(struct seq_file *seq, struct dentry *root)
 {
 	struct proc_fs_info *fs_info = proc_sb_info(root->d_sb);
-	struct pid_namespace *pid = fs_info->pid_ns;
 
-	if (!gid_eq(pid->pid_gid, GLOBAL_ROOT_GID))
-		seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, pid->pid_gid));
-	if (pid->hide_pid != HIDEPID_OFF)
-		seq_printf(seq, ",hidepid=%u", pid->hide_pid);
+	if (!gid_eq(fs_info->pid_gid, GLOBAL_ROOT_GID))
+		seq_printf(seq, ",gid=%u", from_kgid_munged(&init_user_ns, fs_info->pid_gid));
+	if (fs_info->hide_pid != HIDEPID_OFF)
+		seq_printf(seq, ",hidepid=%u", fs_info->hide_pid);
 
 	return 0;
 }
diff --git a/fs/proc/root.c b/fs/proc/root.c
index b28adbb0b937..616e8976185c 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -85,9 +85,9 @@  static void proc_apply_options(struct super_block *s,
 	struct proc_fs_context *ctx = fc->fs_private;
 
 	if (ctx->mask & (1 << Opt_gid))
-		pid_ns->pid_gid = make_kgid(user_ns, ctx->gid);
+		ctx->fs_info->pid_gid = make_kgid(user_ns, ctx->gid);
 	if (ctx->mask & (1 << Opt_hidepid))
-		pid_ns->hide_pid = ctx->hidepid;
+		ctx->fs_info->hide_pid = ctx->hidepid;
 }
 
 static int proc_fill_super(struct super_block *s, struct fs_context *fc)
diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 4956e362e55e..028d7ba242c6 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -17,12 +17,6 @@ 
 
 struct fs_pin;
 
-enum { /* definitions for pid_namespace's hide_pid field */
-	HIDEPID_OFF	  = 0,
-	HIDEPID_NO_ACCESS = 1,
-	HIDEPID_INVISIBLE = 2,
-};
-
 struct pid_namespace {
 	struct kref kref;
 	struct idr idr;
@@ -41,8 +35,6 @@  struct pid_namespace {
 #endif
 	struct user_namespace *user_ns;
 	struct ucounts *ucounts;
-	kgid_t pid_gid;
-	int hide_pid;
 	int reboot;	/* group exit code if this pidns was rebooted */
 	struct ns_common ns;
 } __randomize_layout;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 5920a4ecd71b..7d852dbca253 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -27,10 +27,19 @@  struct proc_ops {
 	unsigned long (*proc_get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
 };
 
+/* definitions for hide_pid field */
+enum {
+	HIDEPID_OFF	  = 0,
+	HIDEPID_NO_ACCESS = 1,
+	HIDEPID_INVISIBLE = 2,
+};
+
 struct proc_fs_info {
 	struct pid_namespace *pid_ns;
 	struct dentry *proc_self;        /* For /proc/self */
 	struct dentry *proc_thread_self; /* For /proc/thread-self */
+	kgid_t pid_gid;
+	int hide_pid;
 };
 
 static inline struct proc_fs_info *proc_sb_info(struct super_block *sb)