break out page allocation warning code

mm/vmalloc.c renamed from mm/vmalloc.c~vmalloc-warn +0 -0

 		if (unlikely(!page)) {
 			/* Successfully allocated i pages, free them in __vunmap() */

@@ -1579,6 +1581,9 @@ static void *__vmalloc_area_node(struct
 	return area->addr;

fail:

mm/vmalloc.c renamed from mm/vmalloc.c~vmalloc-warn +3 -0

+	warn_alloc_failed(gfp_mask, order, "vmalloc: allocation failure, "
+			  "allocated %ld of %ld bytes\n",
+			  (area->nr_pages*PAGE_SIZE), area->size);
 	vfree(area->addr);
 	return NULL;
 }

_

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

Review static void *__vmalloc_node(unsigned lon · line 1537

Quoted 9 lines · Dave Hansen

diff -puN mm/vmalloc.c~vmalloc-warn mm/vmalloc.c
--- linux-2.6.git/mm/vmalloc.c~vmalloc-warn 2011-04-15
08:49:06.823306620 -0700
+++ linux-2.6.git-dave/mm/vmalloc.c 2011-04-15 09:20:17.926460283 -0700

(quoted diff) +1 -0

@@ -1534,6 +1534,7 @@ static void *__vmalloc_node(unsigned lon
 static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 				 pgprot_t prot, int node, void *caller)
 {
+	int order = 0;

Could we make that const?

diff -puN mm/vmalloc.c~vmalloc-warn mm/vmalloc.c

mm/vmalloc.c renamed from mm/vmalloc.c~vmalloc-warn +4 -2

@@ -1534,6 +1534,7 @@ static void *__vmalloc_node(unsigned lon
 static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 				 pgprot_t prot, int node, void *caller)
 {
+	const int order = 0;
 	struct page **pages;
 	unsigned int nr_pages, array_size, i;
 	gfp_t nested_gfp = (gfp_mask & GFP_RECLAIM_MASK) | __GFP_ZERO;

@@ -1560,11 +1561,12 @@ static void *__vmalloc_area_node(struct 
 
 	for (i = 0; i < area->nr_pages; i++) {
 		struct page *page;
+		gfp_t tmp_mask = gfp_mask | __GFP_NOWARN;
 
 		if (node < 0)
-			page = alloc_page(gfp_mask);
+			page = alloc_page(tmp_mask);
 		else
-			page = alloc_pages_node(node, gfp_mask, 0);
+			page = alloc_pages_node(node, tmp_mask, order);
 
 		if (unlikely(!page)) {
 			/* Successfully allocated i pages, free them in __vunmap() */

mm/vmalloc.c static void *__vmalloc_area_node(struct · line 1589

Quoted 7 lines

fail:

mm/vmalloc.c renamed from mm/vmalloc.c~vmalloc-warn +3 -0

+     warn_alloc_failed(gfp_mask, order, "vmalloc: allocation failure, "
+                       "allocated %ld of %ld bytes\n",
+                       (area->nr_pages*PAGE_SIZE), area->size);
      vfree(area->addr);
      return NULL;
 }

Sorry, I still don't understand why this isn't just a three-liner patch to call warn_alloc_failed(). I don't see the benefit of the "order" or "tmp_mask" variables at all, they'll just be removed next time someone goes down the mm/* directory and looks for variables that are used only once or are unchanged as a cleanup.

This originally started as a simple patch to give vmalloc() some more verbose output on failure on top of the plain page allocator messages. Johannes suggested that it might be nicer to lead with the vmalloc() info _before_ the page allocator messages.

But, I do think there's a lot of value in what __alloc_pages_slowpath() does with its filtering and so forth.

This patch creates a new function which other allocators can call instead of relying on the internal page allocator warnings. It also gives this function private rate-limiting which separates it from other printk_ratelimit() users.

Signed-off-by Dave Hansen <[email protected]>

---

 linux-2.6.git-dave/include/linux/mm.h |    2 +
 linux-2.6.git-dave/mm/page_alloc.c    |   63 ++++++++++++++++++++++------------
 2 files changed, 44 insertions(+), 21 deletions(-)

diff -puN include/linux/mm.h~break-out-alloc-failure-messages include/linux/mm.h

include/linux/mm.h renamed from include/linux/mm.h~break-out-alloc-failure-messages +2 -0

@@ -1365,6 +1365,8 @@ extern void si_meminfo(struct sysinfo * 
 extern void si_meminfo_node(struct sysinfo *val, int nid);
 extern int after_bootmem;
 
+extern void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...);
+
 extern void setup_per_cpu_pageset(void);
 
 extern void zone_pcp_update(struct zone *zone);

diff -puN mm/page_alloc.c~break-out-alloc-failure-messages mm/page_alloc.c

mm/page_alloc.c renamed from mm/page_alloc.c~break-out-alloc-failure-messages +1 -0

@@ -54,6 +54,7 @@
 #include <trace/events/kmem.h>
 #include <linux/ftrace_event.h>
 #include <linux/memcontrol.h>
+#include <linux/ratelimit.h>
 
 #include <asm/tlbflush.h>
 #include <asm/div64.h>

@@ -1734,6 +1735,46 @@ static inline bool should_suppress_show_
 	return ret;
 }
 

mm/page_alloc.c renamed from mm/page_alloc.c~break-out-alloc-failure-messages +14 -0

+	if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs))
+		return;
+
+	/*
+	 * This documents exceptions given to allocations in certain
+	 * contexts that are allowed to allocate outside current's set
+	 * of allowed nodes.
+	 */
+	if (!(gfp_mask & __GFP_NOMEMALLOC))
+		if (test_thread_flag(TIF_MEMDIE) ||
+		    (current->flags & (PF_MEMALLOC | PF_EXITING)))
+			filter &= ~SHOW_MEM_FILTER_NODES;
+	if (in_interrupt() || !wait)
+		filter &= ~SHOW_MEM_FILTER_NODES;

mm/page_alloc.c renamed from mm/page_alloc.c~break-out-alloc-failure-messages +6 -15

+
+	dump_stack();
+	if (!should_suppress_show_mem())
+		show_mem(filter);
+}
+
 static inline int
 should_alloc_retry(gfp_t gfp_mask, unsigned int order,
 				unsigned long pages_reclaimed)

@@ -2176,27 +2217,7 @@ rebalance:
 	}
 
 nopage:
-	if (!(gfp_mask & __GFP_NOWARN) && printk_ratelimit()) {
-		unsigned int filter = SHOW_MEM_FILTER_NODES;
-
-		/*
-		 * This documents exceptions given to allocations in certain
-		 * contexts that are allowed to allocate outside current's set
-		 * of allowed nodes.
-		 */
-		if (!(gfp_mask & __GFP_NOMEMALLOC))
-			if (test_thread_flag(TIF_MEMDIE) ||
-			    (current->flags & (PF_MEMALLOC | PF_EXITING)))
-				filter &= ~SHOW_MEM_FILTER_NODES;
-		if (in_interrupt() || !wait)
-			filter &= ~SHOW_MEM_FILTER_NODES;
-

mm/page_alloc.c static inline bool should_suppress_show_ · line 1747

Quoted 6 lines

mm/page_alloc.c renamed from mm/page_alloc.c~break-out-alloc-failure-messages +6 -0

+void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...)
+{
+	va_list args;
+	unsigned int filter = SHOW_MEM_FILTER_NODES;
+	const gfp_t wait = gfp_mask & __GFP_WAIT;
+

"wait" is unnecessary. You didn't do "const gfp_t nowarn = gfp_mask & __GFP_NOWARN;" for the same reason.

Quoted 18 lines

mm/page_alloc.c renamed from mm/page_alloc.c~break-out-alloc-failure-messages +18 -0

+	if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs))
+		return;
+
+	/*
+	 * This documents exceptions given to allocations in certain
+	 * contexts that are allowed to allocate outside current's set
+	 * of allowed nodes.
+	 */
+	if (!(gfp_mask & __GFP_NOMEMALLOC))
+		if (test_thread_flag(TIF_MEMDIE) ||
+		    (current->flags & (PF_MEMALLOC | PF_EXITING)))
+			filter &= ~SHOW_MEM_FILTER_NODES;
+	if (in_interrupt() || !wait)
+		filter &= ~SHOW_MEM_FILTER_NODES;
+
+	if (fmt) {
+		printk(KERN_WARNING);
+		va_start(args, fmt);

mm/page_alloc.c static inline bool should_suppress_show_ · line 1771

Quoted 6 lines

mm/page_alloc.c renamed from mm/page_alloc.c~break-out-alloc-failure-messages +6 -0

+		vprintk(fmt, args);
+		va_end(args);
+	}
+
+	printk(KERN_WARNING "%s: page allocation failure: order:%d, mode:0x%x\n",
+			current->comm, order, gfp_mask);

pr_warning()?

current->comm should always be printed with get_task_comm() to avoid racing with /proc/pid/comm. Since this function can be called potentially deep in the stack, you may need to serialize this with a statically-allocated buffer.

Quoted 9 lines

mm/page_alloc.c static inline bool should_suppress_show_ · line 1747

Quoted 6 lines

mm/page_alloc.c renamed from mm/page_alloc.c~break-out-alloc-failure-messages +6 -0

+void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...)
+{
+	va_list args;
+	unsigned int filter = SHOW_MEM_FILTER_NODES;
+	const gfp_t wait = gfp_mask & __GFP_WAIT;
+

"wait" is unnecessary. You didn't do "const gfp_t nowarn = gfp_mask & __GFP_NOWARN;" for the same reason.

This line is just a copy from the __alloc_pages_slowpath() one. I guess we only use it once, so I've got no problem killing it.

Quoted 18 lines

Quoted 18 lines

mm/page_alloc.c renamed from mm/page_alloc.c~break-out-alloc-failure-messages +18 -0

+	if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs))
+		return;
+
+	/*
+	 * This documents exceptions given to allocations in certain
+	 * contexts that are allowed to allocate outside current's set
+	 * of allowed nodes.
+	 */
+	if (!(gfp_mask & __GFP_NOMEMALLOC))
+		if (test_thread_flag(TIF_MEMDIE) ||
+		    (current->flags & (PF_MEMALLOC | PF_EXITING)))
+			filter &= ~SHOW_MEM_FILTER_NODES;
+	if (in_interrupt() || !wait)
+		filter &= ~SHOW_MEM_FILTER_NODES;
+
+	if (fmt) {
+		printk(KERN_WARNING);
+		va_start(args, fmt);

Quoted 8 lines

mm/page_alloc.c static inline bool should_suppress_show_ · line 1771

Quoted 6 lines

mm/page_alloc.c renamed from mm/page_alloc.c~break-out-alloc-failure-messages +6 -0

+		vprintk(fmt, args);
+		va_end(args);
+	}
+
+	printk(KERN_WARNING "%s: page allocation failure: order:%d, mode:0x%x\n",
+			current->comm, order, gfp_mask);

pr_warning()?

OK, I'll change it back.

Quoted 4 lines

current->comm should always be printed with get_task_comm() to avoid racing with /proc/pid/comm. Since this function can be called potentially deep in the stack, you may need to serialize this with a statically-allocated buffer.

This code was already in page_alloc.c. I'm simply breaking it out here trying to keep the changes down to what is needed minimally to move the code. Correcting this preexisting problem sounds like a great follow-on patch.

It shouldn't be a follow-on patch since you're introducing a new feature here (vmalloc allocation failure warnings) and what I'm identifying is a race in the access to current->comm. A bug fix for a race should always preceed a feature that touches the same code.

Quoted 4 lines

It shouldn't be a follow-on patch since you're introducing a new feature here (vmalloc allocation failure warnings) and what I'm identifying is a race in the access to current->comm. A bug fix for a race should always preceed a feature that touches the same code.

Dude. Seriously. Glass house! a63d83f4

- provide a statically-allocated buffer to use for get_task_comm() and copy current->comm over before printing it, or

- take task_lock(current) to protect against /proc/pid/comm.

The latter probably isn't safe because we could potentially already be holding task_lock(current) during a GFP_ATOMIC page allocation.

It shouldn't be a follow-on patch since you're introducing a new feature here (vmalloc allocation failure warnings) and what I'm identifying is a race in the access to current->comm. A bug fix for a race should always preceed a feature that touches the same code.

Quoted 4 lines

It shouldn't be a follow-on patch since you're introducing a new feature here (vmalloc allocation failure warnings) and what I'm identifying is a race in the access to current->comm. A bug fix for a race should always preceed a feature that touches the same code.

So, what's the race here? kmemleak.c says?

/*
* There is a small chance of a race with set_task_comm(),
* however using get_task_comm() here may cause locking
* dependency issues with current->alloc_lock. In the worst
* case, the command line is not correct.
*/
strncpy(object->comm, current->comm, sizeof(object->comm));

We're trying to make sure we don't print out a partially updated tsk->comm? Or, is there a bigger issue here like potential oopses or kernel information leaks.

1. We require that no memory allocator ever holds the task lock for the current task, and we audit all the existing GFP_ATOMIC users in the kernel to ensure they're not doing it now. In the case of a problem, we end up with a hung kernel while trying to get a message out to the console.
2. We remove current->comm from the printk(), and deal with the information loss.
3. We live with corrupted output, like the other ~400 in-kernel users of ->comm do. (I'm assuming that very few of them hold the task lock). In the case of a race, we get junk on the console, but an otherwise fine bug report (the way it is now).
4. We come up with some way to print out current->comm, without holding any task locks. We could do this by copying it somewhere safe on each context switch. Could probably also do it with RCU.

There's also a very, very odd message in fs/exec.c:

/*
* Threads may access current->comm without holding
* the task lock, so write the string carefully.
* Readers without a lock may see incomplete new
* names but are safe from non-terminating string reads. */

The rule is,

1) writing comm
need task_lock
2) read _another_ thread's comm
need task_lock
3) read own comm
no need task_lock

Quoted 10 lines

The rule is,

1) writing comm
need task_lock
2) read _another_ thread's comm
need task_lock
3) read own comm
no need task_lock

That was true a while ago, but you now need to protect every thread's ->comm with get_task_comm() or ensuring task_lock() is held to protect against /proc/pid/comm which can change other thread's ->comm. That was different before when prctl(PR_SET_NAME) would only operate on current, so no lock was needed when reading current->comm.

Quoted 5 lines

That was true a while ago, but you now need to protect every thread's ->comm with get_task_comm() or ensuring task_lock() is held to protect against /proc/pid/comm which can change other thread's ->comm. That was different before when prctl(PR_SET_NAME) would only operate on current, so no lock was needed when reading current->comm.

Right. /proc/pid/comm is evil. We have to fix it. otherwise we need change all of current->comm user. It's very lots!

Quoted 9 lines · KOSAKI Motohiro

Quoted 5 lines

That was true a while ago, but you now need to protect every thread's ->comm with get_task_comm() or ensuring task_lock() is held to protect against /proc/pid/comm which can change other thread's ->comm. That was different before when prctl(PR_SET_NAME) would only operate on current, so no lock was needed when reading current->comm.

Right. /proc/pid/comm is evil. We have to fix it. otherwise we need change all of current->comm user. It's very lots!

Fixing it in this case would be removing it and only allowing it for current via the usual prctl() :) The code was introduced in 4614a696bd1c (procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm) in December 2009 and seems to originally be meant for debugging. We simply can't continue to let it modify any thread's ->comm unless we change the over 300 current->comm deferences in the kernel.

I'd prefer that we remove /proc/pid/comm entirely or at least prevent writing to it unless CONFIG_EXPERT.

Quoted 19 lines · KOSAKI Motohiro

Quoted 10 lines

Quoted 6 lines

That was true a while ago, but you now need to protect every thread's ->comm with get_task_comm() or ensuring task_lock() is held to protect against /proc/pid/comm which can change other thread's ->comm. That was different before when prctl(PR_SET_NAME) would only operate on current, so no lock was needed when reading current->comm.

Right. /proc/pid/comm is evil. We have to fix it. otherwise we need change all of current->comm user. It's very lots!

Fixing it in this case would be removing it and only allowing it for current via the usual prctl() :) The code was introduced in 4614a696bd1c (procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm) in December 2009 and seems to originally be meant for debugging. We simply can't continue to let it modify any thread's ->comm unless we change the over 300 current->comm deferences in the kernel.

Quoted 2 lines

I'd prefer that we remove /proc/pid/comm entirely or at least prevent writing to it unless CONFIG_EXPERT.

Eeeh. That's probably going to be a tough sell, as I think there is wider interest in what it provides. Its useful for debugging applications not kernels, so I doubt folks will want to rebuild their kernel to try to analyze a java issue.

So I'm well aware that there is the chance that you catch the race and read an incomplete/invalid comm (it was discussed at length when the change went in), but somewhere I've missed how that's causing actual problems. Other then just being "evil" and having the documented race, could you clarify what the issue is that your hitting?

Quoted 8 lines

Quoted 2 lines

I'd prefer that we remove /proc/pid/comm entirely or at least prevent writing to it unless CONFIG_EXPERT.

Eeeh. That's probably going to be a tough sell, as I think there is wider interest in what it provides. Its useful for debugging applications not kernels, so I doubt folks will want to rebuild their kernel to try to analyze a java issue.

Quoted 5 lines

So I'm well aware that there is the chance that you catch the race and read an incomplete/invalid comm (it was discussed at length when the change went in), but somewhere I've missed how that's causing actual problems. Other then just being "evil" and having the documented race, could you clarify what the issue is that your hitting?

The problem is, there is no documented as well. Okay, I recognized you introduced new locking rule for task->comm. But there is no documented it. Thus, We have no way to review current callsites are correct or not. Can you please do it? And, I have a question. Do you mean now task->comm reader don't need task_lock() even if it is another thread?

_if_ every task->comm reader have to realize it has a chance to read incomplete/invalid comm, task_lock() doesn't makes any help.

Quoted 28 lines · David Rientjes

Quoted 19 lines · KOSAKI Motohiro

Quoted 10 lines

Quoted 6 lines

That was true a while ago, but you now need to protect every thread's ->comm with get_task_comm() or ensuring task_lock() is held to protect against /proc/pid/comm which can change other thread's ->comm. That was different before when prctl(PR_SET_NAME) would only operate on current, so no lock was needed when reading current->comm.

Right. /proc/pid/comm is evil. We have to fix it. otherwise we need change all of current->comm user. It's very lots!

Fixing it in this case would be removing it and only allowing it for current via the usual prctl() :) The code was introduced in 4614a696bd1c (procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm) in December 2009 and seems to originally be meant for debugging. We simply can't continue to let it modify any thread's ->comm unless we change the over 300 current->comm deferences in the kernel.

Quoted 2 lines

I'd prefer that we remove /proc/pid/comm entirely or at least prevent writing to it unless CONFIG_EXPERT.

Eeeh. That's probably going to be a tough sell, as I think there is wider interest in what it provides. Its useful for debugging applications not kernels, so I doubt folks will want to rebuild their kernel to try to analyze a java issue.

Quoted 5 lines

So I'm well aware that there is the chance that you catch the race and read an incomplete/invalid comm (it was discussed at length when the change went in), but somewhere I've missed how that's causing actual problems. Other then just being "evil" and having the documented race, could you clarify what the issue is that your hitting?

The problem is, there is no documented as well. Okay, I recognized you introduced new locking rule for task->comm. But there is no documented it. Thus, We have no way to review current callsites are correct or not. Can you please do it? And, I have a question. Do you mean now task->comm reader don't need task_lock() even if it is another thread?

_if_ every task->comm reader have to realize it has a chance to read incomplete/invalid comm, task_lock() doesn't makes any help.

Sorry if this somehow got off on the wrong foot. Its just surprising to see such passion bubble up after almost two years of quiet since the proc patch went in.

So I'm not proposing comm be totally lock free (Dave Hansen might do that for me, we'll see :) but when the original patch was proposed, the idea that transient empty or incomplete comms would be possible was brought up and didn't seem to be a big enough issue at the time to block it from being merged.

Its just having a more specific case where these transient null/incomplete comms causes an issue would help prioritize the need for correctness.

In the meantime, I'll put some effort into trying to protect unlocked current->comm acccess using get_task_comm() where possible. Won't happen in a day, and help would be appreciated.

Quoted 4 lines · john stultz

In the meantime, I'll put some effort into trying to protect unlocked current->comm acccess using get_task_comm() where possible. Won't happen in a day, and help would be appreciated.

We need to stop protecting ->comm with ->alloc_lock since it is used for other members of task_struct that may or may not be held in a function that wants to read ->comm. We should probably introduce a seqlock.

Quoted 9 lines · David Rientjes

Quoted 4 lines · john stultz

In the meantime, I'll put some effort into trying to protect unlocked current->comm acccess using get_task_comm() where possible. Won't happen in a day, and help would be appreciated.

We need to stop protecting ->comm with ->alloc_lock since it is used for other members of task_struct that may or may not be held in a function that wants to read ->comm. We should probably introduce a seqlock.

Agreed. My initial approach is to consolidate accesses to use get_task_comm(), with special case to skip the locking if tsk==current, as well as a lock free __get_task_comm() for cases where its not current being accessed and the task locking is already done.

Once that's all done, the next step is to switch to a seqlock (or possibly RCU if Dave is still playing with that idea), internally in the get_task_comm implementation and then yank the special __get_task_comm.

So thinking further, this can be simplified by adding the seqlock first, and then retaining the task_locking only in the set_task_comm path until all comm accessors are converted to using get_task_comm.

Quoted 4 lines · john stultz

So thinking further, this can be simplified by adding the seqlock first, and then retaining the task_locking only in the set_task_comm path until all comm accessors are converted to using get_task_comm.

On second thought, I think it would be better to just retain using a spinlock but instead of using alloc_lock, introduce a new spinlock to task_struct for the sole purpose of protecting comm.

And, instead, of using get_task_comm() to write into a preallocated buffer, I think it would be easier in the vast majority of cases that you'll need to convert to just provide task_comm_lock(p) and task_comm_unlock(p) so that p->comm can be dereferenced safely.

get_task_comm() could use that interface itself and then write into a preallocated buffer.

The problem with using get_task_comm() everywhere is it requires 16 additional bytes to be allocated on the stack in hundreds of locations around the kernel which may or may not be safe.

Quoted 7 lines · David Rientjes

Quoted 4 lines · john stultz

So thinking further, this can be simplified by adding the seqlock first, and then retaining the task_locking only in the set_task_comm path until all comm accessors are converted to using get_task_comm.

Quoted 8 lines

On second thought, I think it would be better to just retain using a spinlock but instead of using alloc_lock, introduce a new spinlock to task_struct for the sole purpose of protecting comm.

And, instead, of using get_task_comm() to write into a preallocated buffer, I think it would be easier in the vast majority of cases that you'll need to convert to just provide task_comm_lock(p) and task_comm_unlock(p) so that p->comm can be dereferenced safely.

So my concern with this is that it means one more lock that could be mis-nested. By keeping the locking isolated to the get/set_task_comm, we can be sure that won't happen.

Also tracking new current->comm references will be easier if we just don't allow new ones. Validating that all the comm references are correctly locked becomes more difficult if we need locking at each use site.

Further, since I'm not convinced that we never reference current->comm from irq context, if we go with spinlocks, we're going to have to disable irqs in the read path as well. seqlocks were nice for that aspect.

And one correction.
------------------------------------------------------------------
static ssize_t comm_write(struct file *file, const char __user *buf,
size_t count, loff_t *offset)

{
        struct inode *inode = file->f_path.dentry->d_inode;
        struct task_struct *p;
        char buffer[TASK_COMM_LEN];

        memset(buffer, 0, sizeof(buffer));
        if (count > sizeof(buffer) - 1)
                count = sizeof(buffer) - 1;
        if (copy_from_user(buffer, buf, count))
                return -EFAULT;

        p = get_proc_task(inode);
        if (!p)
                return -ESRCH;

        if (same_thread_group(current, p))
                set_task_comm(p, buffer);
        else
                count = -EINVAL;

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

This code doesn't have proper credential check. IOW, you forgot to pthread_setuid_np() case.

Quoted 25 lines

And one correction.
------------------------------------------------------------------
static ssize_t comm_write(struct file *file, const char __user *buf,
size_t count, loff_t *offset)

{
        struct inode *inode = file->f_path.dentry->d_inode;
        struct task_struct *p;
        char buffer[TASK_COMM_LEN];

        memset(buffer, 0, sizeof(buffer));
        if (count > sizeof(buffer) - 1)
                count = sizeof(buffer) - 1;
        if (copy_from_user(buffer, buf, count))
                return -EFAULT;

        p = get_proc_task(inode);
        if (!p)
                return -ESRCH;

        if (same_thread_group(current, p))
                set_task_comm(p, buffer);
        else
                count = -EINVAL;

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

Quoted 2 lines

This code doesn't have proper credential check. IOW, you forgot to pthread_setuid_np() case.

Sorry, could you expand on this a bit? Google isn't coming up with much for pthread_setuid_np. Can a thread actually end up with different uid then the process it is a member of?

Or is same_thread_group not really what I think it is? What would be a better way to check that the two threads are members of the same process?

thanks
-john

Quoted 8 lines · KOSAKI Motohiro

The rule is,

1) writing comm
need task_lock
2) read _another_ thread's comm
need task_lock
3) read own comm
no need task_lock

That was true a while ago, but you now need to protect every thread's ->comm with get_task_comm() or ensuring task_lock() is held to protect against /proc/pid/comm which can change other thread's ->comm. That was different before when prctl(PR_SET_NAME) would only operate on current, so no lock was needed when reading current->comm.

Quoted 10 lines · KOSAKI Motohiro

Quoted 8 lines

The rule is,

1) writing comm
need task_lock
2) read _another_ thread's comm
need task_lock
3) read own comm
no need task_lock

Quoted 5 lines

That was true a while ago, but you now need to protect every thread's ->comm with get_task_comm() or ensuring task_lock() is held to protect against /proc/pid/comm which can change other thread's ->comm. That was different before when prctl(PR_SET_NAME) would only operate on current, so no lock was needed when reading current->comm.

Everybody still goes through set_task_comm() to _set_ it, though. That means that the worst case scenario that we get is output truncated (possibly to nothing). We already have at least one existing user in mm/ (kmemleak) that thinks this is OK. I'd tend to err in the direction of taking a truncated or empty task name to possibly locking up the system.

There are also plenty of instances of current->comm going in to the kernel these days. I count 18 added since 2.6.37.

As for a long-term fix, locks probably aren't the answer. Would something like this completely untested patch work? It would have the added bonus that it keeps tsk->comm users working for the moment. We could eventually add an rcu_read_lock()-annotated access function.

The concept is ok to me. but AFAIK some caller are now using ARRAY_SIZE(tsk->comm). or sizeof(tsk->comm). Probably callers need to be changed too.

void set_task_comm(struct task_struct *tsk, char *buf) {

fs/exec.c renamed from fs/exec.c~tsk_comm +4 -0

+	char tmp_comm[TASK_COMM_LEN];
+
 	task_lock(tsk);
 
+	memcpy(tmp_comm, tsk->comm_buf, TASK_COMM_LEN);
+	tsk->comm = tmp;
 	/*

fs/exec.c char *get_task_comm(char *buf, struct ta · line 1019

Quoted 13 lines

fs/exec.c renamed from fs/exec.c~tsk_comm +4 -7

+     memcpy(tmp_comm, tsk->comm_buf, TASK_COMM_LEN);
+     tsk->comm = tmp;
      /*
-      * Threads may access current->comm without holding
-      * the task lock, so write the string carefully.
-      * Readers without a lock may see incomplete new
-      * names but are safe from non-terminating string reads.
+      * Make sure no one is still looking at tsk->comm_buf
       */
-     memset(tsk->comm, 0, TASK_COMM_LEN);
-     wmb();
-     strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+     synchronize_rcu();

The doc says,

/**
* synchronize_rcu - wait until a grace period has elapsed.