Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1302400 > unrolled thread

[PATCH 2/2] proc read mm's {arg,env}_{start,end} with mmap semaphore taken.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] proc read mm's {arg,env}_{start,end} with mmap semaphore taken. Mateusz Guzik <mguzik@redhat.com> - 2016-01-06 06:10 +0100
    Re: [PATCH 2/2] proc read mm's {arg,env}_{start,end} with mmap  semaphore taken. Anshuman Khandual <anshuman.linux@gmail.com> - 2016-01-06 10:50 +0100
      Re: [PATCH 2/2] proc read mm's {arg,env}_{start,end} with mmap  semaphore taken. Mateusz Guzik <mguzik@redhat.com> - 2016-01-06 20:50 +0100

#1302400 — [PATCH 2/2] proc read mm's {arg,env}_{start,end} with mmap semaphore taken.

Only functions doing more than one read are modified. Consumeres
happened to deal with possibly changing data, but it does not seem
like a good thing to rely on.

Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
---
 fs/proc/base.c | 13 ++++++++++---
 mm/util.c      | 16 ++++++++++++----
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index e665097..4f764c2 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -953,6 +953,7 @@ static ssize_t environ_read(struct file *file, char __user *buf,
 	unsigned long src = *ppos;
 	int ret = 0;
 	struct mm_struct *mm = file->private_data;
+	unsigned long env_start, env_end;
 
 	if (!mm)
 		return 0;
@@ -964,19 +965,25 @@ static ssize_t environ_read(struct file *file, char __user *buf,
 	ret = 0;
 	if (!atomic_inc_not_zero(&mm->mm_users))
 		goto free;
+
+	down_read(&mm->mmap_sem);
+	env_start = mm->env_start;
+	env_end = mm->env_end;
+	up_read(&mm->mmap_sem);
+
 	while (count > 0) {
 		size_t this_len, max_len;
 		int retval;
 
-		if (src >= (mm->env_end - mm->env_start))
+		if (src >= (env_end - env_start))
 			break;
 
-		this_len = mm->env_end - (mm->env_start + src);
+		this_len = env_end - (env_start + src);
 
 		max_len = min_t(size_t, PAGE_SIZE, count);
 		this_len = min(max_len, this_len);
 
-		retval = access_remote_vm(mm, (mm->env_start + src),
+		retval = access_remote_vm(mm, (env_start + src),
 			page, this_len, 0);
 
 		if (retval <= 0) {
diff --git a/mm/util.c b/mm/util.c
index 338e697..bafd4c5 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -506,17 +506,25 @@ int get_cmdline(struct task_struct *task, char *buffer, int buflen)
 	int res = 0;
 	unsigned int len;
 	struct mm_struct *mm = get_task_mm(task);
+	unsigned long arg_start, arg_end, env_start, env_end;
 	if (!mm)
 		goto out;
 	if (!mm->arg_end)
 		goto out_mm;	/* Shh! No looking before we're done */
 
-	len = mm->arg_end - mm->arg_start;
+	down_read(&mm->mmap_sem);
+	arg_start = mm->arg_start;
+	arg_end = mm->arg_end;
+	env_start = mm->env_start;
+	env_end = mm->env_end;
+	up_read(&mm->mmap_sem);
+
+	len = arg_end - arg_start;
 
 	if (len > buflen)
 		len = buflen;
 
-	res = access_process_vm(task, mm->arg_start, buffer, len, 0);
+	res = access_process_vm(task, arg_start, buffer, len, 0);
 
 	/*
 	 * If the nul at the end of args has been overwritten, then
@@ -527,10 +535,10 @@ int get_cmdline(struct task_struct *task, char *buffer, int buflen)
 		if (len < res) {
 			res = len;
 		} else {
-			len = mm->env_end - mm->env_start;
+			len = env_end - env_start;
 			if (len > buflen - res)
 				len = buflen - res;
-			res += access_process_vm(task, mm->env_start,
+			res += access_process_vm(task, env_start,
 						 buffer+res, len, 0);
 			res = strnlen(buffer, res);
 		}
-- 
1.8.3.1

--
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/

[toc] | [next] | [standalone]

#1302507 — Re: [PATCH 2/2] proc read mm's {arg,env}_{start,end} with mmap semaphore taken.

On Wed, Jan 6, 2016 at 10:32 AM, Mateusz Guzik <mguzik@redhat.com> wrote:
> Only functions doing more than one read are modified. Consumeres
> happened to deal with possibly changing data, but it does not seem
> like a good thing to rely on.

There are no other functions which might be reading mm-> members without
having a lock ? Why just deal with functions with more than one read ?
--
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/

[toc] | [prev] | [next] | [standalone]

#1303025 — Re: [PATCH 2/2] proc read mm's {arg,env}_{start,end} with mmap semaphore taken.

On Wed, Jan 06, 2016 at 03:14:22PM +0530, Anshuman Khandual wrote:
> On Wed, Jan 6, 2016 at 10:32 AM, Mateusz Guzik <mguzik@redhat.com> wrote:
> > Only functions doing more than one read are modified. Consumeres
> > happened to deal with possibly changing data, but it does not seem
> > like a good thing to rely on.
> 
> There are no other functions which might be reading mm-> members without
> having a lock ? Why just deal with functions with more than one read ?

Ideally all functions would read stuff with some kind of lock.

However, if only one field is read, the lock does not change anything.
Similarly, if multiple fields are read, but are not used for
calculations against each other, the lock likely does not change
anything, so there is no rush here.

Using mmap_sem in all places may or may not be possible as it is, and
even if it is possible it may turn out to be wasteful and maybe
something else should be derived for protection of said fields (maybe a
seq counter?).

That said, patches here only deal with one actual I found and patch up
consumers which had the most potential for trouble. Patching everything
in some way definitely sounds like a good idea and I may get around to
that.

-- 
Mateusz Guzik
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web