All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/6 v3] proc: clean up /proc/<pid>/environ handling
@ 2012-04-11  5:59 Cong Wang
  2012-04-11  5:59 ` [PATCH 2/6] proc: unify ptrace_may_access locking code Cong Wang
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Cong Wang @ 2012-04-11  5:59 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, Cong Wang, Oleg Nesterov, Alexey Dobriyan,
	Al Viro, Vasiliy Kulikov, David Rientjes

V3: Unify environ_open with mem_open(), and mem_release() too
V2: Add similar fix with 6d08f2c7139790c268820a2e590795cb8333181a
"proc: make sure mem_open() doesn't pin the target's memory",
suggested by Oleg.

Similar to e268337dfe2 ("proc: clean up and fix /proc/<pid>/mem
handling"), move the check of permission to open(), this will simplify
read() code.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 fs/proc/base.c |   45 ++++++++++++++++++++++++---------------------
 1 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 1c8b280..f3c4887 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -677,7 +677,7 @@ static const struct file_operations proc_single_file_operations = {
 	.release	= single_release,
 };
 
-static int mem_open(struct inode* inode, struct file* file)
+static int __mem_open(struct inode* inode, struct file* file, unsigned int mode)
 {
 	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
 	struct mm_struct *mm;
@@ -685,7 +685,7 @@ static int mem_open(struct inode* inode, struct file* file)
 	if (!task)
 		return -ESRCH;
 
-	mm = mm_access(task, PTRACE_MODE_ATTACH);
+	mm = mm_access(task, mode);
 	put_task_struct(task);
 
 	if (IS_ERR(mm))
@@ -705,6 +705,11 @@ static int mem_open(struct inode* inode, struct file* file)
 	return 0;
 }
 
+static int mem_open(struct inode* inode, struct file* file)
+{
+	return __mem_open(inode, file, PTRACE_MODE_ATTACH);
+}
+
 static ssize_t mem_rw(struct file *file, char __user *buf,
 			size_t count, loff_t *ppos, int write)
 {
@@ -801,30 +806,29 @@ static const struct file_operations proc_mem_operations = {
 	.release	= mem_release,
 };
 
+static int environ_open(struct inode* inode, struct file* file)
+{
+	return __mem_open(inode, file, PTRACE_MODE_READ);
+}
+
 static ssize_t environ_read(struct file *file, char __user *buf,
 			size_t count, loff_t *ppos)
 {
-	struct task_struct *task = get_proc_task(file->f_dentry->d_inode);
 	char *page;
 	unsigned long src = *ppos;
-	int ret = -ESRCH;
-	struct mm_struct *mm;
+	int ret = 0;
+	struct mm_struct *mm = file->private_data;
 
-	if (!task)
-		goto out_no_task;
+	if (!mm)
+		return 0;
 
-	ret = -ENOMEM;
 	page = (char *)__get_free_page(GFP_TEMPORARY);
 	if (!page)
-		goto out;
-
-
-	mm = mm_for_maps(task);
-	ret = PTR_ERR(mm);
-	if (!mm || IS_ERR(mm))
-		goto out_free;
+		return -ENOMEM;
 
 	ret = 0;
+	if (!atomic_inc_not_zero(&mm->mm_users))
+		goto free;
 	while (count > 0) {
 		int this_len, retval, max_len;
 
@@ -836,7 +840,7 @@ static ssize_t environ_read(struct file *file, char __user *buf,
 		max_len = (count > PAGE_SIZE) ? PAGE_SIZE : count;
 		this_len = (this_len > max_len) ? max_len : this_len;
 
-		retval = access_process_vm(task, (mm->env_start + src),
+		retval = access_remote_vm(mm, (mm->env_start + src),
 			page, this_len, 0);
 
 		if (retval <= 0) {
@@ -855,19 +859,18 @@ static ssize_t environ_read(struct file *file, char __user *buf,
 		count -= retval;
 	}
 	*ppos = src;
-
 	mmput(mm);
-out_free:
+
+free:
 	free_page((unsigned long) page);
-out:
-	put_task_struct(task);
-out_no_task:
 	return ret;
 }
 
 static const struct file_operations proc_environ_operations = {
+	.open		= environ_open,
 	.read		= environ_read,
 	.llseek		= generic_file_llseek,
+	.release	= mem_release,
 };
 
 static ssize_t oom_adjust_read(struct file *file, char __user *buf,
-- 
1.7.7.6


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 2/6] proc: unify ptrace_may_access locking code
  2012-04-11  5:59 [PATCH 1/6 v3] proc: clean up /proc/<pid>/environ handling Cong Wang
@ 2012-04-11  5:59 ` Cong Wang
  2012-04-11  5:59 ` [PATCH 3/6] proc: remove mm_for_maps() Cong Wang
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Cong Wang @ 2012-04-11  5:59 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, Cong Wang, Oleg Nesterov, Alexey Dobriyan,
	Al Viro, Vasiliy Kulikov, David Rientjes

Unify mutex_lock+ptrace_may_access code and rename lock_trace()
to task_access_lock(), which better describes what it does.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 fs/proc/base.c |   36 +++++++++++++++---------------------
 1 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index f3c4887..919d14c 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -281,19 +281,19 @@ static int proc_pid_wchan(struct task_struct *task, char *buffer)
 }
 #endif /* CONFIG_KALLSYMS */
 
-static int lock_trace(struct task_struct *task)
+static int task_access_lock(struct task_struct *task, unsigned int mode)
 {
 	int err = mutex_lock_killable(&task->signal->cred_guard_mutex);
 	if (err)
 		return err;
-	if (!ptrace_may_access(task, PTRACE_MODE_ATTACH)) {
+	if (!ptrace_may_access(task, mode)) {
 		mutex_unlock(&task->signal->cred_guard_mutex);
 		return -EPERM;
 	}
 	return 0;
 }
 
-static void unlock_trace(struct task_struct *task)
+static void task_access_unlock(struct task_struct *task)
 {
 	mutex_unlock(&task->signal->cred_guard_mutex);
 }
@@ -319,7 +319,7 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 	trace.entries		= entries;
 	trace.skip		= 0;
 
-	err = lock_trace(task);
+	err = task_access_lock(task, PTRACE_MODE_ATTACH);
 	if (!err) {
 		save_stack_trace_tsk(task, &trace);
 
@@ -327,7 +327,7 @@ static int proc_pid_stack(struct seq_file *m, struct pid_namespace *ns,
 			seq_printf(m, "[<%pK>] %pS\n",
 				   (void *)entries[i], (void *)entries[i]);
 		}
-		unlock_trace(task);
+		task_access_unlock(task);
 	}
 	kfree(entries);
 
@@ -494,7 +494,7 @@ static int proc_pid_syscall(struct task_struct *task, char *buffer)
 {
 	long nr;
 	unsigned long args[6], sp, pc;
-	int res = lock_trace(task);
+	int res = task_access_lock(task, PTRACE_MODE_ATTACH);
 	if (res)
 		return res;
 
@@ -508,7 +508,7 @@ static int proc_pid_syscall(struct task_struct *task, char *buffer)
 		       nr,
 		       args[0], args[1], args[2], args[3], args[4], args[5],
 		       sp, pc);
-	unlock_trace(task);
+	task_access_unlock(task);
 	return res;
 }
 #endif /* CONFIG_HAVE_ARCH_TRACEHOOK */
@@ -2180,7 +2180,7 @@ static struct dentry *proc_map_files_lookup(struct inode *dir,
 		goto out;
 
 	result = ERR_PTR(-EACCES);
-	if (lock_trace(task))
+	if (task_access_lock(task, PTRACE_MODE_ATTACH))
 		goto out_put_task;
 
 	result = ERR_PTR(-ENOENT);
@@ -2202,7 +2202,7 @@ out_no_vma:
 	up_read(&mm->mmap_sem);
 	mmput(mm);
 out_unlock:
-	unlock_trace(task);
+	task_access_unlock(task);
 out_put_task:
 	put_task_struct(task);
 out:
@@ -2236,7 +2236,7 @@ proc_map_files_readdir(struct file *filp, void *dirent, filldir_t filldir)
 		goto out;
 
 	ret = -EACCES;
-	if (lock_trace(task))
+	if (task_access_lock(task, PTRACE_MODE_ATTACH))
 		goto out_put_task;
 
 	ret = 0;
@@ -2336,7 +2336,7 @@ proc_map_files_readdir(struct file *filp, void *dirent, filldir_t filldir)
 	}
 
 out_unlock:
-	unlock_trace(task);
+	task_access_unlock(task);
 out_put_task:
 	put_task_struct(task);
 out:
@@ -2897,15 +2897,10 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 	unsigned long flags;
 	int result;
 
-	result = mutex_lock_killable(&task->signal->cred_guard_mutex);
+	result = task_access_lock(task, PTRACE_MODE_READ);
 	if (result)
 		return result;
 
-	if (!ptrace_may_access(task, PTRACE_MODE_READ)) {
-		result = -EACCES;
-		goto out_unlock;
-	}
-
 	if (whole && lock_task_sighand(task, &flags)) {
 		struct task_struct *t = task;
 
@@ -2930,8 +2925,7 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
 			(unsigned long long)acct.read_bytes,
 			(unsigned long long)acct.write_bytes,
 			(unsigned long long)acct.cancelled_write_bytes);
-out_unlock:
-	mutex_unlock(&task->signal->cred_guard_mutex);
+	task_access_unlock(task);
 	return result;
 }
 
@@ -2949,10 +2943,10 @@ static int proc_tgid_io_accounting(struct task_struct *task, char *buffer)
 static int proc_pid_personality(struct seq_file *m, struct pid_namespace *ns,
 				struct pid *pid, struct task_struct *task)
 {
-	int err = lock_trace(task);
+	int err = task_access_lock(task, PTRACE_MODE_ATTACH);
 	if (!err) {
 		seq_printf(m, "%08x\n", task->personality);
-		unlock_trace(task);
+		task_access_unlock(task);
 	}
 	return err;
 }
-- 
1.7.7.6


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 3/6] proc: remove mm_for_maps()
  2012-04-11  5:59 [PATCH 1/6 v3] proc: clean up /proc/<pid>/environ handling Cong Wang
  2012-04-11  5:59 ` [PATCH 2/6] proc: unify ptrace_may_access locking code Cong Wang
@ 2012-04-11  5:59 ` Cong Wang
  2012-04-12  0:38   ` Hugh Dickins
  2012-04-11  5:59 ` [PATCH 4/6] proc: use mm_access() instead of ptrace_may_access() Cong Wang
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Cong Wang @ 2012-04-11  5:59 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, Cong Wang, Oleg Nesterov, Alexey Dobriyan,
	Al Viro, Vasiliy Kulikov, David Rientjes, Eric W. Biederman,
	Stephen Wilson, Siddhesh Poyarekar, Hugh Dickins,
	KOSAKI Motohiro, Naoya Horiguchi

mm_for_maps() is a simple wrapper for mm_access(),
and the name is misleading, so just remove it and use
mm_access() directly.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 fs/proc/base.c       |    7 +------
 fs/proc/internal.h   |    2 --
 fs/proc/task_mmu.c   |    4 ++--
 fs/proc/task_nommu.c |    2 +-
 4 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 919d14c..8608a4d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -198,11 +198,6 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
 	return result;
 }
 
-struct mm_struct *mm_for_maps(struct task_struct *task)
-{
-	return mm_access(task, PTRACE_MODE_READ);
-}
-
 static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 {
 	int res = 0;
@@ -242,7 +237,7 @@ out:
 
 static int proc_pid_auxv(struct task_struct *task, char *buffer)
 {
-	struct mm_struct *mm = mm_for_maps(task);
+	struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ);
 	int res = PTR_ERR(mm);
 	if (mm && !IS_ERR(mm)) {
 		unsigned int nwords = 0;
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 5f79bb8..a306437 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -31,8 +31,6 @@ struct vmalloc_info {
 	unsigned long	largest_chunk;
 };
 
-extern struct mm_struct *mm_for_maps(struct task_struct *);
-
 #ifdef CONFIG_MMU
 #define VMALLOC_TOTAL (VMALLOC_END - VMALLOC_START)
 extern void get_vmalloc_info(struct vmalloc_info *vmi);
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 2b9a760..03360a1 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -125,7 +125,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 	if (!priv->task)
 		return ERR_PTR(-ESRCH);
 
-	mm = mm_for_maps(priv->task);
+	mm = mm_access(priv->task, PTRACE_MODE_READ);
 	if (!mm || IS_ERR(mm))
 		return mm;
 	down_read(&mm->mmap_sem);
@@ -914,7 +914,7 @@ static ssize_t pagemap_read(struct file *file, char __user *buf,
 	if (!pm.buffer)
 		goto out_task;
 
-	mm = mm_for_maps(task);
+	mm = mm_access(task, PTRACE_MODE_READ);
 	ret = PTR_ERR(mm);
 	if (!mm || IS_ERR(mm))
 		goto out_free;
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 74fe164..1ccfa53 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -223,7 +223,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 	if (!priv->task)
 		return ERR_PTR(-ESRCH);
 
-	mm = mm_for_maps(priv->task);
+	mm = mm_access(priv->task, PTRACE_MODE_READ);
 	if (!mm || IS_ERR(mm)) {
 		put_task_struct(priv->task);
 		priv->task = NULL;
-- 
1.7.7.6


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 4/6] proc: use mm_access() instead of ptrace_may_access()
  2012-04-11  5:59 [PATCH 1/6 v3] proc: clean up /proc/<pid>/environ handling Cong Wang
  2012-04-11  5:59 ` [PATCH 2/6] proc: unify ptrace_may_access locking code Cong Wang
  2012-04-11  5:59 ` [PATCH 3/6] proc: remove mm_for_maps() Cong Wang
@ 2012-04-11  5:59 ` Cong Wang
  2012-04-11  5:59 ` [PATCH 5/6] proc: use task_access_lock() " Cong Wang
  2012-04-11  5:59 ` [PATCH 6/6] proc: use IS_ERR_OR_NULL() Cong Wang
  4 siblings, 0 replies; 15+ messages in thread
From: Cong Wang @ 2012-04-11  5:59 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, Cong Wang, Oleg Nesterov, Alexey Dobriyan,
	Al Viro, Vasiliy Kulikov, David Rientjes

mm_access() handles this much better, and avoids some race conditions.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 fs/proc/base.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 8608a4d..f1d18fc 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2035,11 +2035,8 @@ static int map_files_d_revalidate(struct dentry *dentry, struct nameidata *nd)
 	if (!task)
 		goto out_notask;
 
-	if (!ptrace_may_access(task, PTRACE_MODE_READ))
-		goto out;
-
-	mm = get_task_mm(task);
-	if (!mm)
+	mm = mm_access(task, PTRACE_MODE_READ);
+	if (IS_ERR_OR_NULL(mm))
 		goto out;
 
 	if (!dname_to_vma_addr(dentry, &vm_start, &vm_end)) {
-- 
1.7.7.6


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 5/6] proc: use task_access_lock() instead of ptrace_may_access()
  2012-04-11  5:59 [PATCH 1/6 v3] proc: clean up /proc/<pid>/environ handling Cong Wang
                   ` (2 preceding siblings ...)
  2012-04-11  5:59 ` [PATCH 4/6] proc: use mm_access() instead of ptrace_may_access() Cong Wang
@ 2012-04-11  5:59 ` Cong Wang
  2012-04-12 13:22   ` Cong Wang
  2012-04-11  5:59 ` [PATCH 6/6] proc: use IS_ERR_OR_NULL() Cong Wang
  4 siblings, 1 reply; 15+ messages in thread
From: Cong Wang @ 2012-04-11  5:59 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, Cong Wang, Oleg Nesterov, Alexey Dobriyan,
	Al Viro, Vasiliy Kulikov, David Rientjes

There are several places in fs/proc/base.c still use ptrace_may_access()
directly to check the permission, actually this just gets a snapshot of
the permission, nothing prevents the target task from raising the priviledges
itself, it is better to use task_access_lock() for these places, to hold
the priviledges.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 fs/proc/base.c |   74 +++++++++++++++++++++++++-------------------------------
 1 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index f1d18fc..2dff86b 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -253,6 +253,23 @@ static int proc_pid_auxv(struct task_struct *task, char *buffer)
 	return res;
 }
 
+static int task_access_lock(struct task_struct *task, unsigned int mode)
+
+{
+	int err = mutex_lock_killable(&task->signal->cred_guard_mutex);
+	if (err)
+		return err;
+	if (!ptrace_may_access(task, mode)) {
+		mutex_unlock(&task->signal->cred_guard_mutex);
+		return -EPERM;
+	}
+	return 0;
+}
+
+static void task_access_unlock(struct task_struct *task)
+{
+	mutex_unlock(&task->signal->cred_guard_mutex);
+}
 
 #ifdef CONFIG_KALLSYMS
 /*
@@ -264,35 +281,18 @@ static int proc_pid_wchan(struct task_struct *task, char *buffer)
 	unsigned long wchan;
 	char symname[KSYM_NAME_LEN];
 
+	if (task_access_lock(task, PTRACE_MODE_READ))
+		return 0;
 	wchan = get_wchan(task);
 
+	task_access_unlock(task);
 	if (lookup_symbol_name(wchan, symname) < 0)
-		if (!ptrace_may_access(task, PTRACE_MODE_READ))
-			return 0;
-		else
-			return sprintf(buffer, "%lu", wchan);
+		return sprintf(buffer, "%lu", wchan);
 	else
 		return sprintf(buffer, "%s", symname);
 }
 #endif /* CONFIG_KALLSYMS */
 
-static int task_access_lock(struct task_struct *task, unsigned int mode)
-{
-	int err = mutex_lock_killable(&task->signal->cred_guard_mutex);
-	if (err)
-		return err;
-	if (!ptrace_may_access(task, mode)) {
-		mutex_unlock(&task->signal->cred_guard_mutex);
-		return -EPERM;
-	}
-	return 0;
-}
-
-static void task_access_unlock(struct task_struct *task)
-{
-	mutex_unlock(&task->signal->cred_guard_mutex);
-}
-
 #ifdef CONFIG_STACKTRACE
 
 #define MAX_STACK_TRACE_DEPTH	64
@@ -512,23 +512,6 @@ static int proc_pid_syscall(struct task_struct *task, char *buffer)
 /*                       Here the fs part begins                        */
 /************************************************************************/
 
-/* permission checks */
-static int proc_fd_access_allowed(struct inode *inode)
-{
-	struct task_struct *task;
-	int allowed = 0;
-	/* Allow access to a task's file descriptors if it is us or we
-	 * may use ptrace attach to the process and find out that
-	 * information.
-	 */
-	task = get_proc_task(inode);
-	if (task) {
-		allowed = ptrace_may_access(task, PTRACE_MODE_READ);
-		put_task_struct(task);
-	}
-	return allowed;
-}
-
 int proc_setattr(struct dentry *dentry, struct iattr *attr)
 {
 	int error;
@@ -1425,17 +1408,21 @@ static int proc_exe_link(struct dentry *dentry, struct path *exe_path)
 static void *proc_pid_follow_link(struct dentry *dentry, struct nameidata *nd)
 {
 	struct inode *inode = dentry->d_inode;
+	struct task_struct *task = get_proc_task(inode);
 	int error = -EACCES;
 
 	/* We don't need a base pointer in the /proc filesystem */
 	path_put(&nd->path);
 
 	/* Are we allowed to snoop on the tasks file descriptors? */
-	if (!proc_fd_access_allowed(inode))
+	error = task_access_lock(task, PTRACE_MODE_READ);
+	if (error)
 		goto out;
 
 	error = PROC_I(inode)->op.proc_get_link(dentry, &nd->path);
+	task_access_unlock(task);
 out:
+	put_task_struct(task);
 	return ERR_PTR(error);
 }
 
@@ -1467,19 +1454,24 @@ static int proc_pid_readlink(struct dentry * dentry, char __user * buffer, int b
 {
 	int error = -EACCES;
 	struct inode *inode = dentry->d_inode;
+	struct task_struct *task = get_proc_task(inode);
 	struct path path;
 
 	/* Are we allowed to snoop on the tasks file descriptors? */
-	if (!proc_fd_access_allowed(inode))
+	error = task_access_lock(task, PTRACE_MODE_READ);
+	if (error)
 		goto out;
 
 	error = PROC_I(inode)->op.proc_get_link(dentry, &path);
 	if (error)
-		goto out;
+		goto out_unlock;
 
 	error = do_proc_readlink(&path, buffer, buflen);
 	path_put(&path);
+out_unlock:
+	task_access_unlock(task);
 out:
+	put_task_struct(task);
 	return error;
 }
 
-- 
1.7.7.6


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 6/6] proc: use IS_ERR_OR_NULL()
  2012-04-11  5:59 [PATCH 1/6 v3] proc: clean up /proc/<pid>/environ handling Cong Wang
                   ` (3 preceding siblings ...)
  2012-04-11  5:59 ` [PATCH 5/6] proc: use task_access_lock() " Cong Wang
@ 2012-04-11  5:59 ` Cong Wang
  2012-04-12  0:41   ` Hugh Dickins
  2012-04-12 13:23   ` Alexey Dobriyan
  4 siblings, 2 replies; 15+ messages in thread
From: Cong Wang @ 2012-04-11  5:59 UTC (permalink / raw)
  To: linux-kernel
  Cc: Andrew Morton, Cong Wang, Oleg Nesterov, Alexey Dobriyan,
	Al Viro, Vasiliy Kulikov, David Rientjes, Hugh Dickins,
	KOSAKI Motohiro, Naoya Horiguchi, Siddhesh Poyarekar

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 fs/proc/base.c       |    2 +-
 fs/proc/task_mmu.c   |    4 ++--
 fs/proc/task_nommu.c |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2dff86b..a72a409 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -239,7 +239,7 @@ static int proc_pid_auxv(struct task_struct *task, char *buffer)
 {
 	struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ);
 	int res = PTR_ERR(mm);
-	if (mm && !IS_ERR(mm)) {
+	if (!IS_ERR_OR_NULL(mm)) {
 		unsigned int nwords = 0;
 		do {
 			nwords += 2;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 03360a1..da4cf5f 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -126,7 +126,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 		return ERR_PTR(-ESRCH);
 
 	mm = mm_access(priv->task, PTRACE_MODE_READ);
-	if (!mm || IS_ERR(mm))
+	if (IS_ERR_OR_NULL(mm))
 		return mm;
 	down_read(&mm->mmap_sem);
 
@@ -916,7 +916,7 @@ static ssize_t pagemap_read(struct file *file, char __user *buf,
 
 	mm = mm_access(task, PTRACE_MODE_READ);
 	ret = PTR_ERR(mm);
-	if (!mm || IS_ERR(mm))
+	if (IS_ERR_OR_NULL(mm))
 		goto out_free;
 
 	pagemap_walk.pmd_entry = pagemap_pte_range;
diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
index 1ccfa53..57ad3cf 100644
--- a/fs/proc/task_nommu.c
+++ b/fs/proc/task_nommu.c
@@ -224,7 +224,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
 		return ERR_PTR(-ESRCH);
 
 	mm = mm_access(priv->task, PTRACE_MODE_READ);
-	if (!mm || IS_ERR(mm)) {
+	if (IS_ERR_OR_NULL(mm)) {
 		put_task_struct(priv->task);
 		priv->task = NULL;
 		return mm;
-- 
1.7.7.6


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/6] proc: remove mm_for_maps()
  2012-04-11  5:59 ` [PATCH 3/6] proc: remove mm_for_maps() Cong Wang
@ 2012-04-12  0:38   ` Hugh Dickins
  0 siblings, 0 replies; 15+ messages in thread
From: Hugh Dickins @ 2012-04-12  0:38 UTC (permalink / raw)
  To: Cong Wang
  Cc: linux-kernel, Andrew Morton, Oleg Nesterov, Alexey Dobriyan,
	Al Viro, Vasiliy Kulikov, David Rientjes, Eric W. Biederman,
	Stephen Wilson, Siddhesh Poyarekar, KOSAKI Motohiro,
	Naoya Horiguchi

On Wed, 11 Apr 2012, Cong Wang wrote:

> mm_for_maps() is a simple wrapper for mm_access(),
> and the name is misleading, so just remove it and use
> mm_access() directly.
> 
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Yes, this is an improvement in my eyes.

Acked-by: Hugh Dickins <hughd@google.com>

> ---
>  fs/proc/base.c       |    7 +------
>  fs/proc/internal.h   |    2 --
>  fs/proc/task_mmu.c   |    4 ++--
>  fs/proc/task_nommu.c |    2 +-
>  4 files changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 919d14c..8608a4d 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -198,11 +198,6 @@ static int proc_root_link(struct dentry *dentry, struct path *path)
>  	return result;
>  }
>  
> -struct mm_struct *mm_for_maps(struct task_struct *task)
> -{
> -	return mm_access(task, PTRACE_MODE_READ);
> -}
> -
>  static int proc_pid_cmdline(struct task_struct *task, char * buffer)
>  {
>  	int res = 0;
> @@ -242,7 +237,7 @@ out:
>  
>  static int proc_pid_auxv(struct task_struct *task, char *buffer)
>  {
> -	struct mm_struct *mm = mm_for_maps(task);
> +	struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ);
>  	int res = PTR_ERR(mm);
>  	if (mm && !IS_ERR(mm)) {
>  		unsigned int nwords = 0;
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 5f79bb8..a306437 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -31,8 +31,6 @@ struct vmalloc_info {
>  	unsigned long	largest_chunk;
>  };
>  
> -extern struct mm_struct *mm_for_maps(struct task_struct *);
> -
>  #ifdef CONFIG_MMU
>  #define VMALLOC_TOTAL (VMALLOC_END - VMALLOC_START)
>  extern void get_vmalloc_info(struct vmalloc_info *vmi);
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 2b9a760..03360a1 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -125,7 +125,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
>  	if (!priv->task)
>  		return ERR_PTR(-ESRCH);
>  
> -	mm = mm_for_maps(priv->task);
> +	mm = mm_access(priv->task, PTRACE_MODE_READ);
>  	if (!mm || IS_ERR(mm))
>  		return mm;
>  	down_read(&mm->mmap_sem);
> @@ -914,7 +914,7 @@ static ssize_t pagemap_read(struct file *file, char __user *buf,
>  	if (!pm.buffer)
>  		goto out_task;
>  
> -	mm = mm_for_maps(task);
> +	mm = mm_access(task, PTRACE_MODE_READ);
>  	ret = PTR_ERR(mm);
>  	if (!mm || IS_ERR(mm))
>  		goto out_free;
> diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
> index 74fe164..1ccfa53 100644
> --- a/fs/proc/task_nommu.c
> +++ b/fs/proc/task_nommu.c
> @@ -223,7 +223,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
>  	if (!priv->task)
>  		return ERR_PTR(-ESRCH);
>  
> -	mm = mm_for_maps(priv->task);
> +	mm = mm_access(priv->task, PTRACE_MODE_READ);
>  	if (!mm || IS_ERR(mm)) {
>  		put_task_struct(priv->task);
>  		priv->task = NULL;
> -- 
> 1.7.7.6
> 
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 6/6] proc: use IS_ERR_OR_NULL()
  2012-04-11  5:59 ` [PATCH 6/6] proc: use IS_ERR_OR_NULL() Cong Wang
@ 2012-04-12  0:41   ` Hugh Dickins
  2012-04-12 13:17     ` Cong Wang
  2012-04-12 13:23   ` Alexey Dobriyan
  1 sibling, 1 reply; 15+ messages in thread
From: Hugh Dickins @ 2012-04-12  0:41 UTC (permalink / raw)
  To: Cong Wang
  Cc: linux-kernel, Andrew Morton, Oleg Nesterov, Alexey Dobriyan,
	Al Viro, Vasiliy Kulikov, David Rientjes, KOSAKI Motohiro,
	Naoya Horiguchi, Siddhesh Poyarekar

On Wed, 11 Apr 2012, Cong Wang wrote:

> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Whereas this one, I don't object, but I don't see it as an improvement.

> ---
>  fs/proc/base.c       |    2 +-
>  fs/proc/task_mmu.c   |    4 ++--
>  fs/proc/task_nommu.c |    2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 2dff86b..a72a409 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -239,7 +239,7 @@ static int proc_pid_auxv(struct task_struct *task, char *buffer)
>  {
>  	struct mm_struct *mm = mm_access(task, PTRACE_MODE_READ);
>  	int res = PTR_ERR(mm);
> -	if (mm && !IS_ERR(mm)) {
> +	if (!IS_ERR_OR_NULL(mm)) {
>  		unsigned int nwords = 0;
>  		do {
>  			nwords += 2;
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 03360a1..da4cf5f 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -126,7 +126,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
>  		return ERR_PTR(-ESRCH);
>  
>  	mm = mm_access(priv->task, PTRACE_MODE_READ);
> -	if (!mm || IS_ERR(mm))
> +	if (IS_ERR_OR_NULL(mm))
>  		return mm;
>  	down_read(&mm->mmap_sem);
>  
> @@ -916,7 +916,7 @@ static ssize_t pagemap_read(struct file *file, char __user *buf,
>  
>  	mm = mm_access(task, PTRACE_MODE_READ);
>  	ret = PTR_ERR(mm);
> -	if (!mm || IS_ERR(mm))
> +	if (IS_ERR_OR_NULL(mm))
>  		goto out_free;
>  
>  	pagemap_walk.pmd_entry = pagemap_pte_range;
> diff --git a/fs/proc/task_nommu.c b/fs/proc/task_nommu.c
> index 1ccfa53..57ad3cf 100644
> --- a/fs/proc/task_nommu.c
> +++ b/fs/proc/task_nommu.c
> @@ -224,7 +224,7 @@ static void *m_start(struct seq_file *m, loff_t *pos)
>  		return ERR_PTR(-ESRCH);
>  
>  	mm = mm_access(priv->task, PTRACE_MODE_READ);
> -	if (!mm || IS_ERR(mm)) {
> +	if (IS_ERR_OR_NULL(mm)) {
>  		put_task_struct(priv->task);
>  		priv->task = NULL;
>  		return mm;
> -- 
> 1.7.7.6
> 
> 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 6/6] proc: use IS_ERR_OR_NULL()
  2012-04-12  0:41   ` Hugh Dickins
@ 2012-04-12 13:17     ` Cong Wang
  0 siblings, 0 replies; 15+ messages in thread
From: Cong Wang @ 2012-04-12 13:17 UTC (permalink / raw)
  To: Hugh Dickins
  Cc: linux-kernel, Andrew Morton, Oleg Nesterov, Alexey Dobriyan,
	Al Viro, Vasiliy Kulikov, David Rientjes, KOSAKI Motohiro,
	Naoya Horiguchi, Siddhesh Poyarekar

On 04/12/2012 08:41 AM, Hugh Dickins wrote:
> On Wed, 11 Apr 2012, Cong Wang wrote:
>
>> Cc: Oleg Nesterov<oleg@redhat.com>
>> Cc: Alexey Dobriyan<adobriyan@gmail.com>
>> Signed-off-by: Cong Wang<xiyou.wangcong@gmail.com>
>
> Whereas this one, I don't object, but I don't see it as an improvement.
>

It is not am improvement, it is a trivial patch to use IS_ERR_OR_NULL().

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/6] proc: use task_access_lock() instead of ptrace_may_access()
  2012-04-11  5:59 ` [PATCH 5/6] proc: use task_access_lock() " Cong Wang
@ 2012-04-12 13:22   ` Cong Wang
  2012-04-12 15:29     ` Djalal Harouni
  0 siblings, 1 reply; 15+ messages in thread
From: Cong Wang @ 2012-04-12 13:22 UTC (permalink / raw)
  To: Cong Wang
  Cc: linux-kernel, Andrew Morton, Oleg Nesterov, Alexey Dobriyan,
	Al Viro, Vasiliy Kulikov, David Rientjes

On 04/11/2012 01:59 PM, Cong Wang wrote:
> There are several places in fs/proc/base.c still use ptrace_may_access()
> directly to check the permission, actually this just gets a snapshot of
> the permission, nothing prevents the target task from raising the priviledges
> itself, it is better to use task_access_lock() for these places, to hold
> the priviledges.
>
Hi, Andrew,

Please drop this patch, it introduces a deadlock when execve() a 
/proc/<pid>/exec file, and it is not a big improvement nor fixes any 
bugs, so let's just drop this one.

Thanks!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 6/6] proc: use IS_ERR_OR_NULL()
  2012-04-11  5:59 ` [PATCH 6/6] proc: use IS_ERR_OR_NULL() Cong Wang
  2012-04-12  0:41   ` Hugh Dickins
@ 2012-04-12 13:23   ` Alexey Dobriyan
  2012-04-12 18:46     ` KOSAKI Motohiro
  1 sibling, 1 reply; 15+ messages in thread
From: Alexey Dobriyan @ 2012-04-12 13:23 UTC (permalink / raw)
  To: Cong Wang
  Cc: linux-kernel, Andrew Morton, Oleg Nesterov, Al Viro,
	Vasiliy Kulikov, David Rientjes, Hugh Dickins, KOSAKI Motohiro,
	Naoya Horiguchi, Siddhesh Poyarekar

On Wed, Apr 11, 2012 at 8:59 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> -       if (mm && !IS_ERR(mm)) {
> +       if (!IS_ERR_OR_NULL(mm)) {

I personally find original code way more readable.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/6] proc: use task_access_lock() instead of ptrace_may_access()
  2012-04-12 13:22   ` Cong Wang
@ 2012-04-12 15:29     ` Djalal Harouni
  0 siblings, 0 replies; 15+ messages in thread
From: Djalal Harouni @ 2012-04-12 15:29 UTC (permalink / raw)
  To: Cong Wang
  Cc: linux-kernel, Andrew Morton, Oleg Nesterov, Alexey Dobriyan,
	Al Viro, Vasiliy Kulikov, David Rientjes

Hi Cong,

On Thu, Apr 12, 2012 at 09:22:10PM +0800, Cong Wang wrote:
> On 04/11/2012 01:59 PM, Cong Wang wrote:
> > There are several places in fs/proc/base.c still use ptrace_may_access()
> > directly to check the permission, actually this just gets a snapshot of
> > the permission, nothing prevents the target task from raising the priviledges
> > itself, it is better to use task_access_lock() for these places, to hold
> > the priviledges.
> >
> Hi, Andrew,
> 
> Please drop this patch, it introduces a deadlock when execve() a 
> /proc/<pid>/exec file, and it is not a big improvement nor fixes any 
> bugs, so let's just drop this one.
I was going to ask about this since it seems that abusing lock_trace()
or task_access_lock() can cause problems.

Please see commit 5e442a493fc59f which reverts commit aa6afca5bcaba8101f
that tries to protect /proc/PID/fd** files


IMHO A solution for some of the simple /proc/<pid>/* files is to use
mm_access() check just after gathering data and before returning it to
userspace.

So IMO the original code of proc_pid_wchan() was correct, since that data
is not copied to userspace directly, and we can avoid the mm_access() and
the task->signal->cred_guard_mutex lock since we do not race against them,
we have already grabbed the 'wchan', a simple ptrace_may_access() check will
do the job.

(I guess there is a window against another execve and ptrace_may_access()
but that returned data is not useful anymore, is it ?).


For others I don't know what would be the best solution.

Thanks.

> Thanks!
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
tixxdz
http://opendz.org

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 6/6] proc: use IS_ERR_OR_NULL()
  2012-04-12 13:23   ` Alexey Dobriyan
@ 2012-04-12 18:46     ` KOSAKI Motohiro
  2012-04-12 19:26       ` Hugh Dickins
  0 siblings, 1 reply; 15+ messages in thread
From: KOSAKI Motohiro @ 2012-04-12 18:46 UTC (permalink / raw)
  To: Alexey Dobriyan
  Cc: Cong Wang, linux-kernel, Andrew Morton, Oleg Nesterov, Al Viro,
	Vasiliy Kulikov, David Rientjes, Hugh Dickins, KOSAKI Motohiro,
	Naoya Horiguchi, Siddhesh Poyarekar, kosaki.motohiro

(4/12/12 9:23 AM), Alexey Dobriyan wrote:
> On Wed, Apr 11, 2012 at 8:59 AM, Cong Wang<xiyou.wangcong@gmail.com>  wrote:
>> -       if (mm&&  !IS_ERR(mm)) {
>> +       if (!IS_ERR_OR_NULL(mm)) {
>
> I personally find original code way more readable.

personally, me too. but new one is also acceptable to me.

thanks.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 6/6] proc: use IS_ERR_OR_NULL()
  2012-04-12 18:46     ` KOSAKI Motohiro
@ 2012-04-12 19:26       ` Hugh Dickins
  2012-04-12 19:31         ` Andrew Morton
  0 siblings, 1 reply; 15+ messages in thread
From: Hugh Dickins @ 2012-04-12 19:26 UTC (permalink / raw)
  To: KOSAKI Motohiro
  Cc: Alexey Dobriyan, Cong Wang, linux-kernel, Andrew Morton,
	Oleg Nesterov, Al Viro, Vasiliy Kulikov, David Rientjes,
	KOSAKI Motohiro, Naoya Horiguchi, Siddhesh Poyarekar

On Thu, 12 Apr 2012, KOSAKI Motohiro wrote:
> (4/12/12 9:23 AM), Alexey Dobriyan wrote:
> > On Wed, Apr 11, 2012 at 8:59 AM, Cong Wang<xiyou.wangcong@gmail.com>
> > wrote:
> > > -       if (mm&&  !IS_ERR(mm)) {
> > > +       if (!IS_ERR_OR_NULL(mm)) {
> > 
> > I personally find original code way more readable.
> 
> personally, me too. but new one is also acceptable to me.

That now makes 4 votes that it's not an improvement, with no dissent
(unless akpm's + proc-use-is_err_or_null.patch added to -mm tree
should be counted as dissent): let's drop this patch.

Hugh

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 6/6] proc: use IS_ERR_OR_NULL()
  2012-04-12 19:26       ` Hugh Dickins
@ 2012-04-12 19:31         ` Andrew Morton
  0 siblings, 0 replies; 15+ messages in thread
From: Andrew Morton @ 2012-04-12 19:31 UTC (permalink / raw)
  To: Hugh Dickins
  Cc: KOSAKI Motohiro, Alexey Dobriyan, Cong Wang, linux-kernel,
	Oleg Nesterov, Al Viro, Vasiliy Kulikov, David Rientjes,
	KOSAKI Motohiro, Naoya Horiguchi, Siddhesh Poyarekar

On Thu, 12 Apr 2012 12:26:51 -0700 (PDT)
Hugh Dickins <hughd@google.com> wrote:

> On Thu, 12 Apr 2012, KOSAKI Motohiro wrote:
> > (4/12/12 9:23 AM), Alexey Dobriyan wrote:
> > > On Wed, Apr 11, 2012 at 8:59 AM, Cong Wang<xiyou.wangcong@gmail.com>
> > > wrote:
> > > > -       if (mm&&  !IS_ERR(mm)) {
> > > > +       if (!IS_ERR_OR_NULL(mm)) {
> > > 
> > > I personally find original code way more readable.
> > 
> > personally, me too. but new one is also acceptable to me.
> 
> That now makes 4 votes that it's not an improvement, with no dissent
> (unless akpm's + proc-use-is_err_or_null.patch added to -mm tree
> should be counted as dissent): let's drop this patch.

I think it's OK.  IS_ERR_OR_NULL encapsulates the concept "something
went wrong".

Not that I'm particularly passionate about it.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2012-04-12 19:31 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-11  5:59 [PATCH 1/6 v3] proc: clean up /proc/<pid>/environ handling Cong Wang
2012-04-11  5:59 ` [PATCH 2/6] proc: unify ptrace_may_access locking code Cong Wang
2012-04-11  5:59 ` [PATCH 3/6] proc: remove mm_for_maps() Cong Wang
2012-04-12  0:38   ` Hugh Dickins
2012-04-11  5:59 ` [PATCH 4/6] proc: use mm_access() instead of ptrace_may_access() Cong Wang
2012-04-11  5:59 ` [PATCH 5/6] proc: use task_access_lock() " Cong Wang
2012-04-12 13:22   ` Cong Wang
2012-04-12 15:29     ` Djalal Harouni
2012-04-11  5:59 ` [PATCH 6/6] proc: use IS_ERR_OR_NULL() Cong Wang
2012-04-12  0:41   ` Hugh Dickins
2012-04-12 13:17     ` Cong Wang
2012-04-12 13:23   ` Alexey Dobriyan
2012-04-12 18:46     ` KOSAKI Motohiro
2012-04-12 19:26       ` Hugh Dickins
2012-04-12 19:31         ` Andrew Morton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.