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

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

[John Stultz]

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: Improved checkpatch regex from Jiri Slaby and
Michal Nazarewicz. Also replaced the seqlock with a spinlock to
address the possible starvation case brought up by KOSAKI Motohiro.

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: Ted Ts'o <tytso@mit.edu>
CC: Michal Nazarewicz <mina86@mina86.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 seqlock 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 ++---
 lib/vsprintf.c            |   24 ++++++++++++++++++++++++
 scripts/checkpatch.pl     |    4 ++++
 5 files changed, 47 insertions(+), 6 deletions(-)

-- 
1.7.3.2.146.gca209

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

[John Stultz]

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: Improved checkpatch regex from Jiri Slaby and
Michal Nazarewicz. Also replaced the seqlock with a spinlock to
address the possible starvation case brought up by KOSAKI Motohiro.

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: Ted Ts'o <tytso@mit.edu>
CC: Michal Nazarewicz <mina86@mina86.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 seqlock 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 ++---
 lib/vsprintf.c            |   24 ++++++++++++++++++++++++
 scripts/checkpatch.pl     |    4 ++++
 5 files changed, 47 insertions(+), 6 deletions(-)

-- 
1.7.3.2.146.gca209

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/3] comm: Introduce comm_lock seqlock 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: 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
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 ++---
 3 files changed, 19 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;
-- 
1.7.3.2.146.gca209

[PATCH 1/3] comm: Introduce comm_lock seqlock 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: 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
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 ++---
 3 files changed, 19 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;
-- 
1.7.3.2.146.gca209

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

[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

[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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

[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 and Michal Nazarewicz for help improving
the regex!

Close review and feedback would be appreciated.

CC: Ted Ts'o <tytso@mit.edu>
CC: Michal Nazarewicz <mina86@mina86.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 |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d867081..3a713c2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2868,6 +2868,10 @@ 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
+		if ($line =~ /\b(?:current|task|tsk|t)\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

[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 and Michal Nazarewicz for help improving
the regex!

Close review and feedback would be appreciated.

CC: Ted Ts'o <tytso@mit.edu>
CC: Michal Nazarewicz <mina86@mina86.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 |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d867081..3a713c2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2868,6 +2868,10 @@ 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
+		if ($line =~ /\b(?:current|task|tsk|t)\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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[Michal Nazarewicz]

On Mon, 16 May 2011 23:19:17 +0200, John Stultz wrote:
> Now that accessing current->comm needs to be protected,
> @@ -2868,6 +2868,10 @@ 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
> +		if ($line =~ /\b(?:current|task|tsk|t)\s*->\s*comm\b/) {

Not a checkpatch.pl expert but as far as I'm concerned, that looks  
reasonable.

I was sort of worried that t->comm could produce quite a few false  
positives
but all its appearances in the kernel (seem to) refer to task.

> +			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) {


-- 
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 3/3] checkpatch.pl: Add check for task comm references

[Michal Nazarewicz]

On Mon, 16 May 2011 23:19:17 +0200, John Stultz wrote:
> Now that accessing current->comm needs to be protected,
> @@ -2868,6 +2868,10 @@ 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
> +		if ($line =~ /\b(?:current|task|tsk|t)\s*->\s*comm\b/) {

Not a checkpatch.pl expert but as far as I'm concerned, that looks  
reasonable.

I was sort of worried that t->comm could produce quite a few false  
positives
but all its appearances in the kernel (seem to) refer to task.

> +			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) {


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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

On Mon, 16 May 2011, Michal Nazarewicz wrote:

> > Now that accessing current->comm needs to be protected,
> > @@ -2868,6 +2868,10 @@ 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
> > +		if ($line =~ /\b(?:current|task|tsk|t)\s*->\s*comm\b/) {
> 
> Not a checkpatch.pl expert but as far as I'm concerned, that looks reasonable.
> 
> I was sort of worried that t->comm could produce quite a few false positives
> but all its appearances in the kernel (seem to) refer to task.
> 

It's guaranteed to generate false positives since perf events uses a field 
of the same name to store a thread's comm, so I think the most important 
thing is for the checkpatch output to specify that this _may_ be a 
dereference of a thread's comm that needs get_task_comm() or %ptc.

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

[David Rientjes]

On Mon, 16 May 2011, Michal Nazarewicz wrote:

> > Now that accessing current->comm needs to be protected,
> > @@ -2868,6 +2868,10 @@ 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
> > +		if ($line =~ /\b(?:current|task|tsk|t)\s*->\s*comm\b/) {
> 
> Not a checkpatch.pl expert but as far as I'm concerned, that looks reasonable.
> 
> I was sort of worried that t->comm could produce quite a few false positives
> but all its appearances in the kernel (seem to) refer to task.
> 

It's guaranteed to generate false positives since perf events uses a field 
of the same name to store a thread's comm, so I think the most important 
thing is for the checkpatch output to specify that this _may_ be a 
dereference of a thread's comm that needs get_task_comm() or %ptc.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[Jiri Slaby]

On 05/16/2011 11:19 PM, John Stultz 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).
> 
> 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: 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
> 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 ++---
>  3 files changed, 19 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),		\

Hmm, you should also init the spinlock somewhere in copy_process.
Otherwise when a process is forked in the middle of [gs]et_task_comm
called on it on another cpu, you have two locked locks and only the
parent's will be unlocked, right?

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

thanks,
-- 
js

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

[Jiri Slaby]

On 05/16/2011 11:19 PM, John Stultz 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).
> 
> 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: 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
> 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 ++---
>  3 files changed, 19 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),		\

Hmm, you should also init the spinlock somewhere in copy_process.
Otherwise when a process is forked in the middle of [gs]et_task_comm
called on it on another cpu, you have two locked locks and only the
parent's will be unlocked, right?

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

thanks,
-- 
js

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[Joe Perches]

On Mon, 2011-05-16 at 14:34 -0700, David Rientjes wrote:
> On Mon, 16 May 2011, Michal Nazarewicz wrote:
> > > Now that accessing current->comm needs to be protected,
> > > +# check for current->comm usage
> > > +		if ($line =~ /\b(?:current|task|tsk|t)\s*->\s*comm\b/) {
> > Not a checkpatch.pl expert but as far as I'm concerned, that looks reasonable.

I think the only checkpatch expert is Andy Whitcroft.

You don't need (?: just (

curr, chip and object are pretty common (see below)

An option may be to specify another variable
common_comm_vars or something like it

our $common_comm_vars = qr{(?x:
	current|tsk|p|task|curr|chip|t|object|me
)};

and use that variable in your test

Treewide:

$ grep -rPoh --include=*.[ch] "\b\w+\s*\-\>\s*comm\b" * | \
	sort | uniq -c | sort -rn
    319 current->comm
     59 tsk->comm
     32 __entry->comm
     24 p->comm
     23 event->comm
     19 task->comm
     18 thread->comm
     15 self->comm
     14 c->comm
     13 curr->comm
     12 chip->comm
      9 t->comm
      8 object->comm
      8 me->comm
(others not shown)

Perf:

$ grep -rP --include=*.[ch] "\b\w+\s*\-\>\s*comm\b" tools/perf include/trace | \
	sort | uniq -c | sort -rn
     32 __entry->comm
     23 event->comm
     18 thread->comm
     15 self->comm
     14 c->comm
     10 current->comm
      3 tsk->comm
      3 task->comm
      3 p->comm
(others not shown)

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

[Joe Perches]

On Mon, 2011-05-16 at 14:34 -0700, David Rientjes wrote:
> On Mon, 16 May 2011, Michal Nazarewicz wrote:
> > > Now that accessing current->comm needs to be protected,
> > > +# check for current->comm usage
> > > +		if ($line =~ /\b(?:current|task|tsk|t)\s*->\s*comm\b/) {
> > Not a checkpatch.pl expert but as far as I'm concerned, that looks reasonable.

I think the only checkpatch expert is Andy Whitcroft.

You don't need (?: just (

curr, chip and object are pretty common (see below)

An option may be to specify another variable
common_comm_vars or something like it

our $common_comm_vars = qr{(?x:
	current|tsk|p|task|curr|chip|t|object|me
)};

and use that variable in your test

Treewide:

$ grep -rPoh --include=*.[ch] "\b\w+\s*\-\>\s*comm\b" * | \
	sort | uniq -c | sort -rn
    319 current->comm
     59 tsk->comm
     32 __entry->comm
     24 p->comm
     23 event->comm
     19 task->comm
     18 thread->comm
     15 self->comm
     14 c->comm
     13 curr->comm
     12 chip->comm
      9 t->comm
      8 object->comm
      8 me->comm
(others not shown)

Perf:

$ grep -rP --include=*.[ch] "\b\w+\s*\-\>\s*comm\b" tools/perf include/trace | \
	sort | uniq -c | sort -rn
     32 __entry->comm
     23 event->comm
     18 thread->comm
     15 self->comm
     14 c->comm
     10 current->comm
      3 tsk->comm
      3 task->comm
      3 p->comm
(others not shown)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[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


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[Michal Nazarewicz]

On Tue, 17 May 2011 01:04:50 +0200, Joe Perches <joe@perches.com> wrote:

> On Mon, 2011-05-16 at 14:34 -0700, David Rientjes wrote:
>> On Mon, 16 May 2011, Michal Nazarewicz wrote:
>> > > Now that accessing current->comm needs to be protected,
>> > > +# check for current->comm usage
>> > > +		if ($line =~ /\b(?:current|task|tsk|t)\s*->\s*comm\b/) {
>> > Not a checkpatch.pl expert but as far as I'm concerned, that looks  
>> reasonable.
>
> I think the only checkpatch expert is Andy Whitcroft.
>
> You don't need (?: just (

Yep, it's a micro-optimisation though.

-- 
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 3/3] checkpatch.pl: Add check for task comm references

[Michal Nazarewicz]

On Tue, 17 May 2011 01:04:50 +0200, Joe Perches <joe@perches.com> wrote:

> On Mon, 2011-05-16 at 14:34 -0700, David Rientjes wrote:
>> On Mon, 16 May 2011, Michal Nazarewicz wrote:
>> > > Now that accessing current->comm needs to be protected,
>> > > +# check for current->comm usage
>> > > +		if ($line =~ /\b(?:current|task|tsk|t)\s*->\s*comm\b/) {
>> > Not a checkpatch.pl expert but as far as I'm concerned, that looks  
>> reasonable.
>
> I think the only checkpatch expert is Andy Whitcroft.
>
> You don't need (?: just (

Yep, it's a micro-optimisation though.

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[Joe Perches]

On Tue, 2011-05-17 at 01:11 +0200, Michal Nazarewicz wrote:
> On Tue, 17 May 2011 01:04:50 +0200, Joe Perches <joe@perches.com> wrote:
> > On Mon, 2011-05-16 at 14:34 -0700, David Rientjes wrote:
> >> On Mon, 16 May 2011, Michal Nazarewicz wrote:
> >> > > Now that accessing current->comm needs to be protected,
> >> > > +# check for current->comm usage
> >> > > +		if ($line =~ /\b(?:current|task|tsk|t)\s*->\s*comm\b/) {
> >> > Not a checkpatch.pl expert but as far as I'm concerned, that looks  
> >> reasonable.
> > You don't need (?: just (
> Yep, it's a micro-optimisation though.

True, but it's not the common style in checkpatch.
You could submit patches to add non-capture markers to other () uses.

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

[Joe Perches]

On Tue, 2011-05-17 at 01:11 +0200, Michal Nazarewicz wrote:
> On Tue, 17 May 2011 01:04:50 +0200, Joe Perches <joe@perches.com> wrote:
> > On Mon, 2011-05-16 at 14:34 -0700, David Rientjes wrote:
> >> On Mon, 16 May 2011, Michal Nazarewicz wrote:
> >> > > Now that accessing current->comm needs to be protected,
> >> > > +# check for current->comm usage
> >> > > +		if ($line =~ /\b(?:current|task|tsk|t)\s*->\s*comm\b/) {
> >> > Not a checkpatch.pl expert but as far as I'm concerned, that looks  
> >> reasonable.
> > You don't need (?: just (
> Yep, it's a micro-optimisation though.

True, but it's not the common style in checkpatch.
You could submit patches to add non-capture markers to other () uses.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[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


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[John Stultz]

On Tue, 2011-05-17 at 00:01 +0200, Jiri Slaby wrote:
> On 05/16/2011 11:19 PM, John Stultz wrote:
> > 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),		\
> 
> Hmm, you should also init the spinlock somewhere in copy_process.
> Otherwise when a process is forked in the middle of [gs]et_task_comm
> called on it on another cpu, you have two locked locks and only the
> parent's will be unlocked, right?

Ah, yep. Fixed for the next version.

thanks!
-john

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

[John Stultz]

On Tue, 2011-05-17 at 00:01 +0200, Jiri Slaby wrote:
> On 05/16/2011 11:19 PM, John Stultz wrote:
> > 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),		\
> 
> Hmm, you should also init the spinlock somewhere in copy_process.
> Otherwise when a process is forked in the middle of [gs]et_task_comm
> called on it on another cpu, you have two locked locks and only the
> parent's will be unlocked, right?

Ah, yep. Fixed for the next version.

thanks!
-john

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[KOSAKI Motohiro]

(2011/05/17 6:19), John Stultz 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).
> 
> 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: 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
> Acked-by: David Rientjes<rientjes@google.com>
> Signed-off-by: John Stultz<john.stultz@linaro.org>

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

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

[KOSAKI Motohiro]

(2011/05/17 6:19), John Stultz 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).
> 
> 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: 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
> Acked-by: David Rientjes<rientjes@google.com>
> Signed-off-by: John Stultz<john.stultz@linaro.org>

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



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

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



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/3] v4 Improve task->comm locking situation

[Tetsuo Handa]

What about replacing
	char comm[TASK_COMM_LEN];
with
	char *rcu_commname;
and switching it atomically using RCU?

Advantages:
  Readers can use RCU read lock rather than spinlock.
  Makes task_struct smaller.

Disadvantages:
  Need to allocate TASK_COMM_LEN bytes upon dup_task_struct() and set_task_comm().
  Need to rewrite all task_struct.comm readers (steps shown below).

Steps to rewrite task_struct.comm readers

(1) Introduce a temporary accessor (say, task_comm).

    #define task_comm(tsk) (tsk)->comm

(2) Rewrite all tsk->comm users to task_comm(tsk).

(3) Replace sizeof(tsk->comm) with TASK_COMM_LEN.

(4) Temporarily rename from

    char comm[TASK_COMM_LEN];

    to

    char comm_access_me_via_task_comm[TASK_COMM_LEN];

    for blocking new tsk->comm users.

(5) Convert to use RCU.

(6) Rename from

    char comm_access_me_via_task_comm[TASK_COMM_LEN];

    to

    char *rcu_commname;

    .

(7) Rewrite task_comm(tsk) to use %ptc .

(8) Remove the temporary accessor.

Re: [PATCH 0/3] v4 Improve task->comm locking situation

[KOSAKI Motohiro]

(2011/05/18 12:16), Tetsuo Handa wrote:
> What about replacing
> 	char comm[TASK_COMM_LEN];
> with
> 	char *rcu_commname;
> and switching it atomically using RCU?
> 
> Advantages:
>    Readers can use RCU read lock rather than spinlock.
>    Makes task_struct smaller.
> 
> Disadvantages:
>    Need to allocate TASK_COMM_LEN bytes upon dup_task_struct() and set_task_comm().
>    Need to rewrite all task_struct.comm readers (steps shown below).
> 
> Steps to rewrite task_struct.comm readers
> 
> (1) Introduce a temporary accessor (say, task_comm).
> 
>      #define task_comm(tsk) (tsk)->comm
> 
> (2) Rewrite all tsk->comm users to task_comm(tsk).
> 
> (3) Replace sizeof(tsk->comm) with TASK_COMM_LEN.

The problem is, they aren't a few. But if you have a enough brave,
you can rewrite all. I'm not against it.

Re: [PATCH 0/3] v4 Improve task->comm locking situation

[John Stultz]

On Wed, 2011-05-18 at 12:16 +0900, Tetsuo Handa wrote:
> What about replacing
> 	char comm[TASK_COMM_LEN];
> with
> 	char *rcu_commname;
> and switching it atomically using RCU?

So I think this approach is definitely worth trying. However, I think
converting to RCU will be much easier once we've first converted all
current comm users to making use of the get_task_comm accessor
functions. In this way my hope is my current proposal will serve as a
cleanup step before further optimizations can be done.

thanks
-john

Re: [PATCH 0/3] v4 Improve task->comm locking situation

[Tetsuo Handa]

John Stultz wrote:
> So I think this approach is definitely worth trying. However, I think
> converting to RCU will be much easier once we've first converted all
> current comm users to making use of the get_task_comm accessor
> functions. In this way my hope is my current proposal will serve as a
> cleanup step before further optimizations can be done.

OK. But I guess converting all current comm users to making use of the
get_task_comm accessor is not easy because using get_task_comm() changes
locking dependency.

I think replacing

  "%s", tsk->comm

with

  "%ptc", tsk

(without using get_task_comm() inside pointer() because introducing a new lock
 might cause problem since callers can be inside a delicate condition based on
 the assumption that vsnprintf() will not block) should come first in order to
minimize direct ->comm users who may not be ready for this change.

After that, trying to replace

  task->comm

with

  char buf[TASK_COMM_LEN];
  get_task_comm(task, buf)

(with cautions for already held locks) may be considered.



By the way,

  sprintf(current->comm, "drbd%d_receiver", minor);

in drivers/block/drbd/drbd_receiver.c may be off-by-one
because DRBD_MINOR_COUNT_MAX is 256.