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 <[email protected]>
---
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(-)
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)
--
2.25.2
On Tue, Mar 24, 2020 at 1:46 PM Alexey Gladkov <[email protected]> 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
Signed-off-by: Alexey Gladkov <[email protected]>
---
fs/proc/base.c | 2 +-
fs/proc/inode.c | 2 +-
fs/proc/root.c | 4 ++--
include/linux/proc_fs.h | 6 +++---
include/uapi/linux/proc_fs.h | 2 +-
5 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index a836979e42fe..608d60fb79fb 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -699,7 +699,7 @@ int proc_setattr(struct dentry *dentry, struct iattr *attr)
*/
static bool has_pid_permissions(struct proc_fs_info *fs_info,
struct task_struct *task,
- int hide_pid_min)
+ enum proc_hidepid hide_pid_min)
{
/*
* If 'hidpid' mount option is set force a ptrace check,
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index a462fd111719..7870e0be0a1f 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -165,7 +165,7 @@ void proc_invalidate_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock
deactivate_super(old_sb);
}
-static inline const char *hidepid2str(int v)
+static inline const char *hidepid2str(enum proc_hidepid v)
{
switch (v) {
case HIDEPID_OFF: return "off";
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 42f3ee05c584..de7cee435621 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -32,9 +32,9 @@
struct proc_fs_context {
struct proc_fs_info *fs_info;
unsigned int mask;
- int hidepid;
+ enum proc_hidepid hidepid;
int gid;
- int pidonly;
+ enum proc_pidonly pidonly;
};
enum proc_param {
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index d259817ec913..b9f7ecd7f61f 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -29,7 +29,7 @@ struct proc_ops {
};
/* definitions for proc mount option pidonly */
-enum {
+enum proc_pidonly {
PROC_PIDONLY_OFF = 0,
PROC_PIDONLY_ON = 1,
};
@@ -39,8 +39,8 @@ struct proc_fs_info {
struct dentry *proc_self; /* For /proc/self */
struct dentry *proc_thread_self; /* For /proc/thread-self */
kgid_t pid_gid;
- int hide_pid;
- int pidonly;
+ enum proc_hidepid hide_pid;
+ enum proc_pidonly pidonly;
};
static inline struct proc_fs_info *proc_sb_info(struct super_block *sb)
diff --git a/include/uapi/linux/proc_fs.h b/include/uapi/linux/proc_fs.h
index dc6d717aa6ec..f5fe0e8dcfe4 100644
--- a/include/uapi/linux/proc_fs.h
+++ b/include/uapi/linux/proc_fs.h
@@ -3,7 +3,7 @@
#define _UAPI_PROC_FS_H
/* definitions for hide_pid field */
-enum {
+enum proc_hidepid {
HIDEPID_OFF = 0,
HIDEPID_NO_ACCESS = 1,
HIDEPID_INVISIBLE = 2,
--
2.25.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 <[email protected]> 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 <[email protected]>
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 <[email protected]> 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.
--
Rgrds, legion
On Wed, Mar 25, 2020 at 06:42:45PM +0100, Alexey Gladkov wrote:
> Signed-off-by: Alexey Gladkov <[email protected]>
I love these kinds of cleanups. :)
Reviewed-by: Kees Cook <[email protected]>
-Kees
> ---
> fs/proc/base.c | 2 +-
> fs/proc/inode.c | 2 +-
> fs/proc/root.c | 4 ++--
> include/linux/proc_fs.h | 6 +++---
> include/uapi/linux/proc_fs.h | 2 +-
> 5 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a836979e42fe..608d60fb79fb 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -699,7 +699,7 @@ int proc_setattr(struct dentry *dentry, struct iattr *attr)
> */
> static bool has_pid_permissions(struct proc_fs_info *fs_info,
> struct task_struct *task,
> - int hide_pid_min)
> + enum proc_hidepid hide_pid_min)
> {
> /*
> * If 'hidpid' mount option is set force a ptrace check,
> diff --git a/fs/proc/inode.c b/fs/proc/inode.c
> index a462fd111719..7870e0be0a1f 100644
> --- a/fs/proc/inode.c
> +++ b/fs/proc/inode.c
> @@ -165,7 +165,7 @@ void proc_invalidate_siblings_dcache(struct hlist_head *inodes, spinlock_t *lock
> deactivate_super(old_sb);
> }
>
> -static inline const char *hidepid2str(int v)
> +static inline const char *hidepid2str(enum proc_hidepid v)
> {
> switch (v) {
> case HIDEPID_OFF: return "off";
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index 42f3ee05c584..de7cee435621 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -32,9 +32,9 @@
> struct proc_fs_context {
> struct proc_fs_info *fs_info;
> unsigned int mask;
> - int hidepid;
> + enum proc_hidepid hidepid;
> int gid;
> - int pidonly;
> + enum proc_pidonly pidonly;
> };
>
> enum proc_param {
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index d259817ec913..b9f7ecd7f61f 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -29,7 +29,7 @@ struct proc_ops {
> };
>
> /* definitions for proc mount option pidonly */
> -enum {
> +enum proc_pidonly {
> PROC_PIDONLY_OFF = 0,
> PROC_PIDONLY_ON = 1,
> };
> @@ -39,8 +39,8 @@ struct proc_fs_info {
> struct dentry *proc_self; /* For /proc/self */
> struct dentry *proc_thread_self; /* For /proc/thread-self */
> kgid_t pid_gid;
> - int hide_pid;
> - int pidonly;
> + enum proc_hidepid hide_pid;
> + enum proc_pidonly pidonly;
> };
>
> static inline struct proc_fs_info *proc_sb_info(struct super_block *sb)
> diff --git a/include/uapi/linux/proc_fs.h b/include/uapi/linux/proc_fs.h
> index dc6d717aa6ec..f5fe0e8dcfe4 100644
> --- a/include/uapi/linux/proc_fs.h
> +++ b/include/uapi/linux/proc_fs.h
> @@ -3,7 +3,7 @@
> #define _UAPI_PROC_FS_H
>
> /* definitions for hide_pid field */
> -enum {
> +enum proc_hidepid {
> HIDEPID_OFF = 0,
> HIDEPID_NO_ACCESS = 1,
> HIDEPID_INVISIBLE = 2,
> --
> 2.25.2
>
--
Kees Cook