[PATCH 0/3] v5 Improve task->comm locking situation

[PATCH 0/3] v5 Improve task->comm locking situation

[John Stultz]

Ok. Here's v5. Hopefully this is getting close to something
that can be queued for 2.6.40.

Since my commit 4614a696bd1c3a9af3a08f0e5874830a85b889d4, the
current->comm value could be changed by other threads.

This changed the comm locking rules, which previously allowed for
unlocked current->comm access, since only the thread itself could
change its comm.

While this was brought up at the time, it was not considered
problematic, as the comm writing was done in such a way that
only null or incomplete comms could be read. However, recently
folks have made it clear they want to see this issue resolved.

So fair enough, as I opened this can of worms, I should work
to resolve it and this patchset is my initial attempt.

The proposed solution here is to introduce a new spinlock that
exclusively protects the comm value. We use it to serialize
access via get_task_comm() and set_task_comm(). Since some 
comm access is open-coded using the task lock, we preserve
the task locking in set_task_comm for now. Once all comm 
access is converted to using get_task_comm, we can clean that
up as well.

I've also introduced a printk %ptc accessor, which makes the
conversion to locked access simpler (as most uses are for printks)
as well as a checkpatch rule to try to catch any new current->comm
users from being introduced.

New in this version: Added a spin_lock_init in copy_process as
caught by Jiri. Also tweaked the regex as suggested by Joe.

Hopefully this will allow for a smooth transition, where we can
slowly fix up the unlocked current->comm access bit by bit,
reducing the race window with each patch, while not making the
situation any worse then it was yesterday.

Thanks for the comments and feedback so far. 
Any additional comments/feedback would still be appreciated.

thanks
-john


CC: Joe Perches <joe@perches.com>
CC: Michal Nazarewicz <mina86@mina86.com>
CC: Andy Whitcroft <apw@canonical.com>
CC: Jiri Slaby <jirislaby@gmail.com>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: David Rientjes <rientjes@google.com>
CC: Dave Hansen <dave@linux.vnet.ibm.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: linux-mm@kvack.org

John Stultz (3):
  comm: Introduce comm_lock spinlock to protect task->comm access
  printk: Add %ptc to safely print a task's comm
  checkpatch.pl: Add check for task comm references

 fs/exec.c                 |   19 ++++++++++++++++---
 include/linux/init_task.h |    1 +
 include/linux/sched.h     |    5 ++---
 kernel/fork.c             |    1 +
 lib/vsprintf.c            |   24 ++++++++++++++++++++++++
 scripts/checkpatch.pl     |    7 +++++++
 6 files changed, 51 insertions(+), 6 deletions(-)

-- 
1.7.3.2.146.gca209

[PATCH 1/3] comm: Introduce comm_lock spinlock to protect task->comm access

[John Stultz]

The implicit rules for current->comm access being safe without locking
are no longer true. Accessing current->comm without holding the task
lock may result in null or incomplete strings (however, access won't
run off the end of the string).

In order to properly fix this, I've introduced a comm_lock spinlock
which will protect comm access and modified get_task_comm() and
set_task_comm() to use it.

Since there are a number of cases where comm access is open-coded
safely grabbing the task_lock(), we preserve the task locking in
set_task_comm, so those users are also safe.

With this patch, users that access current->comm without a lock
are still prone to null/incomplete comm strings, but it should
be no worse then it is now.

The next step is to go through and convert all comm accesses to
use get_task_comm(). This is substantial, but can be done bit by
bit, reducing the race windows with each patch.

CC: Joe Perches <joe@perches.com>
CC: Michal Nazarewicz <mina86@mina86.com>
CC: Andy Whitcroft <apw@canonical.com>
CC: Jiri Slaby <jirislaby@gmail.com>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: David Rientjes <rientjes@google.com>
CC: Dave Hansen <dave@linux.vnet.ibm.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: linux-mm@kvack.org
Acked-by: David Rientjes <rientjes@google.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 fs/exec.c                 |   19 ++++++++++++++++---
 include/linux/init_task.h |    1 +
 include/linux/sched.h     |    5 ++---
 kernel/fork.c             |    1 +
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 5e62d26..34fa611 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -998,17 +998,28 @@ static void flush_old_files(struct files_struct * files)
 
 char *get_task_comm(char *buf, struct task_struct *tsk)
 {
-	/* buf must be at least sizeof(tsk->comm) in size */
-	task_lock(tsk);
+	unsigned long flags;
+
+	spin_lock_irqsave(&tsk->comm_lock, flags);
 	strncpy(buf, tsk->comm, sizeof(tsk->comm));
-	task_unlock(tsk);
+	spin_unlock_irqrestore(&tsk->comm_lock, flags);
 	return buf;
 }
 
 void set_task_comm(struct task_struct *tsk, char *buf)
 {
+	unsigned long flags;
+
+	/*
+	 * XXX - Even though comm is protected by comm_lock,
+	 * we take the task_lock here to serialize against
+	 * current users that directly access comm.
+	 * Once those users are removed, we can drop the
+	 * task locking & memsetting.
+	 */
 	task_lock(tsk);
 
+	spin_lock_irqsave(&tsk->comm_lock, flags);
 	/*
 	 * Threads may access current->comm without holding
 	 * the task lock, so write the string carefully.
@@ -1018,6 +1029,8 @@ void set_task_comm(struct task_struct *tsk, char *buf)
 	memset(tsk->comm, 0, TASK_COMM_LEN);
 	wmb();
 	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+	spin_unlock_irqrestore(&tsk->comm_lock, flags);
+
 	task_unlock(tsk);
 	perf_event_comm(tsk);
 }
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index caa151f..b69d94b 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -161,6 +161,7 @@ extern struct cred init_cred;
 	.group_leader	= &tsk,						\
 	RCU_INIT_POINTER(.real_cred, &init_cred),			\
 	RCU_INIT_POINTER(.cred, &init_cred),				\
+	.comm_lock	= __SPIN_LOCK_UNLOCKED(tsk.comm_lock),		\
 	.comm		= "swapper",					\
 	.thread		= INIT_THREAD,					\
 	.fs		= &init_fs,					\
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 18d63ce..f8a7cdf 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1333,10 +1333,9 @@ struct task_struct {
 	const struct cred __rcu *cred;	/* effective (overridable) subjective task
 					 * credentials (COW) */
 	struct cred *replacement_session_keyring; /* for KEYCTL_SESSION_TO_PARENT */
-
+	spinlock_t comm_lock;		/* protect's comm */
 	char comm[TASK_COMM_LEN]; /* executable name excluding path
-				     - access with [gs]et_task_comm (which lock
-				       it with task_lock())
+				     - access with [gs]et_task_comm
 				     - initialized normally by setup_new_exec */
 /* file system info */
 	int link_count, total_link_count;
diff --git a/kernel/fork.c b/kernel/fork.c
index e7548de..f53bf29 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1080,6 +1080,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	rcu_copy_process(p);
 	p->vfork_done = NULL;
 	spin_lock_init(&p->alloc_lock);
+	spin_lock_init(&p->comm_lock);
 
 	init_sigpending(&p->pending);
 
-- 
1.7.3.2.146.gca209

[PATCH 2/3] printk: Add %ptc to safely print a task's comm

[John Stultz]

Accessing task->comm requires proper locking. However in the past
access to current->comm could be done without locking. This
is no longer the case, so all comm access needs to be done
while holding the comm_lock.

In my attempt to clean up unprotected comm access, I've noticed
most comm access is done for printk output. To simplify correct
locking in these cases, I've introduced a new %ptc format,
which will print the corresponding task's comm.

Example use:
printk("%ptc: unaligned epc - sending SIGBUS.\n", current);

CC: Joe Perches <joe@perches.com>
CC: Michal Nazarewicz <mina86@mina86.com>
CC: Andy Whitcroft <apw@canonical.com>
CC: Jiri Slaby <jirislaby@gmail.com>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: David Rientjes <rientjes@google.com>
CC: Dave Hansen <dave@linux.vnet.ibm.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: linux-mm@kvack.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 lib/vsprintf.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index bc0ac6b..b7a9953 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -797,6 +797,23 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, uuid, spec);
 }
 
+static noinline_for_stack
+char *task_comm_string(char *buf, char *end, void *addr,
+			 struct printf_spec spec, const char *fmt)
+{
+	struct task_struct *tsk = addr;
+	char *ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tsk->comm_lock, flags);
+	ret = string(buf, end, tsk->comm, spec);
+	spin_unlock_irqrestore(&tsk->comm_lock, flags);
+
+	return ret;
+}
+
+
+
 int kptr_restrict = 1;
 
 /*
@@ -864,6 +881,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	}
 
 	switch (*fmt) {
+	case 't':
+		switch (fmt[1]) {
+		case 'c':
+			return task_comm_string(buf, end, ptr, spec, fmt);
+		}
+		break;
 	case 'F':
 	case 'f':
 		ptr = dereference_function_descriptor(ptr);
@@ -1151,6 +1174,7 @@ qualifier:
  *   http://tools.ietf.org/html/draft-ietf-6man-text-addr-representation-00
  * %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
  *   case.
+ * %ptc outputs the task's comm name
  * %n is ignored
  *
  * The return value is the number of characters which would
-- 
1.7.3.2.146.gca209

[PATCH 3/3] checkpatch.pl: Add check for task comm references

[John Stultz]

Now that accessing current->comm needs to be protected,
avoid new current->comm or other task->comm usage by adding
a warning to checkpatch.pl.

Fair warning: I know zero perl, so this was written in the
style of "monkey see, monkey do". It does appear to work
in my testing though.

Thanks to Jiri Slaby, Michal Nazarewicz and Joe Perches
for help improving the regex!

Close review and feedback would be appreciated.

CC: Joe Perches <joe@perches.com>
CC: Michal Nazarewicz <mina86@mina86.com>
CC: Andy Whitcroft <apw@canonical.com>
CC: Jiri Slaby <jirislaby@gmail.com>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: David Rientjes <rientjes@google.com>
CC: Dave Hansen <dave@linux.vnet.ibm.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: linux-mm@kvack.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 scripts/checkpatch.pl |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d867081..a67ea69 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2868,6 +2868,13 @@ sub process {
 			WARN("usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc\n" . $herecurr);
 		}
 
+# check for current->comm usage
+		our $common_comm_vars = qr{(?x:
+		        current|tsk|p|task|curr|chip|t|object|me
+		)};
+		if ($line =~ /\b($common_comm_vars)\s*->\s*comm\b/) {
+			WARN("comm access needs to be protected. Use get_task_comm, or printk's \%ptc formatting.\n" . $herecurr);
+		}
 # check for %L{u,d,i} in strings
 		my $string;
 		while ($line =~ /(?:^|")([X\t]*)(?:"|$)/g) {
-- 
1.7.3.2.146.gca209

Re: [PATCH 3/3] checkpatch.pl: Add check for task comm references

[Jiri Slaby]

On 05/17/2011 10:47 PM, John Stultz wrote:
> Now that accessing current->comm needs to be protected,
> avoid new current->comm or other task->comm usage by adding
> a warning to checkpatch.pl.
> 
> Fair warning: I know zero perl, so this was written in the
> style of "monkey see, monkey do". It does appear to work
> in my testing though.
> 
> Thanks to Jiri Slaby, Michal Nazarewicz and Joe Perches
> for help improving the regex!
> 
> Close review and feedback would be appreciated.
> 
> CC: Joe Perches <joe@perches.com>
> CC: Michal Nazarewicz <mina86@mina86.com>
> CC: Andy Whitcroft <apw@canonical.com>
> CC: Jiri Slaby <jirislaby@gmail.com>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: David Rientjes <rientjes@google.com>
> CC: Dave Hansen <dave@linux.vnet.ibm.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: linux-mm@kvack.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  scripts/checkpatch.pl |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d867081..a67ea69 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2868,6 +2868,13 @@ sub process {
>  			WARN("usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc\n" . $herecurr);
>  		}
>  
> +# check for current->comm usage
> +		our $common_comm_vars = qr{(?x:
> +		        current|tsk|p|task|curr|chip|t|object|me

Hrm, chip->comm looks like a total bullshit.
object->comm refers to kmemleak object, so this would trigger false
alarms too.

> +		)};
> +		if ($line =~ /\b($common_comm_vars)\s*->\s*comm\b/) {
> +			WARN("comm access needs to be protected. Use get_task_comm, or printk's \%ptc formatting.\n" . $herecurr);
> +		}
>  # check for %L{u,d,i} in strings
>  		my $string;
>  		while ($line =~ /(?:^|")([X\t]*)(?:"|$)/g) {

thanks,
-- 
js

Re: [PATCH 3/3] checkpatch.pl: Add check for task comm references

[Michal Nazarewicz]

On Tue, 17 May 2011 22:47:43 +0200, John Stultz wrote:
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d867081..a67ea69 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2868,6 +2868,13 @@ sub process {
>  			WARN("usage of NR_CPUS is often wrong - consider using  
> cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc\n" .  
> $herecurr);
>  		}
> +# check for current->comm usage
> +		our $common_comm_vars = qr{(?x:

It should by "my" not "our".

> +		        current|tsk|p|task|curr|chip|t|object|me
> +		)};

Also, I would stick it on a single line, ie.:

		my $comm_vars = qr/current|tsk|p|task|curr|chip|t|object|me/;

> +		if ($line =~ /\b($common_comm_vars)\s*->\s*comm\b/) {

The parens are not needed.

> +			WARN("comm access needs to be protected. Use get_task_comm, or  
> printk's \%ptc formatting.\n" . $herecurr);
> +		}

Empty line should be here.

>  # check for %L{u,d,i} in strings
>  		my $string;
>  		while ($line =~ /(?:^|")([X\t]*)(?:"|$)/g) {

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

Re: [PATCH 1/3] comm: Introduce comm_lock spinlock to protect task->comm access

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> The implicit rules for current->comm access being safe without locking are no 
> longer true. Accessing current->comm without holding the task lock may result 
> in null or incomplete strings (however, access won't run off the end of the 
> string).

This is rather unfortunate - task->comm is used in a number of performance 
critical codepaths such as tracing.

Why does this matter so much? A NULL string is not a big deal.

Note, since task->comm is 16 bytes there's the CMPXCHG16B instruction on x86 
which could be used to update it atomically, should atomicity really be 
desired.

Thanks,

	Ingo

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Jiri Slaby]

On 05/17/2011 10:47 PM, John Stultz wrote:
> Accessing task->comm requires proper locking. However in the past
> access to current->comm could be done without locking. This
> is no longer the case, so all comm access needs to be done
> while holding the comm_lock.
> 
> In my attempt to clean up unprotected comm access, I've noticed
> most comm access is done for printk output. To simplify correct
> locking in these cases, I've introduced a new %ptc format,
> which will print the corresponding task's comm.
> 
> Example use:
> printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> 
> CC: Joe Perches <joe@perches.com>
> CC: Michal Nazarewicz <mina86@mina86.com>
> CC: Andy Whitcroft <apw@canonical.com>
> CC: Jiri Slaby <jirislaby@gmail.com>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: David Rientjes <rientjes@google.com>
> CC: Dave Hansen <dave@linux.vnet.ibm.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: linux-mm@kvack.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  lib/vsprintf.c |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index bc0ac6b..b7a9953 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -797,6 +797,23 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
>  	return string(buf, end, uuid, spec);
>  }
>  
> +static noinline_for_stack

I still fail to see why this should be slowed down by noinlining it.
Care to explain?

With my setup, the code below inlined will use 32 bytes of stack. The
same as %pK case. Uninlined it obviously eats "only" 8 bytes for IP.

> +char *task_comm_string(char *buf, char *end, void *addr,
> +			 struct printf_spec spec, const char *fmt)
> +{
> +	struct task_struct *tsk = addr;
> +	char *ret;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tsk->comm_lock, flags);
> +	ret = string(buf, end, tsk->comm, spec);
> +	spin_unlock_irqrestore(&tsk->comm_lock, flags);
> +
> +	return ret;
> +}

thanks,
-- 
js

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Joe Perches]

On Tue, 2011-05-17 at 23:42 +0200, Jiri Slaby wrote:
> On 05/17/2011 10:47 PM, John Stultz wrote:
> > Accessing task->comm requires proper locking. However in the past
> > access to current->comm could be done without locking. This
> > is no longer the case, so all comm access needs to be done
> > while holding the comm_lock.
> > +static noinline_for_stack
> I still fail to see why this should be slowed down by noinlining it.
> Care to explain?

Any vsprintf is slow.

> With my setup, the code below inlined will use 32 bytes of stack. The
> same as %pK case. Uninlined it obviously eats "only" 8 bytes for IP.

The idea is to avoid excess stack consumption for things like:

	struct va_format vaf;

	const char *fmt = "some format with %ptc";

	vaf.fmt = fmt;
	vaf.va = &va_list;

	printk("some format with %pV\n", &vaf);

> > +char *task_comm_string(char *buf, char *end, void *addr,
> > +			 struct printf_spec spec, const char *fmt)
> > +{
> > +	struct task_struct *tsk = addr;
> > +	char *ret;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&tsk->comm_lock, flags);
> > +	ret = string(buf, end, tsk->comm, spec);
> > +	spin_unlock_irqrestore(&tsk->comm_lock, flags);
> > +
> > +	return ret;
> > +}

I think it was more of a problem when "4k stacks" was the default
than today, but I think it is still "good form". 

Re: [PATCH 1/3] comm: Introduce comm_lock spinlock to protect task->comm access

[Peter Zijlstra]

On Tue, 2011-05-17 at 23:27 +0200, Ingo Molnar wrote:
> * John Stultz <john.stultz@linaro.org> wrote:
> 
> > The implicit rules for current->comm access being safe without locking are no 
> > longer true. Accessing current->comm without holding the task lock may result 
> > in null or incomplete strings (however, access won't run off the end of the 
> > string).
> 
> This is rather unfortunate - task->comm is used in a number of performance 
> critical codepaths such as tracing.
> 
> Why does this matter so much? A NULL string is not a big deal.
> 
> Note, since task->comm is 16 bytes there's the CMPXCHG16B instruction on x86 
> which could be used to update it atomically, should atomicity really be 
> desired.

The changelog also fails to mention _WHY_ this is no longer true. Nor
does it treat why making it true again isn't an option.

Who is changing another task's comm? That's just silly.

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Jiri Slaby]

On 05/17/2011 11:52 PM, Joe Perches wrote:
> On Tue, 2011-05-17 at 23:42 +0200, Jiri Slaby wrote:
>> On 05/17/2011 10:47 PM, John Stultz wrote:
>>> Accessing task->comm requires proper locking. However in the past
>>> access to current->comm could be done without locking. This
>>> is no longer the case, so all comm access needs to be done
>>> while holding the comm_lock.
>>> +static noinline_for_stack
>> I still fail to see why this should be slowed down by noinlining it.
>> Care to explain?
> 
> Any vsprintf is slow.
> 
>> With my setup, the code below inlined will use 32 bytes of stack. The
>> same as %pK case. Uninlined it obviously eats "only" 8 bytes for IP.
> 
> The idea is to avoid excess stack consumption for things like:
> 
> 	struct va_format vaf;
> 
> 	const char *fmt = "some format with %ptc";
> 
> 	vaf.fmt = fmt;
> 	vaf.va = &va_list;
> 
> 	printk("some format with %pV\n", &vaf);

There is no way how can noinline_for_stack for task_comm_string lower
the stack usage here, right? Note that it adds no more requirements to
the stack than there were before. Simply because there are no buffers on
the stack in task_comm_string.

If nothing, it saves 100 bytes of .text.

thanks,
-- 
js

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Joe Perches]

On Wed, 2011-05-18 at 00:04 +0200, Jiri Slaby wrote:
> On 05/17/2011 11:52 PM, Joe Perches wrote:
> > On Tue, 2011-05-17 at 23:42 +0200, Jiri Slaby wrote:
> >> On 05/17/2011 10:47 PM, John Stultz wrote:
> >>> Accessing task->comm requires proper locking. However in the past
> >>> access to current->comm could be done without locking. This
> >>> is no longer the case, so all comm access needs to be done
> >>> while holding the comm_lock.
> >>> +static noinline_for_stack
> >> With my setup, the code below inlined will use 32 bytes of stack. The
> >> same as %pK case. Uninlined it obviously eats "only" 8 bytes for IP.
> > The idea is to avoid excess stack consumption for things like:
> > 	struct va_format vaf;
> > 	const char *fmt = "some format with %ptc";
> > 	vaf.fmt = fmt;
> > 	vaf.va = &va_list;
> > 	printk("some format with %pV\n", &vaf);
> There is no way how can noinline_for_stack for task_comm_string lower
> the stack usage here, right? Note that it adds no more requirements to
> the stack than there were before. Simply because there are no buffers on
> the stack in task_comm_string.

Isn't flags always on stack in function pointer
if task_comm_string were inlined?

I believe gcc isn't too good about reusing stack for blocks

void foo(args)
{
	if (bar) {
		long baz;
		...
	} else
		int baz;
		...
}

I believe gcc still creates 2 separate baz vars on 
foo's stack.

> If nothing, it saves 100 bytes of .text.

Submit patches to vsprintf for all the cases you think
appropriate.

> thanks,

cheers, Joe

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[John Stultz]

On Tue, 2011-05-17 at 23:42 +0200, Jiri Slaby wrote:
> On 05/17/2011 10:47 PM, John Stultz wrote:  
> > +static noinline_for_stack
> 
> I still fail to see why this should be slowed down by noinlining it.
> Care to explain?

Just that I was hesitant to change it without consensus and it follows
the convention of other similarly called functions.

> With my setup, the code below inlined will use 32 bytes of stack. The
> same as %pK case. Uninlined it obviously eats "only" 8 bytes for IP.

Maybe could we defer that discussion into a following patch, which maybe
does a similar analysis on the other noinline_for_stack usage in that
case?

(And I may be dropping the whole series here in a bit, so more debate on
it might be moot)

thanks
-john

Re: [PATCH 1/3] comm: Introduce comm_lock spinlock to protect task->comm access

[John Stultz]

On Tue, 2011-05-17 at 23:27 +0200, Ingo Molnar wrote:
> * John Stultz <john.stultz@linaro.org> wrote:
> 
> > The implicit rules for current->comm access being safe without locking are no 
> > longer true. Accessing current->comm without holding the task lock may result 
> > in null or incomplete strings (however, access won't run off the end of the 
> > string).
> 
> This is rather unfortunate - task->comm is used in a number of performance 
> critical codepaths such as tracing.
> 
> Why does this matter so much? A NULL string is not a big deal.

I'll defer to KOSAKI Motohiro and David on this bit. :)

> Note, since task->comm is 16 bytes there's the CMPXCHG16B instruction on x86 
> which could be used to update it atomically, should atomicity really be 
> desired.

Could we use this where cmpxchg16b is available and fall back to locking
if not? Or does that put too much of a penalty on arches that don't have
cmpxchg16b support?

Alternatively, we can have locked accessors that are safe in the
majority of slow-path warning printks, and provide unlocked accessors
for cases where the performance is critical and the code can properly
handle possibly incomplete comms.

thanks
-john

Re: [PATCH 1/3] comm: Introduce comm_lock spinlock to protect task->comm access

[David Rientjes]

On Tue, 17 May 2011, Peter Zijlstra wrote:

> The changelog also fails to mention _WHY_ this is no longer true. Nor
> does it treat why making it true again isn't an option.
> 

It's been true since:

	4614a696bd1c3a9af3a08f0e5874830a85b889d4
	Author: john stultz <johnstul@us.ibm.com>
	Date:   Mon Dec 14 18:00:05 2009 -0800

	    procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm

Although at the time it appears that nobody was concerned about races so 
proper syncronization was never implemented.  We always had the 
prctl(PR_SET_NAME) so the majority of comm reads, those to current, 
required no locking, but this commit changed that.  The remainder of comm 
dereferences always required task_lock() and the helper get_task_comm() to 
read the string into a (usually stack-allocated) buffer.

> Who is changing another task's comm? That's just silly.
> 

I agree, and I suggested taking write privileges away from /proc/pid/comm, 
but others find that it is useful to be able to differentiate between 
threads in the same thread group without using the prctl() for debugging?

Re: [PATCH 1/3] comm: Introduce comm_lock spinlock to protect task->comm access

[KOSAKI Motohiro]

(2011/05/18 6:54), Peter Zijlstra wrote:
> On Tue, 2011-05-17 at 23:27 +0200, Ingo Molnar wrote:
>> * John Stultz<john.stultz@linaro.org>  wrote:
>>
>>> The implicit rules for current->comm access being safe without locking are no
>>> longer true. Accessing current->comm without holding the task lock may result
>>> in null or incomplete strings (however, access won't run off the end of the
>>> string).
>>
>> This is rather unfortunate - task->comm is used in a number of performance
>> critical codepaths such as tracing.
>>
>> Why does this matter so much? A NULL string is not a big deal.
>>
>> Note, since task->comm is 16 bytes there's the CMPXCHG16B instruction on x86
>> which could be used to update it atomically, should atomicity really be
>> desired.
>
> The changelog also fails to mention _WHY_ this is no longer true. Nor
> does it treat why making it true again isn't an option.
>
> Who is changing another task's comm? That's just silly.

I'm not sure it's silly or not. But the fact is, comm override was introduced
following patch. Personally I'd like to mark it to "depend on EXPERT". but John
seems to dislike the idea.



commit 4614a696bd1c3a9af3a08f0e5874830a85b889d4
Author: john stultz <johnstul@us.ibm.com>
Date:   Mon Dec 14 18:00:05 2009 -0800

     procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm

Re: [PATCH 1/3] comm: Introduce comm_lock spinlock to protect task->comm access

[KOSAKI Motohiro]

(2011/05/18 7:27), John Stultz wrote:
> On Tue, 2011-05-17 at 23:27 +0200, Ingo Molnar wrote:
>> * John Stultz<john.stultz@linaro.org>  wrote:
>>
>>> The implicit rules for current->comm access being safe without locking are no
>>> longer true. Accessing current->comm without holding the task lock may result
>>> in null or incomplete strings (however, access won't run off the end of the
>>> string).
>>
>> This is rather unfortunate - task->comm is used in a number of performance
>> critical codepaths such as tracing.

Right.


>> Why does this matter so much? A NULL string is not a big deal.
>
> I'll defer to KOSAKI Motohiro and David on this bit. :)

Heh, I did ask you current locking rule of task->comm after you introduced
writable /proc/<pid>/comm.


>> Note, since task->comm is 16 bytes there's the CMPXCHG16B instruction on x86
>> which could be used to update it atomically, should atomicity really be
>> desired.
>
> Could we use this where cmpxchg16b is available and fall back to locking
> if not? Or does that put too much of a penalty on arches that don't have
> cmpxchg16b support?
>
> Alternatively, we can have locked accessors that are safe in the
> majority of slow-path warning printks, and provide unlocked accessors
> for cases where the performance is critical and the code can properly
> handle possibly incomplete comms.

Probably, this is safer choice.

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[KOSAKI Motohiro]

(2011/05/17 6:19), John Stultz wrote:
> Accessing task->comm requires proper locking. However in the past
> access to current->comm could be done without locking. This
> is no longer the case, so all comm access needs to be done
> while holding the comm_lock.
> 
> In my attempt to clean up unprotected comm access, I've noticed
> most comm access is done for printk output. To simplify correct
> locking in these cases, I've introduced a new %ptc format,
> which will print the corresponding task's comm.
> 
> Example use:
> printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> 
> CC: Ted Ts'o<tytso@mit.edu>
> CC: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> CC: David Rientjes<rientjes@google.com>
> CC: Dave Hansen<dave@linux.vnet.ibm.com>
> CC: Andrew Morton<akpm@linux-foundation.org>
> CC: linux-mm@kvack.org
> Signed-off-by: John Stultz<john.stultz@linaro.org>

Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Jiri Slaby]

On 05/17/2011 01:56 AM, Joe Perches wrote:
> On Mon, 2011-05-16 at 16:10 -0700, John Stultz wrote:
>> On Mon, 2011-05-16 at 23:54 +0200, Jiri Slaby wrote:
>>>> In my attempt to clean up unprotected comm access, I've noticed
>>>> most comm access is done for printk output. To simplify correct
>>>> locking in these cases, I've introduced a new %ptc format,
>>>> which will print the corresponding task's comm.
>>>> Example use:
>>>> printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
>>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
>>>> +static noinline_for_stack
>>> Actually, why noinline? Did your previous version have there some
>>> TASK_COMM_LEN buffer or anything on stack which is not there anymore?
>> No, I was just following how almost all of the pointer() called
>> functions were declared.
>> But with two pointers and a long, I add more then ip6_string() has on
>> the stack, which uses the same notation.
>> But I can drop that bit if there's really no need for it.
> 
> vsprintf can be recursive, I think you should keep it.

Why? pointer is marked as noinline. The others in pointer are marked as
noinline because they really have buffers on stack. There is no reason
to have noinline for task_comm_string though. I guess all tsk, ret and
flags will be optimized that way so they will be in registers, not on
stack at all.

thanks,
-- 
js

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[John Stultz]

On Mon, 2011-05-16 at 16:56 -0700, Joe Perches wrote:
> On Mon, 2011-05-16 at 16:10 -0700, John Stultz wrote:
> > On Mon, 2011-05-16 at 23:54 +0200, Jiri Slaby wrote:
> > > > In my attempt to clean up unprotected comm access, I've noticed
> > > > most comm access is done for printk output. To simplify correct
> > > > locking in these cases, I've introduced a new %ptc format,
> > > > which will print the corresponding task's comm.
> > > > Example use:
> > > > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> > > > +static noinline_for_stack
> > > Actually, why noinline? Did your previous version have there some
> > > TASK_COMM_LEN buffer or anything on stack which is not there anymore?
> > No, I was just following how almost all of the pointer() called
> > functions were declared.
> > But with two pointers and a long, I add more then ip6_string() has on
> > the stack, which uses the same notation.
> > But I can drop that bit if there's really no need for it.
> 
> vsprintf can be recursive, I think you should keep it.

Ok. I'll keep it then. Thanks!
-john

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Joe Perches]

On Mon, 2011-05-16 at 16:10 -0700, John Stultz wrote:
> On Mon, 2011-05-16 at 23:54 +0200, Jiri Slaby wrote:
> > > In my attempt to clean up unprotected comm access, I've noticed
> > > most comm access is done for printk output. To simplify correct
> > > locking in these cases, I've introduced a new %ptc format,
> > > which will print the corresponding task's comm.
> > > Example use:
> > > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> > > +static noinline_for_stack
> > Actually, why noinline? Did your previous version have there some
> > TASK_COMM_LEN buffer or anything on stack which is not there anymore?
> No, I was just following how almost all of the pointer() called
> functions were declared.
> But with two pointers and a long, I add more then ip6_string() has on
> the stack, which uses the same notation.
> But I can drop that bit if there's really no need for it.

vsprintf can be recursive, I think you should keep it.

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[John Stultz]

On Mon, 2011-05-16 at 23:54 +0200, Jiri Slaby wrote:
> On 05/16/2011 11:19 PM, John Stultz wrote:
> > Accessing task->comm requires proper locking. However in the past
> > access to current->comm could be done without locking. This
> > is no longer the case, so all comm access needs to be done
> > while holding the comm_lock.
> > 
> > In my attempt to clean up unprotected comm access, I've noticed
> > most comm access is done for printk output. To simplify correct
> > locking in these cases, I've introduced a new %ptc format,
> > which will print the corresponding task's comm.
> > 
> > Example use:
> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> > 
> > CC: Ted Ts'o <tytso@mit.edu>
> > CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > CC: David Rientjes <rientjes@google.com>
> > CC: Dave Hansen <dave@linux.vnet.ibm.com>
> > CC: Andrew Morton <akpm@linux-foundation.org>
> > CC: linux-mm@kvack.org
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> >  lib/vsprintf.c |   24 ++++++++++++++++++++++++
> >  1 files changed, 24 insertions(+), 0 deletions(-)
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index bc0ac6b..b7a9953 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -797,6 +797,23 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
> >  	return string(buf, end, uuid, spec);
> >  }
> >  
> > +static noinline_for_stack
> 
> Actually, why noinline? Did your previous version have there some
> TASK_COMM_LEN buffer or anything on stack which is not there anymore?

No, I was just following how almost all of the pointer() called
functions were declared.

But with two pointers and a long, I add more then ip6_string() has on
the stack, which uses the same notation.

But I can drop that bit if there's really no need for it.

thanks
-john

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Jiri Slaby]

On 05/16/2011 11:19 PM, John Stultz wrote:
> Accessing task->comm requires proper locking. However in the past
> access to current->comm could be done without locking. This
> is no longer the case, so all comm access needs to be done
> while holding the comm_lock.
> 
> In my attempt to clean up unprotected comm access, I've noticed
> most comm access is done for printk output. To simplify correct
> locking in these cases, I've introduced a new %ptc format,
> which will print the corresponding task's comm.
> 
> Example use:
> printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> 
> CC: Ted Ts'o <tytso@mit.edu>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: David Rientjes <rientjes@google.com>
> CC: Dave Hansen <dave@linux.vnet.ibm.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: linux-mm@kvack.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  lib/vsprintf.c |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index bc0ac6b..b7a9953 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -797,6 +797,23 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
>  	return string(buf, end, uuid, spec);
>  }
>  
> +static noinline_for_stack

Actually, why noinline? Did your previous version have there some
TASK_COMM_LEN buffer or anything on stack which is not there anymore?

> +char *task_comm_string(char *buf, char *end, void *addr,
> +			 struct printf_spec spec, const char *fmt)
> +{
> +	struct task_struct *tsk = addr;
> +	char *ret;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tsk->comm_lock, flags);
> +	ret = string(buf, end, tsk->comm, spec);
> +	spin_unlock_irqrestore(&tsk->comm_lock, flags);
> +
> +	return ret;
> +}
> +
> +
> +
>  int kptr_restrict = 1;
>  
>  /*

thanks,
-- 
js

[PATCH 2/3] printk: Add %ptc to safely print a task's comm

[John Stultz]

Accessing task->comm requires proper locking. However in the past
access to current->comm could be done without locking. This
is no longer the case, so all comm access needs to be done
while holding the comm_lock.

In my attempt to clean up unprotected comm access, I've noticed
most comm access is done for printk output. To simplify correct
locking in these cases, I've introduced a new %ptc format,
which will print the corresponding task's comm.

Example use:
printk("%ptc: unaligned epc - sending SIGBUS.\n", current);

CC: Ted Ts'o <tytso@mit.edu>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: David Rientjes <rientjes@google.com>
CC: Dave Hansen <dave@linux.vnet.ibm.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: linux-mm@kvack.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 lib/vsprintf.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index bc0ac6b..b7a9953 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -797,6 +797,23 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, uuid, spec);
 }
 
+static noinline_for_stack
+char *task_comm_string(char *buf, char *end, void *addr,
+			 struct printf_spec spec, const char *fmt)
+{
+	struct task_struct *tsk = addr;
+	char *ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tsk->comm_lock, flags);
+	ret = string(buf, end, tsk->comm, spec);
+	spin_unlock_irqrestore(&tsk->comm_lock, flags);
+
+	return ret;
+}
+
+
+
 int kptr_restrict = 1;
 
 /*
@@ -864,6 +881,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	}
 
 	switch (*fmt) {
+	case 't':
+		switch (fmt[1]) {
+		case 'c':
+			return task_comm_string(buf, end, ptr, spec, fmt);
+		}
+		break;
 	case 'F':
 	case 'f':
 		ptr = dereference_function_descriptor(ptr);
@@ -1151,6 +1174,7 @@ qualifier:
  *   http://tools.ietf.org/html/draft-ietf-6man-text-addr-representation-00
  * %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
  *   case.
+ * %ptc outputs the task's comm name
  * %n is ignored
  *
  * The return value is the number of characters which would
-- 
1.7.3.2.146.gca209

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[David Rientjes]

On Thu, 12 May 2011, Joe Perches wrote:

> > > > Although I'm not sure if there's precedent for a %p value that didn't
> > > > take a argument. Thoughts on that? Anyone else have an opinion here?
> > > The uses of %ptc must add an argument or else gcc will complain.
> > > I suggest you just ignore the argument value and use current.
> > That doesn't make any sense, why would you needlessly restrict this to 
> > current when accesses to other threads' ->comm needs to be protected in 
> > the same way?  I'd like to use this in the oom killer and try to get rid 
> > of taking task_lock() for every thread group leader in the tasklist dump.
> 
> I suppose another view is coder stuffed up, let them suffer...
> 
> At some point, gcc may let us extend printf argument type
> verification so it may not be a continuing problem.
> 

I don't understand your respose, could you answer my question?  Printing 
the command of threads other than current isn't special.

[PATCH 2/3] printk: Add %ptc to safely print a task's comm

[John Stultz]

Accessing task->comm requires proper locking. However in the past
access to current->comm could be done without locking. This
is no longer the case, so all comm access needs to be done
while holding the comm_lock.

In my attempt to clean up unprotected comm access, I've noticed
most comm access is done for printk output. To simplify correct
locking in these cases, I've introduced a new %ptc format,
which will print the corresponding task's comm.

Example use:
printk("%ptc: unaligned epc - sending SIGBUS.\n", current);

CC: Ted Ts'o <tytso@mit.edu>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: David Rientjes <rientjes@google.com>
CC: Dave Hansen <dave@linux.vnet.ibm.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: linux-mm@kvack.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 lib/vsprintf.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index bc0ac6b..5abbe94 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -797,6 +797,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, uuid, spec);
 }
 
+static noinline_for_stack
+char *task_comm_string(char *buf, char *end, void *addr,
+			 struct printf_spec spec, const char *fmt)
+{
+	struct task_struct *tsk = addr;
+	char *ret;
+	unsigned long seq;
+
+	do {
+		seq = read_seqbegin(&tsk->comm_lock);
+
+		ret = string(buf, end, tsk->comm, spec);
+
+	} while (read_seqretry(&tsk->comm_lock, seq));
+
+	return ret;
+}
+
+
+
 int kptr_restrict = 1;
 
 /*
@@ -864,6 +884,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	}
 
 	switch (*fmt) {
+	case 't':
+		switch (fmt[1]) {
+		case 'c':
+			return task_comm_string(buf, end, ptr, spec, fmt);
+		}
+		break;
 	case 'F':
 	case 'f':
 		ptr = dereference_function_descriptor(ptr);
@@ -1151,6 +1177,7 @@ qualifier:
  *   http://tools.ietf.org/html/draft-ietf-6man-text-addr-representation-00
  * %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
  *   case.
+ * %ptc outputs the task's comm name
  * %n is ignored
  *
  * The return value is the number of characters which would
-- 
1.7.3.2.146.gca209

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Joe Perches]

On Thu, 2011-05-12 at 15:12 -0700, David Rientjes wrote:
> On Tue, 10 May 2011, Joe Perches wrote:
> > > Although I'm not sure if there's precedent for a %p value that didn't
> > > take a argument. Thoughts on that? Anyone else have an opinion here?
> > The uses of %ptc must add an argument or else gcc will complain.
> > I suggest you just ignore the argument value and use current.
> That doesn't make any sense, why would you needlessly restrict this to 
> current when accesses to other threads' ->comm needs to be protected in 
> the same way?  I'd like to use this in the oom killer and try to get rid 
> of taking task_lock() for every thread group leader in the tasklist dump.

I suppose another view is coder stuffed up, let them suffer...

At some point, gcc may let us extend printf argument type
verification so it may not be a continuing problem.

Adding a checkpatch rule for this is non-trivial as it can
be written as:

	printk("%ptc\n",
	       current);

and checkpatch is mostly line oriented.

Andy, do you have a suggestion on how to verify
vsprintf argument types for checkpatch?

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[David Rientjes]

On Tue, 10 May 2011, Joe Perches wrote:

> > Although I'm not sure if there's precedent for a %p value that didn't
> > take a argument. Thoughts on that? Anyone else have an opinion here?
> 
> The uses of %ptc must add an argument or else gcc will complain.
> I suggest you just ignore the argument value and use current.
> 

That doesn't make any sense, why would you needlessly restrict this to 
current when accesses to other threads' ->comm needs to be protected in 
the same way?  I'd like to use this in the oom killer and try to get rid 
of taking task_lock() for every thread group leader in the tasklist dump.

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[David Rientjes]

On Tue, 10 May 2011, John Stultz wrote:

> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > index bc0ac6b..b9c97b8 100644
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -797,6 +797,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
> > >  	return string(buf, end, uuid, spec);
> > >  }
> > >  
> > > +static noinline_for_stack
> > > +char *task_comm_string(char *buf, char *end, u8 *addr,
> > > +			 struct printf_spec spec, const char *fmt)
> > 
> > addr should be void * not u8 *
> > 
> > > +{
> > > +	struct task_struct *tsk = (struct task_struct *) addr;
> > 
> > no cast.
> > 
> > Maybe it'd be better to use current inside this routine and not
> > pass the pointer at all.
> 
> That sounds reasonable. Most users are current, so forcing the more rare
> non-current users to copy it to a buffer first and use the normal %s
> would not be of much impact.
> 

Please still require an argument, otherwise the oom killer (which could 
potentially called right before a stack overflow) would be required to use 
buffers for the commands printed in the tasklist dump.

> Although I'm not sure if there's precedent for a %p value that didn't
> take a argument. Thoughts on that? Anyone else have an opinion here?
> 

After the cleanups are addressed:

	Acked-by: David Rientjes <rientjes@google.com>

It would have been nice if we could force %ptc to expect a 
struct task_struct * rather than a void *, however.

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[John Stultz]

On Thu, 2011-05-12 at 18:43 +0800, Américo Wang wrote:
> On Thu, May 12, 2011 at 5:02 AM, John Stultz <john.stultz@linaro.org> wrote:
> > On Wed, 2011-05-11 at 17:33 +0800, Américo Wang wrote:
> >> On Wed, May 11, 2011 at 8:23 AM, John Stultz <john.stultz@linaro.org> wrote:
> >> > Acessing task->comm requires proper locking. However in the past
> >> > access to current->comm could be done without locking. This
> >> > is no longer the case, so all comm access needs to be done
> >> > while holding the comm_lock.
> >> >
> >> > In my attempt to clean up unprotected comm access, I've noticed
> >> > most comm access is done for printk output. To simpify correct
> >> > locking in these cases, I've introduced a new %ptc format,
> >> > which will safely print the corresponding task's comm.
> >> >
> >> > Example use:
> >> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> >> >
> >>
> >> Why do you hide current->comm behide printk?
> >> How is this better than printk("%s: ....", task_comm(current)) ?
> >
> > So to properly access current->comm, you need to hold the task-lock (or
> > with my new patch set, the comm_lock). Rather then adding locking to all
> > the call sites that printk("%s ...", current->comm), I'm suggesting we
> > add a new %ptc method which will handle the locking for you.
> >
> 
> Sorry, I meant why not adding the locking into a wrapper function,
> probably get_task_comm() and let the users to call it directly?
> 
> Why is %ptc better than
> 
> char comm[...];
> get_task_comm(comm, current);
> printk("%s: ....", comm);

There were concerns about the extra stack usage caused adding a comm
buffer to each location, which can be avoided by adding the
functionality to printk.

Further it reduces the amount of change necessary to correct invalid
usage.

thanks
-john

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Américo Wang]

On Thu, May 12, 2011 at 6:43 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, May 12, 2011 at 5:02 AM, John Stultz <john.stultz@linaro.org> wrote:
>> On Wed, 2011-05-11 at 17:33 +0800, Américo Wang wrote:
>>> On Wed, May 11, 2011 at 8:23 AM, John Stultz <john.stultz@linaro.org> wrote:
>>> > Acessing task->comm requires proper locking. However in the past
>>> > access to current->comm could be done without locking. This
>>> > is no longer the case, so all comm access needs to be done
>>> > while holding the comm_lock.
>>> >
>>> > In my attempt to clean up unprotected comm access, I've noticed
>>> > most comm access is done for printk output. To simpify correct
>>> > locking in these cases, I've introduced a new %ptc format,
>>> > which will safely print the corresponding task's comm.
>>> >
>>> > Example use:
>>> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
>>> >
>>>
>>> Why do you hide current->comm behide printk?
>>> How is this better than printk("%s: ....", task_comm(current)) ?
>>
>> So to properly access current->comm, you need to hold the task-lock (or
>> with my new patch set, the comm_lock). Rather then adding locking to all
>> the call sites that printk("%s ...", current->comm), I'm suggesting we
>> add a new %ptc method which will handle the locking for you.
>>
>
> Sorry, I meant why not adding the locking into a wrapper function,
> probably get_task_comm() and let the users to call it directly?
>

Ahhh, never mind, I see the points now... Then it is fine. :)

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Américo Wang]

On Thu, May 12, 2011 at 5:02 AM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, 2011-05-11 at 17:33 +0800, Américo Wang wrote:
>> On Wed, May 11, 2011 at 8:23 AM, John Stultz <john.stultz@linaro.org> wrote:
>> > Acessing task->comm requires proper locking. However in the past
>> > access to current->comm could be done without locking. This
>> > is no longer the case, so all comm access needs to be done
>> > while holding the comm_lock.
>> >
>> > In my attempt to clean up unprotected comm access, I've noticed
>> > most comm access is done for printk output. To simpify correct
>> > locking in these cases, I've introduced a new %ptc format,
>> > which will safely print the corresponding task's comm.
>> >
>> > Example use:
>> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
>> >
>>
>> Why do you hide current->comm behide printk?
>> How is this better than printk("%s: ....", task_comm(current)) ?
>
> So to properly access current->comm, you need to hold the task-lock (or
> with my new patch set, the comm_lock). Rather then adding locking to all
> the call sites that printk("%s ...", current->comm), I'm suggesting we
> add a new %ptc method which will handle the locking for you.
>

Sorry, I meant why not adding the locking into a wrapper function,
probably get_task_comm() and let the users to call it directly?

Why is %ptc better than

char comm[...];
get_task_comm(comm, current);
printk("%s: ....", comm);

?

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[John Stultz]

On Wed, 2011-05-11 at 10:36 -0700, Andi Kleen wrote:
> John Stultz <john.stultz@linaro.org> writes:
> 
> > Acessing task->comm requires proper locking. However in the past
> > access to current->comm could be done without locking. This
> > is no longer the case, so all comm access needs to be done
> > while holding the comm_lock.
> >
> > In my attempt to clean up unprotected comm access, I've noticed
> > most comm access is done for printk output. To simpify correct
> > locking in these cases, I've introduced a new %ptc format,
> > which will safely print the corresponding task's comm.
> >
> > Example use:
> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> 
> Neat. But you probably want a checkpatch rule for this too
> to catch new offenders.

Yea. That's on my queue.

thanks
-john

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[John Stultz]

On Wed, 2011-05-11 at 17:33 +0800, Américo Wang wrote:
> On Wed, May 11, 2011 at 8:23 AM, John Stultz <john.stultz@linaro.org> wrote:
> > Acessing task->comm requires proper locking. However in the past
> > access to current->comm could be done without locking. This
> > is no longer the case, so all comm access needs to be done
> > while holding the comm_lock.
> >
> > In my attempt to clean up unprotected comm access, I've noticed
> > most comm access is done for printk output. To simpify correct
> > locking in these cases, I've introduced a new %ptc format,
> > which will safely print the corresponding task's comm.
> >
> > Example use:
> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> >
> 
> Why do you hide current->comm behide printk?
> How is this better than printk("%s: ....", task_comm(current)) ?

So to properly access current->comm, you need to hold the task-lock (or
with my new patch set, the comm_lock). Rather then adding locking to all
the call sites that printk("%s ...", current->comm), I'm suggesting we
add a new %ptc method which will handle the locking for you.

thanks
-john

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Andi Kleen]

John Stultz <john.stultz@linaro.org> writes:

> Acessing task->comm requires proper locking. However in the past
> access to current->comm could be done without locking. This
> is no longer the case, so all comm access needs to be done
> while holding the comm_lock.
>
> In my attempt to clean up unprotected comm access, I've noticed
> most comm access is done for printk output. To simpify correct
> locking in these cases, I've introduced a new %ptc format,
> which will safely print the corresponding task's comm.
>
> Example use:
> printk("%ptc: unaligned epc - sending SIGBUS.\n", current);

Neat. But you probably want a checkpatch rule for this too
to catch new offenders.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Américo Wang]

On Wed, May 11, 2011 at 8:23 AM, John Stultz <john.stultz@linaro.org> wrote:
> Acessing task->comm requires proper locking. However in the past
> access to current->comm could be done without locking. This
> is no longer the case, so all comm access needs to be done
> while holding the comm_lock.
>
> In my attempt to clean up unprotected comm access, I've noticed
> most comm access is done for printk output. To simpify correct
> locking in these cases, I've introduced a new %ptc format,
> which will safely print the corresponding task's comm.
>
> Example use:
> printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
>

Why do you hide current->comm behide printk?
How is this better than printk("%s: ....", task_comm(current)) ?

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Joe Perches]

On Tue, 2011-05-10 at 18:10 -0700, John Stultz wrote:
> On Tue, 2011-05-10 at 17:51 -0700, Joe Perches wrote:
> > On Tue, 2011-05-10 at 17:23 -0700, John Stultz wrote:
> > > Acessing task->comm requires proper locking. However in the past
> > > access to current->comm could be done without locking. This
> > > is no longer the case, so all comm access needs to be done
> > > while holding the comm_lock.
> > Could misuse of %ptc (not using current) cause system lockup?
> It very well could. Although I don't see other %p options tring to
> handle invalid pointers. Any suggestions on how to best handle this?

The only one I know of is ipv6 which copies a 16 byte buffer
in case the pointed to value is unaligned.  I suppose %pI6c
could be a problem or maybe %pS too, but it hasn't been in
practice.  The use of %ptc somehow seemed more error prone.

> Most users are current, so forcing the more rare
> non-current users to copy it to a buffer first and use the normal %s
> would not be of much impact.
> 
> Although I'm not sure if there's precedent for a %p value that didn't
> take a argument. Thoughts on that? Anyone else have an opinion here?

The uses of %ptc must add an argument or else gcc will complain.
I suggest you just ignore the argument value and use current.

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[john stultz]

On Tue, 2011-05-10 at 18:10 -0700, John Stultz wrote:
> On Tue, 2011-05-10 at 17:51 -0700, Joe Perches wrote:
> > Could misuse of %ptc (not using current) cause system lockup?
> 
> It very well could. Although I don't see other %p options tring to
> handle invalid pointers. Any suggestions on how to best handle this?

And just to clarify on this point, I'm responding to if a invalid
pointer was provided, causing the dereference to go awry.

If a valid non-current task was provided, the locking should be ok as we
disable irqs while the write_seqlock is held in set_task_comm().

The only places this could cause a problem was if you tried to printk
with a %ptc while holding the task->comm_lock. However, the lock is only
shortly held in task_comm_string, and get_task_comm and set_task_comm.
So it is fairly easy to audit for correctness.

If there is some other situation you had in mind, please let me know.

thanks
-john

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[John Stultz]

On Tue, 2011-05-10 at 17:51 -0700, Joe Perches wrote:
> On Tue, 2011-05-10 at 17:23 -0700, John Stultz wrote:
> > Acessing task->comm requires proper locking. However in the past
> > access to current->comm could be done without locking. This
> > is no longer the case, so all comm access needs to be done
> > while holding the comm_lock.
> > 
> > In my attempt to clean up unprotected comm access, I've noticed
> > most comm access is done for printk output. To simpify correct
> > locking in these cases, I've introduced a new %ptc format,
> > which will safely print the corresponding task's comm.
> 
> Hi John.
> 
> Couple of tyops for Accessing and simplify in your commit message
> and a few comments on the patch.

Ah. Yes. Thanks!

> Could misuse of %ptc (not using current) cause system lockup?

It very well could. Although I don't see other %p options tring to
handle invalid pointers. Any suggestions on how to best handle this?


> > Example use:
> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> 
> 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index bc0ac6b..b9c97b8 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -797,6 +797,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
> >  	return string(buf, end, uuid, spec);
> >  }
> >  
> > +static noinline_for_stack
> > +char *task_comm_string(char *buf, char *end, u8 *addr,
> > +			 struct printf_spec spec, const char *fmt)
> 
> addr should be void * not u8 *
> 
> > +{
> > +	struct task_struct *tsk = (struct task_struct *) addr;
> 
> no cast.
> 
> Maybe it'd be better to use current inside this routine and not
> pass the pointer at all.

That sounds reasonable. Most users are current, so forcing the more rare
non-current users to copy it to a buffer first and use the normal %s
would not be of much impact.

Although I'm not sure if there's precedent for a %p value that didn't
take a argument. Thoughts on that? Anyone else have an opinion here?

Thanks so much for the review and feedback!
-john

Re: [PATCH 2/3] printk: Add %ptc to safely print a task's comm

[Joe Perches]

On Tue, 2011-05-10 at 17:23 -0700, John Stultz wrote:
> Acessing task->comm requires proper locking. However in the past
> access to current->comm could be done without locking. This
> is no longer the case, so all comm access needs to be done
> while holding the comm_lock.
> 
> In my attempt to clean up unprotected comm access, I've noticed
> most comm access is done for printk output. To simpify correct
> locking in these cases, I've introduced a new %ptc format,
> which will safely print the corresponding task's comm.

Hi John.

Couple of tyops for Accessing and simplify in your commit message
and a few comments on the patch.

Could misuse of %ptc (not using current) cause system lockup?

> Example use:
> printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
 

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index bc0ac6b..b9c97b8 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -797,6 +797,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
>  	return string(buf, end, uuid, spec);
>  }
>  
> +static noinline_for_stack
> +char *task_comm_string(char *buf, char *end, u8 *addr,
> +			 struct printf_spec spec, const char *fmt)

addr should be void * not u8 *

> +{
> +	struct task_struct *tsk = (struct task_struct *) addr;

no cast.

Maybe it'd be better to use current inside this routine and not
pass the pointer at all.

static noinline_for_stack
char *task_comm_string(char *buf, char *end,
		       struct printf_spec spec, const char *fmt)

> +	char *ret;
> +	unsigned long seq;
> +
> +	do {
> +		seq = read_seqbegin(&tsk->comm_lock);
> +
> +		ret = string(buf, end, tsk->comm, spec);
> +
> +	} while (read_seqretry(&tsk->comm_lock, seq));


> @@ -864,6 +884,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  	}
>  
>  	switch (*fmt) {
> +	case 't':
> +		switch (fmt[1]) {
> +		case 'c':
> +			return task_comm_string(buf, end, ptr, spec, fmt);

maybe
			return task_comm_string(buf, end, spec, fmt);

[PATCH 2/3] printk: Add %ptc to safely print a task's comm

[John Stultz]

Acessing task->comm requires proper locking. However in the past
access to current->comm could be done without locking. This
is no longer the case, so all comm access needs to be done
while holding the comm_lock.

In my attempt to clean up unprotected comm access, I've noticed
most comm access is done for printk output. To simpify correct
locking in these cases, I've introduced a new %ptc format,
which will safely print the corresponding task's comm.

Example use:
printk("%ptc: unaligned epc - sending SIGBUS.\n", current);

CC: Ted Ts'o <tytso@mit.edu>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: David Rientjes <rientjes@google.com>
CC: Dave Hansen <dave@linux.vnet.ibm.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: linux-mm@kvack.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 lib/vsprintf.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index bc0ac6b..b9c97b8 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -797,6 +797,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
 	return string(buf, end, uuid, spec);
 }
 
+static noinline_for_stack
+char *task_comm_string(char *buf, char *end, u8 *addr,
+			 struct printf_spec spec, const char *fmt)
+{
+	struct task_struct *tsk = (struct task_struct *) addr;
+	char *ret;
+	unsigned long seq;
+
+	do {
+		seq = read_seqbegin(&tsk->comm_lock);
+
+		ret = string(buf, end, tsk->comm, spec);
+
+	} while (read_seqretry(&tsk->comm_lock, seq));
+
+	return ret;
+}
+
+
+
 int kptr_restrict = 1;
 
 /*
@@ -864,6 +884,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	}
 
 	switch (*fmt) {
+	case 't':
+		switch (fmt[1]) {
+		case 'c':
+			return task_comm_string(buf, end, ptr, spec, fmt);
+		}
+		break;
 	case 'F':
 	case 'f':
 		ptr = dereference_function_descriptor(ptr);
@@ -1151,6 +1177,7 @@ qualifier:
  *   http://tools.ietf.org/html/draft-ietf-6man-text-addr-representation-00
  * %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
  *   case.
+ * %ptc outputs the task's comm name
  * %n is ignored
  *
  * The return value is the number of characters which would
-- 
1.7.3.2.146.gca209