oom: remove totalpage normalization from oom_badness()

Current oom_score_adj is completely broken because It is strongly bound google usecase and ignore other all.

1) Priority inversion
As kamezawa-san pointed out, This break cgroup and lxr environment. He said,

	> Assume 2 proceses A, B which has oom_score_adj of 300 and 0
	> And A uses 200M, B uses 1G of memory under 4G system
	>
	> Under the system.
	> 	A's socre = (200M *1000)/4G + 300 = 350
	> 	B's score = (1G * 1000)/4G = 250.
	>
	> In the cpuset, it has 2G of memory.
	> 	A's score = (200M * 1000)/2G + 300 = 400
	> 	B's socre = (1G * 1000)/2G = 500
	>
	> This priority-inversion don't happen in current system.

2) Ratio base point don't works large machine oom_score_adj normalize oom-score to 0-1000 range. but if the machine has 1TB memory, 1 point (i.e. 0.1%) mean 1GB. this is no suitable for tuning parameter.
As I said, proposional value oriented tuning parameter has scalability risk.

3) No reason to implement ABI breakage.
old tuning parameter mean)
oom-score = oom-base-score x 2^oom_adj
new tuning parameter mean)
oom-score = oom-base-score + oom_score_adj / (totalram + totalswap)
but "oom_score_adj / (totalram + totalswap)" can be calculated in userland too. beucase both totalram and totalswap has been exporsed by /proc. So no reason to introduce funny new equation.

4) totalram based normalization assume flat memory model. example, the machine is assymmetric numa. fat node memory and thin node memory might have another wight value.
In other word, totalram based priority is a one of policy. Fixed and workload depended policy shouldn't be embedded in kernel. probably.

Then, this patch remove *UGLY* total_pages suck completely. Googler can calculate it at userland!

---
 fs/proc/base.c        |   35 +++----------
 include/linux/oom.h   |   16 +-----
 include/linux/sched.h |    2 +-
 mm/oom_kill.c         |  142 ++++++++++++++++++++-----------------------------
 4 files changed, 69 insertions(+), 126 deletions(-)

index dc5d5f5..86c402e 100644
@@ -434,8 +434,7 @@ static int proc_oom_score(struct task_struct *task, char *buffer)
 
 	read_lock(&tasklist_lock);
 	if (pid_alive(task))
-		points = oom_badness(task, NULL, NULL,
-					totalram_pages + total_swap_pages);
+		points = oom_badness(task, NULL, NULL);
 	read_unlock(&tasklist_lock);
 	return sprintf(buffer, "%lu\n", points);
 }

@@ -1056,15 +1055,6 @@ static ssize_t oom_adjust_write(struct file *file, const char __user *buf,
 			current->comm, task_pid_nr(current),
 			task_pid_nr(task), task_pid_nr(task));
 	task->signal->oom_adj = oom_adjust;
-	/*
-	 * Scale /proc/pid/oom_score_adj appropriately ensuring that a maximum
-	 * value is always attainable.
-	 */
-	if (task->signal->oom_adj == OOM_ADJUST_MAX)
-		task->signal->oom_score_adj = OOM_SCORE_ADJ_MAX;
-	else
-		task->signal->oom_score_adj = (oom_adjust * OOM_SCORE_ADJ_MAX) /
-								-OOM_DISABLE;
 	unlock_task_sighand(task, &flags);
 	put_task_struct(task);
 

@@ -1077,12 +1067,14 @@ static const struct file_operations proc_oom_adjust_operations = {
 	.llseek		= generic_file_llseek,
 };
 
+#define TMPBUFLEN 21
+
 static ssize_t oom_score_adj_read(struct file *file, char __user *buf,
 					size_t count, loff_t *ppos)
 {
 	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
-	char buffer[PROC_NUMBUF];
-	int oom_score_adj = OOM_SCORE_ADJ_MIN;
+	char buffer[TMPBUFLEN];
+	long oom_score_adj = 0;
 	unsigned long flags;
 	size_t len;
 

@@ -1093,7 +1085,7 @@ static ssize_t oom_score_adj_read(struct file *file, char __user *buf,
 		unlock_task_sighand(task, &flags);
 	}
 	put_task_struct(task);
-	len = snprintf(buffer, sizeof(buffer), "%d\n", oom_score_adj);
+	len = snprintf(buffer, sizeof(buffer), "%ld\n", oom_score_adj);
 	return simple_read_from_buffer(buf, count, ppos, buffer, len);
 }
 

@@ -1101,7 +1093,7 @@ static ssize_t oom_score_adj_write(struct file *file, const char __user *buf,
 					size_t count, loff_t *ppos)
 {
 	struct task_struct *task;
-	char buffer[PROC_NUMBUF];
+	char buffer[TMPBUFLEN];
 	unsigned long flags;
 	long oom_score_adj;
 	int err;

@@ -1115,9 +1107,6 @@ static ssize_t oom_score_adj_write(struct file *file, const char __user *buf,
 	err = strict_strtol(strstrip(buffer), 0, &oom_score_adj);
 	if (err)
 		return -EINVAL;
-	if (oom_score_adj < OOM_SCORE_ADJ_MIN ||
-			oom_score_adj > OOM_SCORE_ADJ_MAX)
-		return -EINVAL;
 
 	task = get_proc_task(file->f_path.dentry->d_inode);
 	if (!task)

@@ -1134,15 +1123,6 @@ static ssize_t oom_score_adj_write(struct file *file, const char __user *buf,
 	}
 
 	task->signal->oom_score_adj = oom_score_adj;
-	/*
-	 * Scale /proc/pid/oom_adj appropriately ensuring that OOM_DISABLE is
-	 * always attainable.
-	 */
-	if (task->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
-		task->signal->oom_adj = OOM_DISABLE;
-	else
-		task->signal->oom_adj = (oom_score_adj * OOM_ADJUST_MAX) /
-							OOM_SCORE_ADJ_MAX;
 	unlock_task_sighand(task, &flags);
 	put_task_struct(task);
 	return count;

@@ -1155,7 +1135,6 @@ static const struct file_operations proc_oom_score_adj_operations = {
 };
 
 #ifdef CONFIG_AUDITSYSCALL
-#define TMPBUFLEN 21
 static ssize_t proc_loginuid_read(struct file * file, char __user * buf,
 				  size_t count, loff_t *ppos)
 {

index 5e3aa83..21006dc 100644
@@ -12,13 +12,6 @@
 #define OOM_ADJUST_MIN (-16)
 #define OOM_ADJUST_MAX 15
 
-/*
- * /proc/<pid>/oom_score_adj set to OOM_SCORE_ADJ_MIN disables oom killing for
- * pid.
- */
-#define OOM_SCORE_ADJ_MIN	(-1000)
-#define OOM_SCORE_ADJ_MAX	1000
-
 #ifdef __KERNEL__
 
 #include <linux/sched.h>

@@ -40,8 +33,9 @@ enum oom_constraint {
 	CONSTRAINT_MEMCG,
 };
 
-extern unsigned int oom_badness(struct task_struct *p, struct mem_cgroup *mem,
-			const nodemask_t *nodemask, unsigned long totalpages);
+/* The badness from the OOM killer */
+extern unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *mem,
+				 const nodemask_t *nodemask);
 extern int try_set_zonelist_oom(struct zonelist *zonelist, gfp_t gfp_flags);
 extern void clear_zonelist_oom(struct zonelist *zonelist, gfp_t gfp_flags);
 

@@ -62,10 +56,6 @@ static inline void oom_killer_enable(void)
 	oom_killer_disabled = false;
 }
 
-/* The badness from the OOM killer */
-extern unsigned long badness(struct task_struct *p, struct mem_cgroup *mem,
-		      const nodemask_t *nodemask, unsigned long uptime);
-
 extern struct task_struct *find_lock_task_mm(struct task_struct *p);
 
 /* sysctls */

index 56154bb..74ed859 100644
@@ -625,7 +625,7 @@ struct signal_struct {
 #endif
 
 	int oom_adj;		/* OOM kill score adjustment (bit shift) */
-	int oom_score_adj;	/* OOM kill score adjustment */
+	long oom_score_adj;	/* OOM kill score adjustment */
 };
 
 /* Context switch must be unlocked if interrupts are to be enabled */

1.6.5.2



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

oom_adj is not only used for kernel knob, but also used for application interface. Then, adding new knob is no good reason to deprecate it. Don't do stupid!

Also, after former patch, oom_score_adj can't be used for setting OOM_DISABLE. We need "echo -17 > /proc/<pid>/oom_adj" thing.

This reverts commit 51b1bd2ace1595b72956224deda349efa880b693.

---
 Documentation/feature-removal-schedule.txt |   25 -------------------------
 Documentation/filesystems/proc.txt         |    3 ---
 fs/proc/base.c                             |    8 --------
 include/linux/oom.h                        |    3 ---
 4 files changed, 0 insertions(+), 39 deletions(-)

index 9961f15..1cba5b8 100644
@@ -151,31 +151,6 @@ Who:	Eric Biederman <[email protected]>
 
 ---------------------------
 
-What:	/proc/<pid>/oom_adj
-When:	August 2012
-Why:	/proc/<pid>/oom_adj allows userspace to influence the oom killer's
-	badness heuristic used to determine which task to kill when the kernel
-	is out of memory.
-
-	The badness heuristic has since been rewritten since the introduction of
-	this tunable such that its meaning is deprecated.  The value was
-	implemented as a bitshift on a score generated by the badness()
-	function that did not have any precise units of measure.  With the
-	rewrite, the score is given as a proportion of available memory to the
-	task allocating pages, so using a bitshift which grows the score
-	exponentially is, thus, impossible to tune with fine granularity.
-
-	A much more powerful interface, /proc/<pid>/oom_score_adj, was
-	introduced with the oom killer rewrite that allows users to increase or
-	decrease the badness() score linearly.  This interface will replace
-	/proc/<pid>/oom_adj.
-
-	A warning will be emitted to the kernel log if an application uses this
-	deprecated interface.  After it is printed once, future warnings will be
-	suppressed until the kernel is rebooted.
-
----------------------------
-
 What:	remove EXPORT_SYMBOL(kernel_thread)
 When:	August 2006
 Files:	arch/*/kernel/*_ksyms.c

index a6aca87..cf1295c 100644
@@ -1285,9 +1285,6 @@ scaled linearly with /proc/<pid>/oom_score_adj.
 Writing to /proc/<pid>/oom_score_adj or /proc/<pid>/oom_adj will change the
 other with its scaled value.
 
-NOTICE: /proc/<pid>/oom_adj is deprecated and will be removed, please see
-Documentation/feature-removal-schedule.txt.
-
 Caveat: when a parent task is selected, the oom killer will sacrifice any first
 generation children with seperate address spaces instead, if possible.  This
 avoids servers and important system daemons from being killed and loses the

index 86c402e..0d2ce21 100644
@@ -1046,14 +1046,6 @@ static ssize_t oom_adjust_write(struct file *file, const char __user *buf,
 		return -EACCES;
 	}
 
-	/*
-	 * Warn that /proc/pid/oom_adj is deprecated, see
-	 * Documentation/feature-removal-schedule.txt.
-	 */
-	printk_once(KERN_WARNING "%s (%d): /proc/%d/oom_adj is deprecated, "
-			"please use /proc/%d/oom_score_adj instead.\n",
-			current->comm, task_pid_nr(current),
-			task_pid_nr(task), task_pid_nr(task));
 	task->signal->oom_adj = oom_adjust;
 	unlock_task_sighand(task, &flags);
 	put_task_struct(task);

index 21006dc..394f2e6 100644
@@ -2,9 +2,6 @@
 #define __INCLUDE_LINUX_OOM_H
 
 /*
- * /proc/<pid>/oom_adj is deprecated, see
- * Documentation/feature-removal-schedule.txt.
- *
  * /proc/<pid>/oom_adj set to -17 protects from the oom-killer
  */
 #define OOM_DISABLE (-17)

1.6.5.2



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

NACK as a logical follow-up to my NACK for "oom: remove totalpage normalization from oom_badness()"

Huh?

I requested you show us justification. BUT YOU DIDNT. If you have any usecase, show us RIGHT NOW.

Don't speaking sucking crap. If you don't want ignore you. you are busy than you.

DONT STUPID.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

The new tunable added in 2.6.36, /proc/pid/oom_score_adj, is necessary for the units that the badness score now uses. We need a tunable with a much higher resolution than the oom_adj scale from -16 to +15, and one that scales linearly as opposed to exponentially. Since that tunable is much more powerful than the oom_adj implementation, which never made any real sense for defining oom killing priority for any purpose other than polarization, the old tunable is deprecated for two years.

Who we?

The reason that you ware NAKed was not to introduce new powerful feature. It was caused to break old and used feature from applications.

You haven't tested your patch at all. Distro's initram script are using oom_adj interface and latest kernel show pointless warnings "/proc/xx/oom_adj is deprecated, please use /proc/xx/oom_score_adj instead." at _every_ boot time.

As I said, DON'T SEND UNTESTED PATCH! DON'T BREAK USERLAND CARELESSLY!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Linux users who care about prioritizing tasks for oom kill with a tunable that (1) has a unit, (2) has a higher resolution, and (3) is linear and not exponential. Memcg doesn't solve this issue without incurring a 1% memory cost.

No, it doesn't, and you completely and utterly failed to show a single usecase that broke as a result of this because nobody can currently use oom_adj for anything other than polarization. Thus, there's no backwards compatibility issue.

Yes, I've tested it, and it deprecates the tunable as expected. A single warning message serves the purpose well: let users know one time without being overly verbose that the tunable is deprecated and give them sufficient time (2 years) to start using the new tunable. That's how deprecation is done.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No. Majority user don't care. You only talk about your case. Don't ignore end user.

Look at a real.
All major distributions has already turn on memcg. End user don't need to pay additional cost.

No. I showed.
1) Google code search showed some application are using this feature.
http://www.google.com/codesearch?as_q=oom_adj&btnG=Search+Code&hl=ja&as_package=&as_lang=&as_filename=&as_class=&as_function=&as_license=&as_case=

2) Not body use oom_adj other than polarization even though there are a few. example, kde are using.

	http://www.google.com/codesearch/p?hl=ja#MPJuLvSvNYM/pub/kde/unstable/snapshots/kdelibs.tar.bz2%7CWClmGVN5niU/kdelibs-1164923/kinit/start_kdeinit.c&q=oom_adj%20kde%205

When you are talking polarization issue, you blind a real. Don't talk your dream.

3) udev are using this feature. It's one of major linux component and you broke.

http://www.google.com/codesearch/p?hl=ja#KVTjzuVpblQ/pub/linux/utils/kernel/hotplug/udev-072.tar.bz2%7CwUSE-Ay3lLI/udev-072/udevd.c&q=oom_adj

You don't have to break our userland. you can't rewrite or deprecate old one. It's used! You can only add orthogonal new knob.

no sense.

Why do their application need to rewrite for *YOU*? Okey, you will got benefit from your new knob. But NOBDOY use the new one. and People need to rewrite their application even though no benefit.

Don't do selfish userland breakage!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

And you said you ignore bug even though you have seen it. It suck!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

At v2.6.36-rc1, oom-killer doesn't work at all because YOU BROKE. And I was working on fixing it.

2010-08-19
http://marc.info/?t=128223176900001&r=1&w=2
http://marc.info/?t=128221532700003&r=1&w=2
http://marc.info/?t=128221532500008&r=1&w=2

However, You submitted new crap before the fixing.

2010-08-15
http://marc.info/?t=128184669600001&r=1&w=2

If you tested mainline a bit, you could find the problem quickly. You should have fixed mainline kernel at first.

Again, YOU HAVEN'T TESTED YOUR OWN PATCH AT ALL.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

This existed before my oom killer rewrite, it was only noticed because the rewrite enabled oom_dump_tasks by default.

Yes, tasklist_lock was dropped in a mismerge of my patches when posting them. Thanks for finding it and posting a patch, I appreciate it.

Yes, if a task was racing between oom_kill_process() and oom_kill_task() and all threads had dropped its mm between calls then there was a NULL pointer dereference, thanks for fixing that as well.

This isn't "crap", this is a necessary bit to ensure that tasks that share an ->mm with a task immune from kill aren't killed themselves since we can't free the memory. We came to the consensus that it would be better to count the tasks that are OOM_DISABLE in the mm_struct to avoid the O(2*n) tasklist scan.

Thanks for finding a couple fixes during the 2.6.36-rc1 when the rewrite was first merged, it's much appreciated!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

If they don't care, then they won't be using oom_adj, so you're point about it's deprecation is irrelevant.

Other users do want a more powerful userspace interface with a unit and higher resolution (I am one of them), there's no requirement that those users need to be in the majority.

Memcg also has a command-line disabling option to avoid incurring this 1% memory cost when you're not going to be using it.

oom_adj isn't removed, it's deprecated. These users are using a deprecated interface and have a few years to convert to using the new interface (if it ever is actually removed).

I don't understand what you're trying to say here, but the current users of oom_adj that aren't +15 or -16 (or OOM_DISABLE) are arbitrary based relative to other tasks such as +5, +10, etc. They don't have any semantics other than being arbitrarily relative because it doesn't work in a linear way or with a scale.

That's incorrect, I didn't break anything by deprecating a tunable for a few years. oom_adj gets converted roughly into an equivalent (but linear) oom_score_adj.

Unfortunately for your argument, you can't show a single example of a current oom_adj user that has a scientific calculation behind its value that is now broken on the linear scale.

It's deprecated for a few years so users can gradually convert to the new tunable, it wasn't removed when the new one was introduced. A higher resolution tunable that scales linearly with a unit is an advantage for Linux (for the minority of users who care about oom killing priority beyond the heuristic) and I think a few years is enough time for users to do a simple conversion to the new tunable.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Documentation/ABI/obsolete/

should have all obsoletes in it.

Alan

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Good point, the only documentation right now is in Documentation/feature-removal-schedule.txt and in the kernel log the first time oom_adj is written. I'll generate a patch, thanks!

/proc/pid/oom_adj was deprecated in August 2010 with the introduction of the new oom killer heuristic.

This patch copies the Documentation/feature-removal-schedule.txt entry for this tunable to the Documentation/ABI/obsolete directory so nobody misses it.

---
 Documentation/ABI/obsolete/proc-pid-oom_adj |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/obsolete/proc-pid-oom_adj

new file mode 100644
@@ -0,0 +1,22 @@
+What:	/proc/<pid>/oom_adj
+When:	August 2012
+Why:	/proc/<pid>/oom_adj allows userspace to influence the oom killer's
+	badness heuristic used to determine which task to kill when the kernel
+	is out of memory.
+
+	The badness heuristic has since been rewritten since the introduction of
+	this tunable such that its meaning is deprecated.  The value was
+	implemented as a bitshift on a score generated by the badness()
+	function that did not have any precise units of measure.  With the
+	rewrite, the score is given as a proportion of available memory to the
+	task allocating pages, so using a bitshift which grows the score
+	exponentially is, thus, impossible to tune with fine granularity.
+
+	A much more powerful interface, /proc/<pid>/oom_score_adj, was
+	introduced with the oom killer rewrite that allows users to increase or
+	decrease the badness() score linearly.  This interface will replace
+	/proc/<pid>/oom_adj.
+
+	A warning will be emitted to the kernel log if an application uses this
+	deprecated interface.  After it is printed once, future warnings will be
+	suppressed until the kernel is rebooted.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

NAK. You seems to think shouting claim makes some effect. but It's incorrect. Your childish shout doesn't solve any real world issue. Only code fix does.

The tunable is deprecated. If you are really that concerned about the existing users who you don't think can convert in the next two years, why don't you help them convert? That fixes the issue, but you're not interested in that. I offered to convert any open-source users you can list (the hardest part of the conversion is finding who to send patches to :). You're only interested in continuing to assert your position as correct even when the kernel is obviously moving in a different direction.

Others may have a different opinion of who is being childish in this whole ordeal.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Why don't you change by _your_ hand?

_Usually_ userland software changed at first _by_ who wanted the change. Example, we fujitsu changed elf core file format when vma are >65536, but It was not made any breakage. we changed gdb, binutils, elfutils and etc etc _at_ first.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No irrelevant. Your patch break their environment even though they don't use oom_adj explicitly. because their application are using it.

But, they only live in your DREAM. you coldn't show who necessary.

Look at real. who use it?

No. there is no reason to enforce rewrite tons applicatin.

Even if you don't understand, they are IN THE WORLD. you don't have to ignore a real.

you are talking unrelated thing.

no sense.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

The _only_ difference too oom_adj since the rewrite is that it is now mapped on a linear scale rather than an exponential scale. That's because the heuristic itself has a defined range [0, 1000] that characterizes the memory usage of the application it is ranking. To show any breakge, you would have to show how oom_adj values being used by applications are based on a calculated value that prioritizes those tasks amongst each other. With the exponential scale, that's nearly impossible because of the number of arbitrary heuristics that were used before oom_adj were considered (runtime, nice level, CAP_SYS_RAWIO, etc).

So don't talk about userspace breakage when you can't even describe it or present a single usecase.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

_only_ mean don't ZERO different. Why do userland application need to rewrite?

But, No people have agreed your powerfulness even though you talked about the same explanation a lot of times.

Again, IF you need to [0 .. 1000] range, you can calculate it by your application. current oom score can be get from /proc/pid/oom_score and total memory can be get from /proc/meminfo. You shouldn't have break anything.

Huh? Remember! your feature have ZERO user.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Because NOTHING breaks with the new mapping. Eight months later since this was initially proposed on linux-mm, you still cannot show a single example that depended on the exponential mapping of oom_adj. I'm not going to continue responding to your criticism about this point since your argument is completely and utterly baseless.

That would require the userspace tunable to be adjusted anytime a task's mempolicy changes, its nodemask changes, it's cpuset attachment changes, its mems change, a memcg limit changes, etc. The only constant is the task's priority, and the current oom_score_adj implementation preserves that unless explicitly changed later by the user. I completely understand that you may not have a use for this.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No regression mean no break. Not single nor multiple. see?

All situation can be calculated on userland. User process can be know their bindings.

Nothing breaks. If something did, you could respond to my answer above and provide a single example of a real-world example that broke as a result of the new linear mapping.

Yes, but the proportional priority-based oom_score_adj values allow users to avoid recalculating and writing that value anytime a mempolicy attachment changes, its nodemask changes, it moves to another cpuset, its set of mems changes, its memcg attachment changes, its limit is modiifed, etc.

Changelog
o since v1
- function comment also change current->cred_guard_mutex to current->signal->cred_guard_mutex.

---------------------------------------------------------------------------
Oleg Nesterov pointed out we have to prevent multiple-threads-inside-exec itself and we can reuse ->cred_guard_mutex for it. Yes, concurrent execve() has no worth.

Let's move ->cred_guard_mutex from task_struct to signal_struct. It naturally prevent multiple-threads-inside-exec.

---
 fs/exec.c                 |   10 +++++-----
 fs/proc/base.c            |    8 ++++----
 include/linux/init_task.h |    4 ++--
 include/linux/sched.h     |    7 ++++---
 include/linux/tracehook.h |    2 +-
 kernel/cred.c             |    4 +---
 kernel/fork.c             |    2 ++
 kernel/ptrace.c           |    4 ++--
 8 files changed, 21 insertions(+), 20 deletions(-)

index 6d2b6f9..94dabd2 100644
@@ -1078,14 +1078,14 @@ EXPORT_SYMBOL(setup_new_exec);
  */
 int prepare_bprm_creds(struct linux_binprm *bprm)
 {
-	if (mutex_lock_interruptible(&current->cred_guard_mutex))
+	if (mutex_lock_interruptible(&current->signal->cred_guard_mutex))
 		return -ERESTARTNOINTR;
 
 	bprm->cred = prepare_exec_creds();
 	if (likely(bprm->cred))
 		return 0;
 
-	mutex_unlock(&current->cred_guard_mutex);
+	mutex_unlock(&current->signal->cred_guard_mutex);
 	return -ENOMEM;
 }
 

@@ -1093,7 +1093,7 @@ void free_bprm(struct linux_binprm *bprm)
 {
 	free_arg_pages(bprm);
 	if (bprm->cred) {
-		mutex_unlock(&current->cred_guard_mutex);
+		mutex_unlock(&current->signal->cred_guard_mutex);
 		abort_creds(bprm->cred);
 	}
 	kfree(bprm);

@@ -1114,13 +1114,13 @@ void install_exec_creds(struct linux_binprm *bprm)
 	 * credentials; any time after this it may be unlocked.
 	 */
 	security_bprm_committed_creds(bprm);
-	mutex_unlock(&current->cred_guard_mutex);
+	mutex_unlock(&current->signal->cred_guard_mutex);
 }
 EXPORT_SYMBOL(install_exec_creds);
 
 /*
  * determine how safe it is to execute the proposed program
- * - the caller must hold current->cred_guard_mutex to protect against
+ * - the caller must hold ->cred_guard_mutex to protect against
  *   PTRACE_ATTACH
  */
 int check_unsafe_exec(struct linux_binprm *bprm)

index 0d2ce21..d3ea8b0 100644
@@ -226,7 +226,7 @@ struct mm_struct *mm_for_maps(struct task_struct *task)
 {
 	struct mm_struct *mm;
 
-	if (mutex_lock_killable(&task->cred_guard_mutex))
+	if (mutex_lock_killable(&task->signal->cred_guard_mutex))
 		return NULL;
 
 	mm = get_task_mm(task);

@@ -235,7 +235,7 @@ struct mm_struct *mm_for_maps(struct task_struct *task)
 		mmput(mm);
 		mm = NULL;
 	}
-	mutex_unlock(&task->cred_guard_mutex);
+	mutex_unlock(&task->signal->cred_guard_mutex);
 
 	return mm;
 }

@@ -2277,14 +2277,14 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 		goto out_free;
 
 	/* Guard against adverse ptrace interaction */
-	length = mutex_lock_interruptible(&task->cred_guard_mutex);
+	length = mutex_lock_interruptible(&task->signal->cred_guard_mutex);
 	if (length < 0)
 		goto out_free;
 
 	length = security_setprocattr(task,
 				      (char*)file->f_path.dentry->d_name.name,
 				      (void*)page, count);
-	mutex_unlock(&task->cred_guard_mutex);
+	mutex_unlock(&task->signal->cred_guard_mutex);
 out_free:
 	free_page((unsigned long) page);
 out:

index 2fea6c8..1f8c06c 100644
@@ -29,6 +29,8 @@ extern struct fs_struct init_fs;
 		.running = 0,						\
 		.lock = __SPIN_LOCK_UNLOCKED(sig.cputimer.lock),	\
 	},								\
+	.cred_guard_mutex =						\
+		 __MUTEX_INITIALIZER(sig.cred_guard_mutex),		\
 }
 
 extern struct nsproxy init_nsproxy;

@@ -145,8 +147,6 @@ extern struct cred init_cred;
 	.group_leader	= &tsk,						\
 	RCU_INIT_POINTER(.real_cred, &init_cred),			\
 	RCU_INIT_POINTER(.cred, &init_cred),				\
-	.cred_guard_mutex =						\
-		 __MUTEX_INITIALIZER(tsk.cred_guard_mutex),		\
 	.comm		= "swapper",					\
 	.thread		= INIT_THREAD,					\
 	.fs		= &init_fs,					\

index 74ed859..ac65605 100644
@@ -626,6 +626,10 @@ struct signal_struct {
 
 	int oom_adj;		/* OOM kill score adjustment (bit shift) */
 	long oom_score_adj;	/* OOM kill score adjustment */
+
+	struct mutex cred_guard_mutex;	/* guard against foreign influences on
+					 * credential calculations
+					 * (notably. ptrace) */
 };
 
 /* Context switch must be unlocked if interrupts are to be enabled */

@@ -1305,9 +1309,6 @@ struct task_struct {
 					 * credentials (COW) */
 	const struct cred __rcu *cred;	/* effective (overridable) subjective task
 					 * credentials (COW) */
-	struct mutex cred_guard_mutex;	/* guard against foreign influences on
-					 * credential calculations
-					 * (notably. ptrace) */
 	struct cred *replacement_session_keyring; /* for KEYCTL_SESSION_TO_PARENT */
 
 	char comm[TASK_COMM_LEN]; /* executable name excluding path

index 10db010..3a2e66d 100644
@@ -150,7 +150,7 @@ static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step)
  *
  * Return %LSM_UNSAFE_* bits applied to an exec because of tracing.
  *
- * @task->cred_guard_mutex is held by the caller through the do_execve().
+ * @task->signal->cred_guard_mutex is held by the caller through the do_execve().
  */
 static inline int tracehook_unsafe_exec(struct task_struct *task)
 {

index 9a3e226..6a1aa00 100644
@@ -325,7 +325,7 @@ EXPORT_SYMBOL(prepare_creds);
 
 /*
  * Prepare credentials for current to perform an execve()
- * - The caller must hold current->cred_guard_mutex
+ * - The caller must hold ->cred_guard_mutex
  */
 struct cred *prepare_exec_creds(void)
 {

@@ -384,8 +384,6 @@ int copy_creds(struct task_struct *p, unsigned long clone_flags)
 	struct cred *new;
 	int ret;
 
-	mutex_init(&p->cred_guard_mutex);
-
 	if (
 #ifdef CONFIG_KEYS
 		!p->cred->thread_keyring &&

index c445f8c..8c09cf9 100644
@@ -904,6 +904,8 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	sig->oom_adj = current->signal->oom_adj;
 	sig->oom_score_adj = current->signal->oom_score_adj;
 
+	mutex_init(&sig->cred_guard_mutex);
+
 	return 0;
 }
 

index f34d798..ac5013a 100644
@@ -181,7 +181,7 @@ int ptrace_attach(struct task_struct *task)
 	 * under ptrace.
 	 */
 	retval = -ERESTARTNOINTR;
-	if (mutex_lock_interruptible(&task->cred_guard_mutex))
+	if (mutex_lock_interruptible(&task->signal->cred_guard_mutex))
 		goto out;
 
 	task_lock(task);

@@ -208,7 +208,7 @@ int ptrace_attach(struct task_struct *task)
 unlock_tasklist:
 	write_unlock_irq(&tasklist_lock);
 unlock_creds:
-	mutex_unlock(&task->cred_guard_mutex);
+	mutex_unlock(&task->signal->cred_guard_mutex);
 out:
 	return retval;
 }

1.6.5.2

This has my ACK if Oleg doesn't see any problems.

Thanks,
Roland

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I believe the patch is fine (it already has my reviewed-by).

Except: I am not sure about -stable. At least, this patch should not go into the <2.6.35 kernels, it relies on misc changes which changed the scope of task->signal. Before 2.6.35 almost any user of ->cred_guard_mutex can race with exit and hit ->signal == NULL.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I see no justification for a change like this in any -stable tree. It's just a cleanup, right? If it's a prerequisite for the fix we like for an "important" bug, then that's a different story. In its own right, it's clearly not appropriate for backporting.

Thanks,
Roland

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Hello,

Because [4/4] depend on [3/4] and I hope to backport it. Do you dislike it too?

Thanks.

Ah, OK. That is indeed a fix for an important bug. Not knowing the mm code very well, I'm not in a position to judge whether it's safe enough for a -stable stream or not. If it is and it could be done safely without relying on 3/4, that would seem safer to me, but it is not a strong opinion.

Thanks,
Roland

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

ChangeLog
o since v2
- Move ->in_exec_mm from task_struct to signal_struct
- clean up oom_rss_swap_usage()
o since v1
- Always use thread group leader's ->in_exec_mm.
It slightly makes efficient oom when a process has many thread.
- Add the link of Brad's explanation to the description.

-----------------------------------------------------------
Brad Spengler published a local memory-allocation DoS that evades the OOM-killer (though not the virtual memory RLIMIT):
http://www.grsecurity.net/~spender/64bit_dos.c

Because execve() makes new mm struct and setup stack and copy argv. It mean the task have two mm while execve() temporary. Unfortunately this nascent mm is not pointed any tasks, then OOM-killer can't detect this memory usage. therefore OOM-killer may kill incorrect task.

Thus, this patch added signal->in_exec_mm member and track nascent mm usage.

---
 fs/compat.c             |    4 +++-
 fs/exec.c               |   16 +++++++++++++++-
 include/linux/binfmts.h |    1 +
 include/linux/sched.h   |    1 +
 mm/oom_kill.c           |   26 +++++++++++++++++++-------
 5 files changed, 39 insertions(+), 9 deletions(-)

index 0644a15..a85b196 100644
@@ -1567,8 +1567,10 @@ int compat_do_execve(char * filename,
 	return retval;
 
 out:
-	if (bprm->mm)
+	if (bprm->mm) {
+		set_exec_mm(NULL);
 		mmput(bprm->mm);
+	}
 
 out_file:
 	if (bprm->file) {

index 94dabd2..2395d10 100644
@@ -347,6 +347,8 @@ int bprm_mm_init(struct linux_binprm *bprm)
 	if (err)
 		goto err;
 
+	set_exec_mm(mm);
+
 	return 0;
 
 err:

@@ -759,6 +761,7 @@ static int exec_mmap(struct mm_struct *mm)
 	tsk->mm = mm;
 	tsk->active_mm = mm;
 	activate_mm(active_mm, mm);
+	tsk->signal->in_exec_mm = NULL;
 	task_unlock(tsk);
 	arch_pick_mmap_layout(mm);
 	if (old_mm) {

@@ -1328,6 +1331,15 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 
 EXPORT_SYMBOL(search_binary_handler);
 
+void set_exec_mm(struct mm_struct *mm)
+{
+	struct task_struct *leader = current->group_leader;
+
+	task_lock(leader);
+	leader->signal->in_exec_mm = mm;
+	task_unlock(leader);
+}
+
 /*
  * sys_execve() executes a new program.
  */

@@ -1416,8 +1428,10 @@ int do_execve(const char * filename,
 	return retval;
 
 out:
-	if (bprm->mm)
+	if (bprm->mm) {
+		set_exec_mm(NULL);
 		mmput (bprm->mm);
+	}
 
 out_file:
 	if (bprm->file) {

index a065612..2fde1ba 100644
@@ -133,6 +133,7 @@ extern void install_exec_creds(struct linux_binprm *bprm);
 extern void do_coredump(long signr, int exit_code, struct pt_regs *regs);
 extern void set_binfmt(struct linux_binfmt *new);
 extern void free_bprm(struct linux_binprm *);
+extern void set_exec_mm(struct mm_struct *mm);
 
 #endif /* __KERNEL__ */
 #endif /* _LINUX_BINFMTS_H */

index ac65605..b880931 100644
@@ -630,6 +630,7 @@ struct signal_struct {
 	struct mutex cred_guard_mutex;	/* guard against foreign influences on
 					 * credential calculations
 					 * (notably. ptrace) */
+	struct mm_struct *in_exec_mm;	/* temporary nascent mm in execve */
 };
 
 /* Context switch must be unlocked if interrupts are to be enabled */

index d58925e..830065f 100644
@@ -120,6 +120,15 @@ struct task_struct *find_lock_task_mm(struct task_struct *p)
 	return NULL;
 }
 
+/*
+ * The baseline for the badness score is the proportion of RAM that each
+ * task's rss and swap space use.
+ */
+static unsigned long oom_rss_swap_usage(struct mm_struct *mm)
+{
+	return get_mm_rss(mm) + get_mm_counter(mm, MM_SWAPENTS);
+}
+
 /* return true if the task is not adequate as candidate victim task. */
 static bool oom_unkillable_task(struct task_struct *p,
 		const struct mem_cgroup *mem, const nodemask_t *nodemask)

@@ -151,7 +160,7 @@ static bool oom_unkillable_task(struct task_struct *p,
 unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *mem,
 			  const nodemask_t *nodemask)
 {
-	unsigned long points;
+	unsigned long points = 0;
 	unsigned long points_orig;
 	int oom_adj = p->signal->oom_adj;
 	long oom_score_adj = p->signal->oom_score_adj;

@@ -169,15 +178,18 @@ unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *mem,
 	if (p->flags & PF_OOM_ORIGIN)
 		return ULONG_MAX;
 
+	/* The task is now processing execve(). then it has second mm */
+	if (unlikely(p->signal->in_exec_mm)) {
+		task_lock(p->group_leader);
+		if (p->signal->in_exec_mm)
+			points = oom_rss_swap_usage(p->signal->in_exec_mm);
+		task_unlock(p->group_leader);
+	}
+
 	p = find_lock_task_mm(p);
 	if (!p)
 		return 0;
-
-	/*
-	 * The baseline for the badness score is the proportion of RAM that each
-	 * task's rss and swap space use.
-	 */
-	points = (get_mm_rss(p->mm) + get_mm_counter(p->mm, MM_SWAPENTS));
+	points += oom_rss_swap_usage(p->mm);
 	task_unlock(p);
 
 	/*

1.6.5.2



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

On 25 Oct 2010 at 12:29, KOSAKI Motohiro wrote:

hi,

i've got a few comments/questions about the whole approach, see them inline.

Hi

Thank you for reviewing.

patch 3/4 prevent this race :)
now, 3/4 move cred_guard_mutex into signal struct. and execve() take signal->cred_guard_mutex for protecting concurent execve race.

Ah, sorry. that's my mistake. I thought you've reviewed this one at my last posting.

can you please see 3/4? the URL is below.

http://www.gossamer-threads.com/lists/linux/kernel/1293297?do=post_view_threaded

Sorry, I haven't got this point. can you please elaborate this worse scenario?

As I said at previous discussion, It is possible and one of option. and I've made the patch of this way too at once. But, It is messy than current. because pages in nascent mm are also swappable. then, a swapping-out of such page need to update both mm->rss_stat and nascent_mm->rss_stat. IOW, we need to change VM core. But, actually, execve vs OOM race is very rarely event, then, I don't hope to add some new branch and complexity.

Note: before 2.6.35, oom_kill.c track amount of process virtual address space. then changing get_arg_page() is enough. but on 2.6.36 or later, oom_kill.c track amount of process rss. then we can't ignore swap in/out event. and changing get_arg_page() is not enough. Or, Do you propse new OOM account mm->rss + nascent_mm->total_vm? this can be easily. but tricky more.

So, I think this is one of trade-off issue. If you have better patch rather than me, I'm glad to accept your one and join to review it. However myself don't plan to take this approach.

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Stupid question.

Can't we just account these allocations in the old -mm temporary?

IOW. Please look at the "patch" below. It is of course incomplete and wrong (to the point inc_mm_counter() is not safe without SPLIT_RSS_COUNTING), and copy_strings/flush_old_exec are not the best places to play with mm-counters, just to explain what I mean.

It is very simple. copy_strings() increments MM_ANONPAGES every time we add a new page into bprm->vma. This makes this memory visible to select_bad_process().

When exec changes ->mm (or if it fails), we change MM_ANONPAGES counter back.

Most probably I missed something, but what do you think?

Oleg.

@@ -29,6 +29,7 @@ struct linux_binprm{
 	char buf[BINPRM_BUF_SIZE];
 #ifdef CONFIG_MMU
 	struct vm_area_struct *vma;
+	unsigned long mm_anonpages;
 #else
 # define MAX_ARG_PAGES	32
 	struct page *page[MAX_ARG_PAGES];

@@ -457,6 +457,9 @@ static int copy_strings(int argc, const 
 					goto out;
 				}
 
+				bmrp->mm_anonpages--;
+				inc_mm_counter(current->mm, MM_ANONPAGES);
+
 				if (kmapped_page) {
 					flush_kernel_dcache_page(kmapped_page);
 					kunmap(kmapped_page);

@@ -1003,6 +1006,7 @@ int flush_old_exec(struct linux_binprm *
 	/*
 	 * Release all of the old mmap stuff
 	 */
+	add_mm_counter(current->mm, bprm->mm_anonpages);
 	retval = exec_mmap(bprm->mm);
 	if (retval)
 		goto out;

@@ -1426,8 +1430,10 @@ int do_execve(const char * filename,
 	return retval;
 
 out:
-	if (bprm->mm)
-		mmput (bprm->mm);
+	if (bprm->mm) {
+		add_mm_counter(current->mm, bprm->mm_anonpages);
+		mmput(bprm->mm);
+	}
 
 out_file:
 	if (bprm->file) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Hi

Because, If the pages of argv is swapping out when processing execve, This accouing doesn't work.

Of cource, changing swapping-out logic is one of way. But I did hope no VM core logic change. taking implict mlocking argv area during execve is also one of option. But I did think implicit mlocking is more risky.

Is this enough explanation? Please don't hesitate say "no". If people don't like my approach, I don't hesitate change my thinking.

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Why?

If copy_strings() inserts the new page into bprm->vma and then this page is swapped out, inc_mm_counter(current->mm, MM_ANONPAGES) becomes incorrect, yes. And we can't turn it into MM_SWAPENTS.

But does this really matter? oom_badness() counts MM_ANONPAGES + MM_SWAPENTS, and result is the same.

Well, certainly I can't say no ;)

But it would be nice to find a more simple fix (if it can work, of course).

And. I need a simple solution for the older kernels.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Ah, I got it. I did too strongly get stucked correct accounting. but you mean it's not must.

Okey, I'll tackle this one at this weekend hopefully.

Alright. It is certinally considerable one.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Yes. In fact, I _think_ this patch makes accounting better, even if the extra MM_ANONPAGES numbers are not 100% correct.

Even if we add signal->in_exec_mm, nobody except oom_badness() will look at it.

With this patch, say, /proc/pid/statm or /proc/pid/status will report the memory allocated by the execing task. Even if technically this is not correct (and 'swap' part may be wrong), this makes sense imho. Otherwise, there is no way to see that this task allocates (may be a lot) of memory.

This can "confuse" update_hiwater_rss(), but imho this is fine too.

Great! I'll send the patch tomorrow.

Even if you prefer another fix for 2.6.37/stable, I'd like to see your review to know if it is correct or not (for backporting).

Oleg.

OK, what do you think about the patch below?

Seems to work, with this patch the test-case doesn't kill the system (sysctl_oom_kill_allocating_task == 0).

I didn't dare to change !CONFIG_MMU case, I do not know how to test it.

The patch is not complete, compat_copy_strings() needs changes. But, shouldn't it use get_arg_page() too? Otherwise, where do we check RLIMIT_STACK?

The patch asks for the cleanups. In particular, I think exec_mmap() should accept bprm, not mm. But I'd prefer to do this later.

Oleg.

 include/linux/binfmts.h |    1 +
 fs/exec.c               |   28 ++++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

@@ -33,6 +33,7 @@ struct linux_binprm{
 # define MAX_ARG_PAGES	32
 	struct page *page[MAX_ARG_PAGES];
 #endif
+	unsigned long vma_pages;
 	struct mm_struct *mm;
 	unsigned long p; /* current top of mem */
 	unsigned int

@@ -162,6 +162,25 @@ out:
   	return error;
 }
 
+static void acct_arg_size(struct linux_binprm *bprm, unsigned long pages)
+{
+	struct mm_struct *mm = current->mm;
+	long diff = pages - bprm->vma_pages;
+
+	if (!mm || !diff)
+		return;
+
+	bprm->vma_pages += diff;
+
+#ifdef SPLIT_RSS_COUNTING
+	add_mm_counter(mm, MM_ANONPAGES, diff);
+#else
+	spin_lock(&mm->page_table_lock);
+	add_mm_counter(mm, MM_ANONPAGES, diff);
+	spin_unlock(&mm->page_table_lock);
+#endif
+}
+
 #ifdef CONFIG_MMU
 
 static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,

@@ -186,6 +205,8 @@ static struct page *get_arg_page(struct 
 		unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start;
 		struct rlimit *rlim;
 
+		acct_arg_size(bprm, size / PAGE_SIZE);
+
 		/*
 		 * We've historically supported up to 32 pages (ARG_MAX)
 		 * of argument strings even with small stacks

@@ -1003,6 +1024,7 @@ int flush_old_exec(struct linux_binprm *
 	/*
 	 * Release all of the old mmap stuff
 	 */
+	acct_arg_size(bprm, 0);
 	retval = exec_mmap(bprm->mm);
 	if (retval)
 		goto out;

@@ -1426,8 +1448,10 @@ int do_execve(const char * filename,
 	return retval;
 
 out:
-	if (bprm->mm)
-		mmput (bprm->mm);
+	if (bprm->mm) {
+		acct_arg_size(bprm, 0);
+		mmput(bprm->mm);
+	}
 
 out_file:
 	if (bprm->file) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Great. Thanks a lot.

Because NOMMU doesn't have variable length argv. Instead it is still using MAX_ARG_STRLEN as old MMU code.

32 pages hard coded argv limitation naturally prevent this nascent mm issue.

General request. Please consider to keep Brad's reported-by tag.

Ah, I didn't mean NOMMU. I meant compat_execve()->compat_copy_strings(). If a 32bit process execs we seem to miss the RLIMIT_STACK check, no?

Yes, yes, sure.

OK.

Well it does, to revert the MM_ANONPAGES counter. I'll add the empty function for NOMMU.

OK.

Yes, we already killed all sub-threads. But this doesn't mean nobody else can use current->mm, think about CLONE_VM. The simplest example is vfork().

Yes.

Thanks Kosaki!

I'll resend v2 today. I am still not sure about compat_copy_strings()...

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

OK, please see below, just for your review.

I was going to sent it "officially" with the changelog/etc, but

Yes, I think it needs the same checks. It should use get_arg_page() or we need more copy-and-paste code, I think it should also check fatal_signal_pending() like copy_strings() does.

I was going to export get_arg_page/acct_arg_size, but it is so ugly. I'll try to find the way to unify copy_strings and compat_copy_strings, not sure it is possible to do cleanly.

Probably this needs a separate patch in any case.

Oleg.

Changes:

- move acct_arg_size() under CONFIG_MMU

- add the "nop" version for NOMMMU

 include/linux/binfmts.h |    1 +
 fs/exec.c               |   32 ++++++++++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 2 deletions(-)

@@ -29,6 +29,7 @@ struct linux_binprm{
 	char buf[BINPRM_BUF_SIZE];
 #ifdef CONFIG_MMU
 	struct vm_area_struct *vma;
+	unsigned long vma_pages;
 #else
 # define MAX_ARG_PAGES	32
 	struct page *page[MAX_ARG_PAGES];

@@ -164,6 +164,25 @@ out:
 
 #ifdef CONFIG_MMU
 
+static void acct_arg_size(struct linux_binprm *bprm, unsigned long pages)
+{
+	struct mm_struct *mm = current->mm;
+	long diff = (long)(pages - bprm->vma_pages);
+
+	if (!mm || !diff)
+		return;
+
+	bprm->vma_pages = pages;
+
+#ifdef SPLIT_RSS_COUNTING
+	add_mm_counter(mm, MM_ANONPAGES, diff);
+#else
+	spin_lock(&mm->page_table_lock);
+	add_mm_counter(mm, MM_ANONPAGES, diff);
+	spin_unlock(&mm->page_table_lock);
+#endif
+}
+
 static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
 		int write)
 {

@@ -186,6 +205,8 @@ static struct page *get_arg_page(struct 
 		unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start;
 		struct rlimit *rlim;
 
+		acct_arg_size(bprm, size / PAGE_SIZE);
+
 		/*
 		 * We've historically supported up to 32 pages (ARG_MAX)
 		 * of argument strings even with small stacks

@@ -276,6 +297,10 @@ static bool valid_arg_len(struct linux_b
 
 #else
 
+static inline void acct_arg_size(struct linux_binprm *bprm, unsigned long pages)
+{
+}
+
 static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
 		int write)
 {

@@ -1003,6 +1028,7 @@ int flush_old_exec(struct linux_binprm *
 	/*
 	 * Release all of the old mmap stuff
 	 */
+	acct_arg_size(bprm, 0);
 	retval = exec_mmap(bprm->mm);
 	if (retval)
 		goto out;

@@ -1426,8 +1452,10 @@ int do_execve(const char * filename,
 	return retval;
 
 out:
-	if (bprm->mm)
-		mmput (bprm->mm);
+	if (bprm->mm) {
+		acct_arg_size(bprm, 0);
+		mmput(bprm->mm);
+	}
 
 out_file:
 	if (bprm->file) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

But I think this is the only option for 2.6.37/stable.

So. I am sending 2 patches, hopefully they fix the problems and there are simple enough for 2.6.27/stable.

I'll send the cleanups which unify compat/non-compat code on top of these fixes, this is not stable material.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Brad Spengler published a local memory-allocation DoS that evades the OOM-killer (though not the virtual memory RLIMIT):
http://www.grsecurity.net/~spender/64bit_dos.c

execve()->copy_strings() can allocate a lot of memory, but this is not visible to oom-killer, nobody can see the nascent bprm->mm and take it into account.

With this patch get_arg_page() increments current's MM_ANONPAGES counter every time we allocate the new page for argv/envp. When do_execve() succeds or fails, we change this counter back.

Technically this is not 100% correct, we can't know if the new page is swapped out and turn MM_ANONPAGES into MM_SWAPENTS, but I don't think this really matters and everything becomes correct once exec changes ->mm or fails.

By-discussion-with: KOSAKI Motohiro <[email protected]>

---

 include/linux/binfmts.h |    1 +
 fs/exec.c               |   32 ++++++++++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 2 deletions(-)

@@ -29,6 +29,7 @@ struct linux_binprm{
 	char buf[BINPRM_BUF_SIZE];
 #ifdef CONFIG_MMU
 	struct vm_area_struct *vma;
+	unsigned long vma_pages;
 #else
 # define MAX_ARG_PAGES	32
 	struct page *page[MAX_ARG_PAGES];

@@ -164,6 +164,25 @@ out:
 
 #ifdef CONFIG_MMU
 
+static void acct_arg_size(struct linux_binprm *bprm, unsigned long pages)
+{
+	struct mm_struct *mm = current->mm;
+	long diff = (long)(pages - bprm->vma_pages);
+
+	if (!mm || !diff)
+		return;
+
+	bprm->vma_pages = pages;
+
+#ifdef SPLIT_RSS_COUNTING
+	add_mm_counter(mm, MM_ANONPAGES, diff);
+#else
+	spin_lock(&mm->page_table_lock);
+	add_mm_counter(mm, MM_ANONPAGES, diff);
+	spin_unlock(&mm->page_table_lock);
+#endif
+}
+
 static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
 		int write)
 {

@@ -186,6 +205,8 @@ static struct page *get_arg_page(struct 
 		unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start;
 		struct rlimit *rlim;
 
+		acct_arg_size(bprm, size / PAGE_SIZE);
+
 		/*
 		 * We've historically supported up to 32 pages (ARG_MAX)
 		 * of argument strings even with small stacks

@@ -276,6 +297,10 @@ static bool valid_arg_len(struct linux_b
 
 #else
 
+static inline void acct_arg_size(struct linux_binprm *bprm, unsigned long pages)
+{
+}
+
 static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
 		int write)
 {

@@ -1003,6 +1028,7 @@ int flush_old_exec(struct linux_binprm *
 	/*
 	 * Release all of the old mmap stuff
 	 */
+	acct_arg_size(bprm, 0);
 	retval = exec_mmap(bprm->mm);
 	if (retval)
 		goto out;

@@ -1426,8 +1452,10 @@ int do_execve(const char * filename,
 	return retval;
 
 out:
-	if (bprm->mm)
-		mmput (bprm->mm);
+	if (bprm->mm) {
+		acct_arg_size(bprm, 0);
+		mmput(bprm->mm);
+	}
 
 out_file:
 	if (bprm->file) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Looks good to me.

Thank you very much.

Agreed, this needs a comment.

The patch was already applied, I'll send a separate one on top of the next "unify exec/compat" series. Or, I'll add the comments into this series, depending on review.

Thanks,

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Note: this patch targets 2.6.37 and tries to be as simple as possible. That is why it adds more copy-and-paste horror into fs/compat.c and uglifies fs/exec.c, this will be cleanuped later.

compat_copy_strings() plays with bprm->vma/mm directly and thus has two problems: it lacks the RLIMIT_STACK check and argv/envp memory is not visible to oom killer.

Export acct_arg_size() and get_arg_page(), change compat_copy_strings() to use get_arg_page(), change compat_do_execve() to do acct_arg_size(0) as do_execve() does.

Add the fatal_signal_pending/cond_resched checks into compat_count() and compat_copy_strings(), this matches the code in fs/exec.c and certainly makes sense.

---

 include/linux/binfmts.h |    4 ++++
 fs/exec.c               |    8 ++++----
 fs/compat.c             |   28 +++++++++++++++-------------
 3 files changed, 23 insertions(+), 17 deletions(-)

@@ -60,6 +60,10 @@ struct linux_binprm{
 	unsigned long loader, exec;
 };
 
+extern void acct_arg_size(struct linux_binprm *bprm, unsigned long pages);
+extern struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
+					int write);
+
 #define BINPRM_FLAGS_ENFORCE_NONDUMP_BIT 0
 #define BINPRM_FLAGS_ENFORCE_NONDUMP (1 << BINPRM_FLAGS_ENFORCE_NONDUMP_BIT)
 

@@ -164,7 +164,7 @@ out:
 
 #ifdef CONFIG_MMU
 
-static void acct_arg_size(struct linux_binprm *bprm, unsigned long pages)
+void acct_arg_size(struct linux_binprm *bprm, unsigned long pages)
 {
 	struct mm_struct *mm = current->mm;
 	long diff = (long)(pages - bprm->vma_pages);

@@ -183,7 +183,7 @@ static void acct_arg_size(struct linux_b
 #endif
 }
 
-static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
+struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
 		int write)
 {
 	struct page *page;

@@ -297,11 +297,11 @@ static bool valid_arg_len(struct linux_b
 
 #else
 
-static inline void acct_arg_size(struct linux_binprm *bprm, unsigned long pages)
+void acct_arg_size(struct linux_binprm *bprm, unsigned long pages)
 {
 }
 
-static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
+struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
 		int write)
 {
 	struct page *page;

@@ -1350,6 +1350,10 @@ static int compat_count(compat_uptr_t __
 			argv++;
 			if (i++ >= max)
 				return -E2BIG;
+
+			if (fatal_signal_pending(current))
+				return -ERESTARTNOHAND;
+			cond_resched();
 		}
 	}
 	return i;

@@ -1391,6 +1395,12 @@ static int compat_copy_strings(int argc,
 		while (len > 0) {
 			int offset, bytes_to_copy;
 
+			if (fatal_signal_pending(current)) {
+				ret = -ERESTARTNOHAND;
+				goto out;
+			}
+			cond_resched();
+
 			offset = pos % PAGE_SIZE;
 			if (offset == 0)
 				offset = PAGE_SIZE;

@@ -1407,18 +1417,8 @@ static int compat_copy_strings(int argc,
 			if (!kmapped_page || kpos != (pos & PAGE_MASK)) {
 				struct page *page;
 
-#ifdef CONFIG_STACK_GROWSUP
-				ret = expand_stack_downwards(bprm->vma, pos);
-				if (ret < 0) {
-					/* We've exceed the stack rlimit. */
-					ret = -E2BIG;
-					goto out;
-				}
-#endif
-				ret = get_user_pages(current, bprm->mm, pos,
-						     1, 1, 1, &page, NULL);
-				if (ret <= 0) {
-					/* We've exceed the stack rlimit. */
+				page = get_arg_page(bprm, pos, 1);
+				if (!page) {
 					ret = -E2BIG;
 					goto out;
 				}

@@ -1539,8 +1539,10 @@ int compat_do_execve(char * filename,
 	return retval;
 
 out:
-	if (bprm->mm)
+	if (bprm->mm) {
+		acct_arg_size(bprm, 0);
 		mmput(bprm->mm);
+	}
 
 out_file:
 	if (bprm->file) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Looks good to me.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

(remove stable)

On top of

[PATCH 1/2] exec: make argv/envp memory visible to oom-killer [PATCH 2/2] exec: copy-and-paste the fixes into compat_do_execve() paths

Imho, execve code in fs/compat.c must die. It is very hard to maintain this copy-and-paste horror.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Introduce get_arg_ptr() helper, convert count() and copy_strings() to use it.

No functional changes, preparation. This helper is trivial, it just reads the pointer from argv/envp user-space array.

---

 fs/exec.c |   36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

@@ -390,6 +390,17 @@ err:
 	return err;
 }
 
+static const char __user *
+get_arg_ptr(const char __user * const __user *argv, int argc)
+{
+	const char __user *ptr;
+
+	if (get_user(ptr, argv + argc))
+		return ERR_PTR(-EFAULT);
+
+	return ptr;
+}
+
 /*
  * count() counts the number of strings in array ARGV.
  */

@@ -399,13 +410,14 @@ static int count(const char __user * con
 
 	if (argv != NULL) {
 		for (;;) {
-			const char __user * p;
+			const char __user *p = get_arg_ptr(argv, i);
 
-			if (get_user(p, argv))
-				return -EFAULT;
 			if (!p)
 				break;
-			argv++;
+
+			if (IS_ERR(p))
+				return -EFAULT;
+
 			if (i++ >= max)
 				return -E2BIG;
 

@@ -435,16 +447,18 @@ static int copy_strings(int argc, const 
 		int len;
 		unsigned long pos;
 
-		if (get_user(str, argv+argc) ||
-				!(len = strnlen_user(str, MAX_ARG_STRLEN))) {
-			ret = -EFAULT;
+		ret = -EFAULT;
+		str = get_arg_ptr(argv, argc);
+		if (IS_ERR(str))
 			goto out;
-		}
 
-		if (!valid_arg_len(bprm, len)) {
-			ret = -E2BIG;
+		len = strnlen_user(str, MAX_ARG_STRLEN);
+		if (!len)
+			goto out;
+
+		ret = -E2BIG;
+		if (!valid_arg_len(bprm, len))
 			goto out;
-		}
 
 		/* We're going to work our way backwords. */
 		pos = bprm->p;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No functional changes, preparation to simplify the review.

And the new (and currently unused) "bool compat" argument to get_arg_ptr(), count(), and copy_strings().

Add this argument to do_execve() as well, and rename it to do_execve_common().

Reintroduce do_execve() as a trivial wrapper() on top of do_execve_common(compat => false).

---

 fs/exec.c |   33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

@@ -391,7 +391,7 @@ err:
 }
 
 static const char __user *
-get_arg_ptr(const char __user * const __user *argv, int argc)
+get_arg_ptr(const char __user * const __user *argv, int argc, bool compat)
 {
 	const char __user *ptr;
 

@@ -404,13 +404,13 @@ get_arg_ptr(const char __user * const __
 /*
  * count() counts the number of strings in array ARGV.
  */
-static int count(const char __user * const __user * argv, int max)
+static int count(const char __user * const __user *argv, int max, bool compat)
 {
 	int i = 0;
 
 	if (argv != NULL) {
 		for (;;) {
-			const char __user *p = get_arg_ptr(argv, i);
+			const char __user *p = get_arg_ptr(argv, i, compat);
 
 			if (!p)
 				break;

@@ -435,7 +435,7 @@ static int count(const char __user * con
  * ensures the destination page is created and not swapped out.
  */
 static int copy_strings(int argc, const char __user *const __user *argv,
-			struct linux_binprm *bprm)
+			struct linux_binprm *bprm, bool compat)
 {
 	struct page *kmapped_page = NULL;
 	char *kaddr = NULL;

@@ -448,7 +448,7 @@ static int copy_strings(int argc, const 
 		unsigned long pos;
 
 		ret = -EFAULT;
-		str = get_arg_ptr(argv, argc);
+		str = get_arg_ptr(argv, argc, compat);
 		if (IS_ERR(str))
 			goto out;
 

@@ -531,7 +531,8 @@ int copy_strings_kernel(int argc, const 
 	int r;
 	mm_segment_t oldfs = get_fs();
 	set_fs(KERNEL_DS);
-	r = copy_strings(argc, (const char __user *const  __user *)argv, bprm);
+	r = copy_strings(argc, (const char __user *const  __user *)argv,
+				bprm, false);
 	set_fs(oldfs);
 	return r;
 }

@@ -1382,10 +1383,10 @@ EXPORT_SYMBOL(search_binary_handler);
 /*
  * sys_execve() executes a new program.
  */
-int do_execve(const char * filename,
+static int do_execve_common(const char *filename,
 	const char __user *const __user *argv,
 	const char __user *const __user *envp,
-	struct pt_regs * regs)
+	struct pt_regs *regs, bool compat)
 {
 	struct linux_binprm *bprm;
 	struct file *file;

@@ -1427,11 +1428,11 @@ int do_execve(const char * filename,
 	if (retval)
 		goto out_file;
 
-	bprm->argc = count(argv, MAX_ARG_STRINGS);
+	bprm->argc = count(argv, MAX_ARG_STRINGS, compat);
 	if ((retval = bprm->argc) < 0)
 		goto out;
 
-	bprm->envc = count(envp, MAX_ARG_STRINGS);
+	bprm->envc = count(envp, MAX_ARG_STRINGS, compat);
 	if ((retval = bprm->envc) < 0)
 		goto out;
 

@@ -1444,11 +1445,11 @@ int do_execve(const char * filename,
 		goto out;
 
 	bprm->exec = bprm->p;
-	retval = copy_strings(bprm->envc, envp, bprm);
+	retval = copy_strings(bprm->envc, envp, bprm, compat);
 	if (retval < 0)
 		goto out;
 
-	retval = copy_strings(bprm->argc, argv, bprm);
+	retval = copy_strings(bprm->argc, argv, bprm, compat);
 	if (retval < 0)
 		goto out;
 

@@ -1492,6 +1493,14 @@ out_ret:
 	return retval;
 }
 
+int do_execve(const char *filename,
+	const char __user *const __user *argv,
+	const char __user *const __user *envp,
+	struct pt_regs *regs)
+{
+	return do_execve_common(filename, argv, envp, regs, false);
+}
+
 void set_binfmt(struct linux_binfmt *new)
 {
 	struct mm_struct *mm = current->mm;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Teach get_arg_ptr() to handle compat = T case correctly.

This allows us to remove the compat_do_execve() code from fs/compat.c and reimplement compat_do_execve() as the trivial wrapper on top of do_execve_common(compat => true).

In fact, this fixes another (minor) bug. "compat_uptr_t str" can overflow after "str += len" in compat_copy_strings() if a 64bit application execs via sys32_execve().

---

 fs/exec.c   |   25 ++++++
 fs/compat.c |  235 ------------------------------------------------------------
 2 files changed, 25 insertions(+), 235 deletions(-)

@@ -55,6 +55,7 @@
 #include <linux/fs_struct.h>
 #include <linux/pipe_fs_i.h>
 #include <linux/oom.h>
+#include <linux/compat.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>

@@ -395,6 +396,18 @@ get_arg_ptr(const char __user * const __
 {
 	const char __user *ptr;
 
+#ifdef CONFIG_COMPAT
+	if (unlikely(compat)) {
+		compat_uptr_t __user *a = (void __user*)argv;
+		compat_uptr_t p;
+
+		if (get_user(p, a + argc))
+			return ERR_PTR(-EFAULT);
+
+		return compat_ptr(p);
+	}
+#endif
+
 	if (get_user(ptr, argv + argc))
 		return ERR_PTR(-EFAULT);
 

@@ -1501,6 +1514,18 @@ int do_execve(const char *filename,
 	return do_execve_common(filename, argv, envp, regs, false);
 }
 
+#ifdef CONFIG_COMPAT
+int compat_do_execve(char * filename,
+	compat_uptr_t __user *argv,
+	compat_uptr_t __user *envp,
+	struct pt_regs * regs)
+{
+	return do_execve_common(filename,
+				(void __user*)argv, (void __user*)envp,
+				regs, true);
+}
+#endif
+
 void set_binfmt(struct linux_binfmt *new)
 {
 	struct mm_struct *mm = current->mm;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

This applies to almost every likely/unlikely, and I think that compat processes should fall into "unlikely category". But I don't really mind, I can remove this hint, I added it mostly as documentation.

I'll recheck, but I don't think so. Please note that compat_ptr() accepts "compat_uptr_t", not "compat_uptr_t *".

argv should be correct as a pointer to user-space, otherwise the current code is buggy. For example, compat_do_execve() passes argv to compat_count() which does get_user(argv) without any conversion.

IOW, even if this should be fixed, I think this have nothing to do with this patch. But I'll recheck, thanks.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

So, once again, this should not (and can not) be compat_ptr(argv) afaics.

I don't understand the s390 asm, but compat_wrapper.S:sys32_execve_wrapper looks correct. If not, the current code is already buggy and s390 should be fixed. argv/envp are not compat ptrs, they just point to compat_ data, we should not do any conversion.

I am resending this series unchanged, plus the trivial 5/5 to document acct_arg_size().

----------------------------------------------------------------------

execve code in fs/compat.c must die. It is very hard to maintain this copy-and-paste horror. And the only reason for this duplication is that argv/envp point to char* or compat_uptr_t depending on compat. We can add the trivial helper which hides the difference and unify the code.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Introduce get_arg_ptr() helper, convert count() and copy_strings() to use it.

No functional changes, preparation. This helper is trivial, it just reads the pointer from argv/envp user-space array.

---

 fs/exec.c |   36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

@@ -395,6 +395,17 @@ err:
 	return err;
 }
 
+static const char __user *
+get_arg_ptr(const char __user * const __user *argv, int argc)
+{
+	const char __user *ptr;
+
+	if (get_user(ptr, argv + argc))
+		return ERR_PTR(-EFAULT);
+
+	return ptr;
+}
+
 /*
  * count() counts the number of strings in array ARGV.
  */

@@ -404,13 +415,14 @@ static int count(const char __user * con
 
 	if (argv != NULL) {
 		for (;;) {
-			const char __user * p;
+			const char __user *p = get_arg_ptr(argv, i);
 
-			if (get_user(p, argv))
-				return -EFAULT;
 			if (!p)
 				break;
-			argv++;
+
+			if (IS_ERR(p))
+				return -EFAULT;
+
 			if (i++ >= max)
 				return -E2BIG;
 

@@ -440,16 +452,18 @@ static int copy_strings(int argc, const 
 		int len;
 		unsigned long pos;
 
-		if (get_user(str, argv+argc) ||
-				!(len = strnlen_user(str, MAX_ARG_STRLEN))) {
-			ret = -EFAULT;
+		ret = -EFAULT;
+		str = get_arg_ptr(argv, argc);
+		if (IS_ERR(str))
 			goto out;
-		}
 
-		if (!valid_arg_len(bprm, len)) {
-			ret = -E2BIG;
+		len = strnlen_user(str, MAX_ARG_STRLEN);
+		if (!len)
+			goto out;
+
+		ret = -E2BIG;
+		if (!valid_arg_len(bprm, len))
 			goto out;
-		}
 
 		/* We're going to work our way backwords. */
 		pos = bprm->p;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No functional changes, preparation to simplify the review.

And the new (and currently unused) "bool compat" argument to get_arg_ptr(), count(), and copy_strings().

Add this argument to do_execve() as well, and rename it to do_execve_common().

Reintroduce do_execve() as a trivial wrapper() on top of do_execve_common(compat => false).

---

 fs/exec.c |   33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

@@ -396,7 +396,7 @@ err:
 }
 
 static const char __user *
-get_arg_ptr(const char __user * const __user *argv, int argc)
+get_arg_ptr(const char __user * const __user *argv, int argc, bool compat)
 {
 	const char __user *ptr;
 

@@ -409,13 +409,13 @@ get_arg_ptr(const char __user * const __
 /*
  * count() counts the number of strings in array ARGV.
  */
-static int count(const char __user * const __user * argv, int max)
+static int count(const char __user * const __user *argv, int max, bool compat)
 {
 	int i = 0;
 
 	if (argv != NULL) {
 		for (;;) {
-			const char __user *p = get_arg_ptr(argv, i);
+			const char __user *p = get_arg_ptr(argv, i, compat);
 
 			if (!p)
 				break;

@@ -440,7 +440,7 @@ static int count(const char __user * con
  * ensures the destination page is created and not swapped out.
  */
 static int copy_strings(int argc, const char __user *const __user *argv,
-			struct linux_binprm *bprm)
+			struct linux_binprm *bprm, bool compat)
 {
 	struct page *kmapped_page = NULL;
 	char *kaddr = NULL;

@@ -453,7 +453,7 @@ static int copy_strings(int argc, const 
 		unsigned long pos;
 
 		ret = -EFAULT;
-		str = get_arg_ptr(argv, argc);
+		str = get_arg_ptr(argv, argc, compat);
 		if (IS_ERR(str))
 			goto out;
 

@@ -536,7 +536,8 @@ int copy_strings_kernel(int argc, const 
 	int r;
 	mm_segment_t oldfs = get_fs();
 	set_fs(KERNEL_DS);
-	r = copy_strings(argc, (const char __user *const  __user *)argv, bprm);
+	r = copy_strings(argc, (const char __user *const  __user *)argv,
+				bprm, false);
 	set_fs(oldfs);
 	return r;
 }

@@ -1387,10 +1388,10 @@ EXPORT_SYMBOL(search_binary_handler);
 /*
  * sys_execve() executes a new program.
  */
-int do_execve(const char * filename,
+static int do_execve_common(const char *filename,
 	const char __user *const __user *argv,
 	const char __user *const __user *envp,
-	struct pt_regs * regs)
+	struct pt_regs *regs, bool compat)
 {
 	struct linux_binprm *bprm;
 	struct file *file;

@@ -1432,11 +1433,11 @@ int do_execve(const char * filename,
 	if (retval)
 		goto out_file;
 
-	bprm->argc = count(argv, MAX_ARG_STRINGS);
+	bprm->argc = count(argv, MAX_ARG_STRINGS, compat);
 	if ((retval = bprm->argc) < 0)
 		goto out;
 
-	bprm->envc = count(envp, MAX_ARG_STRINGS);
+	bprm->envc = count(envp, MAX_ARG_STRINGS, compat);
 	if ((retval = bprm->envc) < 0)
 		goto out;
 

@@ -1449,11 +1450,11 @@ int do_execve(const char * filename,
 		goto out;
 
 	bprm->exec = bprm->p;
-	retval = copy_strings(bprm->envc, envp, bprm);
+	retval = copy_strings(bprm->envc, envp, bprm, compat);
 	if (retval < 0)
 		goto out;
 
-	retval = copy_strings(bprm->argc, argv, bprm);
+	retval = copy_strings(bprm->argc, argv, bprm, compat);
 	if (retval < 0)
 		goto out;
 

@@ -1497,6 +1498,14 @@ out_ret:
 	return retval;
 }
 
+int do_execve(const char *filename,
+	const char __user *const __user *argv,
+	const char __user *const __user *envp,
+	struct pt_regs *regs)
+{
+	return do_execve_common(filename, argv, envp, regs, false);
+}
+
 void set_binfmt(struct linux_binfmt *new)
 {
 	struct mm_struct *mm = current->mm;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

I think this is wrong.

If you introduce the "bool compat" thing, you should also change the type of the argument pointers to some opaque type at the same time. It's no longer really a

const char __user *const __user *

pointer at that point. Trying to claim it is, is just wrong. The type suddently becomes conditional on that 'compat' variable.

Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Yes, this is true.

And I agree this could be done in more clean way, just we need more changed. Please see the next email.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Teach get_arg_ptr() to handle compat = T case correctly.

This allows us to remove the compat_do_execve() code from fs/compat.c and reimplement compat_do_execve() as the trivial wrapper on top of do_execve_common(compat => true).

In fact, this fixes another (minor) bug. "compat_uptr_t str" can overflow after "str += len" in compat_copy_strings() if a 64bit application execs via sys32_execve().

---

 fs/exec.c   |   25 ++++++
 fs/compat.c |  235 ------------------------------------------------------------
 2 files changed, 25 insertions(+), 235 deletions(-)

@@ -55,6 +55,7 @@
 #include <linux/fs_struct.h>
 #include <linux/pipe_fs_i.h>
 #include <linux/oom.h>
+#include <linux/compat.h>
 
 #include <asm/uaccess.h>
 #include <asm/mmu_context.h>

@@ -400,6 +401,18 @@ get_arg_ptr(const char __user * const __
 {
 	const char __user *ptr;
 
+#ifdef CONFIG_COMPAT
+	if (unlikely(compat)) {
+		compat_uptr_t __user *a = (void __user *)argv;
+		compat_uptr_t p;
+
+		if (get_user(p, a + argc))
+			return ERR_PTR(-EFAULT);
+
+		return compat_ptr(p);
+	}
+#endif
+
 	if (get_user(ptr, argv + argc))
 		return ERR_PTR(-EFAULT);
 

@@ -1506,6 +1519,18 @@ int do_execve(const char *filename,
 	return do_execve_common(filename, argv, envp, regs, false);
 }
 
+#ifdef CONFIG_COMPAT
+int compat_do_execve(char *filename,
+	compat_uptr_t __user *argv,
+	compat_uptr_t __user *envp,
+	struct pt_regs *regs)
+{
+	return do_execve_common(filename,
+				(void __user *)argv, (void __user*)envp,
+				regs, true);
+}
+#endif
+
 void set_binfmt(struct linux_binfmt *new)
 {
 	struct mm_struct *mm = current->mm;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Does it?

  typedef union {
     compat_uptr_t compat;
     const char __user *native;
   } conditional_user_ptr_t;

  int compat_do_execve(char *filename,
                  compat_uptr_t argv,
                  compat_uptr_t envp,
                  struct pt_regs *regs)
   {
             return do_execve_common(filename,
                      compat_ptr(argv), compat_ptr(envp), regs);

I think it does.

Personally I don't really like this union, to me "void __user*" looks better, but I won't insist.

Indeed! But, again, this has nothing to do with this series. We can do this later and change the callers in arch/.

Once again, to me "void __user*" looks better (just simpler). In this case get_arg_ptr() becomes (without const/__user for the clarity)

	void *get_arg_ptr(void **argv, int argc, bool compat)
	{
		char *ptr;

	#ifdef CONFIG_COMPAT
		if (unlikely(compat)) {
			compat_uptr_t *a = argv;
			compat_uptr_t p;

			if (get_user(p, a + argc))
				return ERR_PTR(-EFAULT);

			return compat_ptr(p);
		}
	#endif

		if (get_user(ptr, argv + argc))
			return ERR_PTR(-EFAULT);

		return ptr;
	}

Otherwise, get_arg_ptr() should return conditional_user_ptr_t as well, this looks like the unnecessary complication to me, but of course this is subjective.

So, what do you think?

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No, this is not true, I am stupid.

Still,

Yes, please.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Umm. "void __user *" may look simpler/better, but it's WRONG.

Using "const char __user *const __user *" is correct - but only for the non-compat case.

And using "void __user *" may result in compiling code, but it will have lost all actual information about the type. We don't do that in the kernel if we can avoid it, because "void *" basically does no type checking. Sure, sometimes it's the only thing we can do, but _if_ we have a type, we should use it.

And that "union" really is the true type. You are passing a user pointer down that can be either of those members.

So if you think it looks ugly, then you shouldn't do that "conditional compat argument at run-time at all". Because the real ugliness of the type comes not from the type, but from the fact that you pass a pointer that can contain two different things.

No.

I simply won't apply that. It's WRONG. It's wrong because you've dropped all the type information.

With the right union,

No it shouldn't. The get_arg_ptr() should always just return the actual pointer. It will have _resolved_ the ambiguity! That's what the "compat_ptr()" thing does in the return case inside teh CONFIG_COMPAT.

So the correct way to do this is something like the following (yeah, maybe I got the syntax wrong, I didn't test this, I just wrote it in my MUA):

void *get_arg_ptr(const union compat_ptr_union __user *argv,
int argc, bool compat)

       {
               char *ptr;

        #ifdef CONFIG_COMPAT
               if (unlikely(compat)) {
                       compat_uptr_t p;

                       if (get_user(p, &argv->compat + argc))
                               return ERR_PTR(-EFAULT);

                       return compat_ptr(p);
               }
        #endif

               if (get_user(ptr, &argv->noncompat +argc))
                       return ERR_PTR(-EFAULT);

               return ptr;
       }

and notice how it gets the types right, and it even has one line LESS than your version, exactly because it gets the types right and doesn't need that implied cast in your

     compat_uptr_t *a = argv;

(in fact, I think your version needs an _explicit_ cast in order to not get a warning: you can't just cast "void **" to something else).

See? The advantage of the union is that the types are correct, which means that the casts are unnecessary.

The advantage of the union is also that you see what is going on, and it's clear from the function prototype that this doesn't just take a random user pointer, it takes a user pointer to something that can be two different types.

See? Correct typing is important.

Linus

(Yes I am stupid, see the next email).

Yes, and get_user(argv) in the !compat case doesn't look nice, I agree.

My point was, apart from the trivial get_arg_ptr() helper, nobody else uses this argv/envp, so I thought it is OK to drop the type info and use "void *".

But as I said, I won't insist. I'll redo/resend.

Thanks.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Well, yes... But it turns out I didn't actually read what you proposed.

	typedef union {
		compat_uptr_t compat;
		const char __user *native;
	} conditional_user_ptr_t;

	...

	where that 'do_execve_common()' takes it's arguments as

		union conditional_user_ptr_t __user *argv,
		union conditional_user_ptr_t __user *envp

I hope you didn't really mean this...

OK, we have two kinds of pointers, the union makes sense. But I think we do not want the 3rd kind, pointer to the union. This can't help to avoid the casts. Yes, get_arg_ptr() can do

	&argv->native

but this still means the cast even if looks differently (and tricky).

And. How can we pass "argv" from do_execve() to do_execve_common() ? We need another cast.

So. If you insist you prefer the pointer to the union - no need to convince me. Just say this and I'll redo again.

This patch does:

	typedef union {
		const char __user *const __user *native;
		compat_uptr_t __user *compat;
	} conditional_user_ptr_t;

	static int do_execve_common(const char *filename,
			conditional_user_ptr_t argv,
			conditional_user_ptr_t envp,
			struct pt_regs *regs, bool compat)

get_arg_ptr() does argv.native/compat, this looks more understandable.

Do you agree?

copy_strings_kernel() still needs the cast, but this is only because we want to add "__user" for annotation.

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Introduce get_arg_ptr() helper, convert count() and copy_strings() to use it.

No functional changes, preparation. This helper is trivial, it just reads the pointer from argv/envp user-space array.

---

 fs/exec.c |   36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

@@ -395,6 +395,17 @@ err:
 	return err;
 }
 
+static const char __user *
+get_arg_ptr(const char __user * const __user *argv, int argc)
+{
+	const char __user *ptr;
+
+	if (get_user(ptr, argv + argc))
+		return ERR_PTR(-EFAULT);
+
+	return ptr;
+}
+
 /*
  * count() counts the number of strings in array ARGV.
  */

@@ -404,13 +415,14 @@ static int count(const char __user * con
 
 	if (argv != NULL) {
 		for (;;) {
-			const char __user * p;
+			const char __user *p = get_arg_ptr(argv, i);
 
-			if (get_user(p, argv))
-				return -EFAULT;
 			if (!p)
 				break;
-			argv++;
+
+			if (IS_ERR(p))
+				return -EFAULT;
+
 			if (i++ >= max)
 				return -E2BIG;
 

@@ -440,16 +452,18 @@ static int copy_strings(int argc, const 
 		int len;
 		unsigned long pos;
 
-		if (get_user(str, argv+argc) ||
-				!(len = strnlen_user(str, MAX_ARG_STRLEN))) {
-			ret = -EFAULT;
+		ret = -EFAULT;
+		str = get_arg_ptr(argv, argc);
+		if (IS_ERR(str))
 			goto out;
-		}
 
-		if (!valid_arg_len(bprm, len)) {
-			ret = -E2BIG;
+		len = strnlen_user(str, MAX_ARG_STRLEN);
+		if (!len)
+			goto out;
+
+		ret = -E2BIG;
+		if (!valid_arg_len(bprm, len))
 			goto out;
-		}
 
 		/* We're going to work our way backwords. */
 		pos = bprm->p;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

No functional changes, preparation to simplify the review.

And the new (and currently unused) "bool compat" argument to get_arg_ptr(), count(), and copy_strings().

Add this argument to do_execve() as well, and rename it to do_execve_common().

Reintroduce do_execve() as a trivial wrapper() on top of do_execve_common(compat => false).

---

 fs/exec.c |   33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

@@ -396,7 +396,7 @@ err:
 }
 
 static const char __user *
-get_arg_ptr(const char __user * const __user *argv, int argc)
+get_arg_ptr(const char __user * const __user *argv, int argc, bool compat)
 {
 	const char __user *ptr;