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

[RFC][PATCH 0/3] 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 seqlock 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.

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.

Also in this patch set I have a few examples of how I've
converted comm access to use get_task_comm. I've got quite 
a number of similar patches queued, but wanted to get some
feedback on the approach before I start patchbombing everyone.

Comments/feedback would be appreciated.

thanks
-john

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
  comm: timerstats: Protect task->comm access by using get_task_comm()
  comm: ext4: Protect task->comm access by using get_task_comm()

 fs/exec.c                 |   25 ++++++++++++++++++++-----
 fs/ext4/file.c            |    8 ++++++--
 fs/ext4/super.c           |   13 ++++++++++---
 include/linux/init_task.h |    1 +
 include/linux/sched.h     |    5 ++---
 kernel/timer.c            |    2 +-
 6 files changed, 40 insertions(+), 14 deletions(-)

-- 
1.7.3.2.146.gca209

[RFC][PATCH 0/3] 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 seqlock 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.

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.

Also in this patch set I have a few examples of how I've
converted comm access to use get_task_comm. I've got quite 
a number of similar patches queued, but wanted to get some
feedback on the approach before I start patchbombing everyone.

Comments/feedback would be appreciated.

thanks
-john

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
  comm: timerstats: Protect task->comm access by using get_task_comm()
  comm: ext4: Protect task->comm access by using get_task_comm()

 fs/exec.c                 |   25 ++++++++++++++++++++-----
 fs/ext4/file.c            |    8 ++++++--
 fs/ext4/super.c           |   13 ++++++++++---
 include/linux/init_task.h |    1 +
 include/linux/sched.h     |    5 ++---
 kernel/timer.c            |    2 +-
 6 files changed, 40 insertions(+), 14 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 seqlock
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: 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>
---
 fs/exec.c                 |   25 ++++++++++++++++++++-----
 include/linux/init_task.h |    1 +
 include/linux/sched.h     |    5 ++---
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 5e62d26..fcd056a 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -998,18 +998,32 @@ 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);
-	strncpy(buf, tsk->comm, sizeof(tsk->comm));
-	task_unlock(tsk);
+	unsigned long seq;
+
+	do {
+		seq = read_seqbegin(&tsk->comm_lock);
+
+		strncpy(buf, tsk->comm, sizeof(tsk->comm));
+
+	} while (read_seqretry(&tsk->comm_lock, seq));
+
 	return buf;
 }
 
 void set_task_comm(struct task_struct *tsk, char *buf)
 {
-	task_lock(tsk);
+	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);
+	write_seqlock_irqsave(&tsk->comm_lock, flags);
+	/*
 	 * Threads may access current->comm without holding
 	 * the task lock, so write the string carefully.
 	 * Readers without a lock may see incomplete new
@@ -1018,6 +1032,7 @@ 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));
+	write_sequnlock_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..b4f7584 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	= SEQLOCK_UNLOCKED,				\
 	.comm		= "swapper",					\
 	.thread		= INIT_THREAD,					\
 	.fs		= &init_fs,					\
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 18d63ce..f9324e4 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 */
-
+	seqlock_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 seqlock
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: 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>
---
 fs/exec.c                 |   25 ++++++++++++++++++++-----
 include/linux/init_task.h |    1 +
 include/linux/sched.h     |    5 ++---
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 5e62d26..fcd056a 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -998,18 +998,32 @@ 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);
-	strncpy(buf, tsk->comm, sizeof(tsk->comm));
-	task_unlock(tsk);
+	unsigned long seq;
+
+	do {
+		seq = read_seqbegin(&tsk->comm_lock);
+
+		strncpy(buf, tsk->comm, sizeof(tsk->comm));
+
+	} while (read_seqretry(&tsk->comm_lock, seq));
+
 	return buf;
 }
 
 void set_task_comm(struct task_struct *tsk, char *buf)
 {
-	task_lock(tsk);
+	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);
+	write_seqlock_irqsave(&tsk->comm_lock, flags);
+	/*
 	 * Threads may access current->comm without holding
 	 * the task lock, so write the string carefully.
 	 * Readers without a lock may see incomplete new
@@ -1018,6 +1032,7 @@ 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));
+	write_sequnlock_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..b4f7584 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	= SEQLOCK_UNLOCKED,				\
 	.comm		= "swapper",					\
 	.thread		= INIT_THREAD,					\
 	.fs		= &init_fs,					\
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 18d63ce..f9324e4 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 */
-
+	seqlock_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] comm: timerstats: Protect task->comm access by using get_task_comm()

[John Stultz]

Converts the timerstats code to use get_task_comm for protected
comm access.

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>
---
 kernel/timer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index fd61986..85308fb 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -379,7 +379,7 @@ void __timer_stats_timer_set_start_info(struct timer_list *timer, void *addr)
 		return;
 
 	timer->start_site = addr;
-	memcpy(timer->start_comm, current->comm, TASK_COMM_LEN);
+	get_task_comm(timer->start_comm, current);
 	timer->start_pid = current->pid;
 }
 
-- 
1.7.3.2.146.gca209

[PATCH 2/3] comm: timerstats: Protect task->comm access by using get_task_comm()

[John Stultz]

Converts the timerstats code to use get_task_comm for protected
comm access.

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>
---
 kernel/timer.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index fd61986..85308fb 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -379,7 +379,7 @@ void __timer_stats_timer_set_start_info(struct timer_list *timer, void *addr)
 		return;
 
 	timer->start_site = addr;
-	memcpy(timer->start_comm, current->comm, TASK_COMM_LEN);
+	get_task_comm(timer->start_comm, current);
 	timer->start_pid = current->pid;
 }
 
-- 
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] comm: ext4: Protect task->comm access by using get_task_comm()

[John Stultz]

Converts ext4 comm access to use the safe get_task_comm accessor.

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>
---
 fs/ext4/file.c  |    8 ++++++--
 fs/ext4/super.c |   13 ++++++++++---
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 7b80d54..d37414e 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -124,11 +124,15 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 		static unsigned long unaligned_warn_time;
 
 		/* Warn about this once per day */
-		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ))
+		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ)) {
+			char comm[TASK_COMM_LEN];
+
+			get_task_comm(comm, current);
 			ext4_msg(inode->i_sb, KERN_WARNING,
 				 "Unaligned AIO/DIO on inode %ld by %s; "
 				 "performance will be poor.",
-				 inode->i_ino, current->comm);
+				 inode->i_ino, comm);
+		}
 		mutex_lock(ext4_aio_mutex(inode));
 		ext4_aiodio_wait(inode);
 	}
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8553dfb..6c9151f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -409,12 +409,15 @@ void __ext4_error(struct super_block *sb, const char *function,
 {
 	struct va_format vaf;
 	va_list args;
+	char comm[TASK_COMM_LEN];
 
 	va_start(args, fmt);
 	vaf.fmt = fmt;
 	vaf.va = &args;
+
+	get_task_comm(comm, current);
 	printk(KERN_CRIT "EXT4-fs error (device %s): %s:%d: comm %s: %pV\n",
-	       sb->s_id, function, line, current->comm, &vaf);
+	       sb->s_id, function, line, comm, &vaf);
 	va_end(args);
 
 	ext4_handle_error(sb);
@@ -427,6 +430,7 @@ void ext4_error_inode(struct inode *inode, const char *function,
 	va_list args;
 	struct va_format vaf;
 	struct ext4_super_block *es = EXT4_SB(inode->i_sb)->s_es;
+	char comm[TASK_COMM_LEN];
 
 	es->s_last_error_ino = cpu_to_le32(inode->i_ino);
 	es->s_last_error_block = cpu_to_le64(block);
@@ -438,7 +442,8 @@ void ext4_error_inode(struct inode *inode, const char *function,
 	       inode->i_sb->s_id, function, line, inode->i_ino);
 	if (block)
 		printk(KERN_CONT "block %llu: ", block);
-	printk(KERN_CONT "comm %s: %pV\n", current->comm, &vaf);
+	get_task_comm(comm, current);
+	printk(KERN_CONT "comm %s: %pV\n", comm, &vaf);
 	va_end(args);
 
 	ext4_handle_error(inode->i_sb);
@@ -453,6 +458,7 @@ void ext4_error_file(struct file *file, const char *function,
 	struct ext4_super_block *es;
 	struct inode *inode = file->f_dentry->d_inode;
 	char pathname[80], *path;
+	char comm[TASK_COMM_LEN];
 
 	es = EXT4_SB(inode->i_sb)->s_es;
 	es->s_last_error_ino = cpu_to_le32(inode->i_ino);
@@ -468,7 +474,8 @@ void ext4_error_file(struct file *file, const char *function,
 	va_start(args, fmt);
 	vaf.fmt = fmt;
 	vaf.va = &args;
-	printk(KERN_CONT "comm %s: path %s: %pV\n", current->comm, path, &vaf);
+	get_task_comm(comm, current);
+	printk(KERN_CONT "comm %s: path %s: %pV\n", comm, path, &vaf);
 	va_end(args);
 
 	ext4_handle_error(inode->i_sb);
-- 
1.7.3.2.146.gca209

[PATCH 3/3] comm: ext4: Protect task->comm access by using get_task_comm()

[John Stultz]

Converts ext4 comm access to use the safe get_task_comm accessor.

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>
---
 fs/ext4/file.c  |    8 ++++++--
 fs/ext4/super.c |   13 ++++++++++---
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 7b80d54..d37414e 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -124,11 +124,15 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
 		static unsigned long unaligned_warn_time;
 
 		/* Warn about this once per day */
-		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ))
+		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ)) {
+			char comm[TASK_COMM_LEN];
+
+			get_task_comm(comm, current);
 			ext4_msg(inode->i_sb, KERN_WARNING,
 				 "Unaligned AIO/DIO on inode %ld by %s; "
 				 "performance will be poor.",
-				 inode->i_ino, current->comm);
+				 inode->i_ino, comm);
+		}
 		mutex_lock(ext4_aio_mutex(inode));
 		ext4_aiodio_wait(inode);
 	}
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8553dfb..6c9151f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -409,12 +409,15 @@ void __ext4_error(struct super_block *sb, const char *function,
 {
 	struct va_format vaf;
 	va_list args;
+	char comm[TASK_COMM_LEN];
 
 	va_start(args, fmt);
 	vaf.fmt = fmt;
 	vaf.va = &args;
+
+	get_task_comm(comm, current);
 	printk(KERN_CRIT "EXT4-fs error (device %s): %s:%d: comm %s: %pV\n",
-	       sb->s_id, function, line, current->comm, &vaf);
+	       sb->s_id, function, line, comm, &vaf);
 	va_end(args);
 
 	ext4_handle_error(sb);
@@ -427,6 +430,7 @@ void ext4_error_inode(struct inode *inode, const char *function,
 	va_list args;
 	struct va_format vaf;
 	struct ext4_super_block *es = EXT4_SB(inode->i_sb)->s_es;
+	char comm[TASK_COMM_LEN];
 
 	es->s_last_error_ino = cpu_to_le32(inode->i_ino);
 	es->s_last_error_block = cpu_to_le64(block);
@@ -438,7 +442,8 @@ void ext4_error_inode(struct inode *inode, const char *function,
 	       inode->i_sb->s_id, function, line, inode->i_ino);
 	if (block)
 		printk(KERN_CONT "block %llu: ", block);
-	printk(KERN_CONT "comm %s: %pV\n", current->comm, &vaf);
+	get_task_comm(comm, current);
+	printk(KERN_CONT "comm %s: %pV\n", comm, &vaf);
 	va_end(args);
 
 	ext4_handle_error(inode->i_sb);
@@ -453,6 +458,7 @@ void ext4_error_file(struct file *file, const char *function,
 	struct ext4_super_block *es;
 	struct inode *inode = file->f_dentry->d_inode;
 	char pathname[80], *path;
+	char comm[TASK_COMM_LEN];
 
 	es = EXT4_SB(inode->i_sb)->s_es;
 	es->s_last_error_ino = cpu_to_le32(inode->i_ino);
@@ -468,7 +474,8 @@ void ext4_error_file(struct file *file, const char *function,
 	va_start(args, fmt);
 	vaf.fmt = fmt;
 	vaf.va = &args;
-	printk(KERN_CONT "comm %s: path %s: %pV\n", current->comm, path, &vaf);
+	get_task_comm(comm, current);
+	printk(KERN_CONT "comm %s: path %s: %pV\n", comm, path, &vaf);
 	va_end(args);
 
 	ext4_handle_error(inode->i_sb);
-- 
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] comm: ext4: Protect task->comm access by using get_task_comm()

[David Rientjes]

On Wed, 27 Apr 2011, John Stultz wrote:

> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 7b80d54..d37414e 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -124,11 +124,15 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  		static unsigned long unaligned_warn_time;
>  
>  		/* Warn about this once per day */
> -		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ))
> +		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ)) {
> +			char comm[TASK_COMM_LEN];
> +
> +			get_task_comm(comm, current);
>  			ext4_msg(inode->i_sb, KERN_WARNING,
>  				 "Unaligned AIO/DIO on inode %ld by %s; "
>  				 "performance will be poor.",
> -				 inode->i_ino, current->comm);
> +				 inode->i_ino, comm);
> +		}
>  		mutex_lock(ext4_aio_mutex(inode));
>  		ext4_aiodio_wait(inode);
>  	}

Thanks very much for looking into concurrent readers of current->comm, 
John!

This patch in the series demonstrates one of the problems with using 
get_task_comm(), however: we must allocate a 16-byte buffer on the stack 
and that could become risky if we don't know its current depth.  We may be 
particularly deep in the stack and then cause an overflow because of the 
16 bytes.

I'm wondering if it would be better for ->comm to be protected by a 
spinlock (or rwlock) other than ->alloc_lock and then just require readers 
to take the lock prior to dereferencing it?  That's what is done in the 
oom killer with task_lock().  Perhaps you could introduce new 
task_comm_lock() and task_comm_unlock() to prevent the extra stack usage 
in over 300 locations within the kernel?

Re: [PATCH 3/3] comm: ext4: Protect task->comm access by using get_task_comm()

[David Rientjes]

On Wed, 27 Apr 2011, John Stultz wrote:

> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 7b80d54..d37414e 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -124,11 +124,15 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  		static unsigned long unaligned_warn_time;
>  
>  		/* Warn about this once per day */
> -		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ))
> +		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ)) {
> +			char comm[TASK_COMM_LEN];
> +
> +			get_task_comm(comm, current);
>  			ext4_msg(inode->i_sb, KERN_WARNING,
>  				 "Unaligned AIO/DIO on inode %ld by %s; "
>  				 "performance will be poor.",
> -				 inode->i_ino, current->comm);
> +				 inode->i_ino, comm);
> +		}
>  		mutex_lock(ext4_aio_mutex(inode));
>  		ext4_aiodio_wait(inode);
>  	}

Thanks very much for looking into concurrent readers of current->comm, 
John!

This patch in the series demonstrates one of the problems with using 
get_task_comm(), however: we must allocate a 16-byte buffer on the stack 
and that could become risky if we don't know its current depth.  We may be 
particularly deep in the stack and then cause an overflow because of the 
16 bytes.

I'm wondering if it would be better for ->comm to be protected by a 
spinlock (or rwlock) other than ->alloc_lock and then just require readers 
to take the lock prior to dereferencing it?  That's what is done in the 
oom killer with task_lock().  Perhaps you could introduce new 
task_comm_lock() and task_comm_unlock() to prevent the extra stack usage 
in over 300 locations within the kernel?

--
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] comm: ext4: Protect task->comm access by using get_task_comm()

[Andrew Morton]

On Thu, 28 Apr 2011 14:35:32 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> On Wed, 27 Apr 2011, John Stultz wrote:
> 
> > diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> > index 7b80d54..d37414e 100644
> > --- a/fs/ext4/file.c
> > +++ b/fs/ext4/file.c
> > @@ -124,11 +124,15 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
> >  		static unsigned long unaligned_warn_time;
> >  
> >  		/* Warn about this once per day */
> > -		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ))
> > +		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ)) {
> > +			char comm[TASK_COMM_LEN];
> > +
> > +			get_task_comm(comm, current);
> >  			ext4_msg(inode->i_sb, KERN_WARNING,
> >  				 "Unaligned AIO/DIO on inode %ld by %s; "
> >  				 "performance will be poor.",
> > -				 inode->i_ino, current->comm);
> > +				 inode->i_ino, comm);
> > +		}
> >  		mutex_lock(ext4_aio_mutex(inode));
> >  		ext4_aiodio_wait(inode);
> >  	}
> 
> Thanks very much for looking into concurrent readers of current->comm, 
> John!
> 
> This patch in the series demonstrates one of the problems with using 
> get_task_comm(), however: we must allocate a 16-byte buffer on the stack 
> and that could become risky if we don't know its current depth.  We may be 
> particularly deep in the stack and then cause an overflow because of the 
> 16 bytes.
> 
> I'm wondering if it would be better for ->comm to be protected by a 
> spinlock (or rwlock) other than ->alloc_lock and then just require readers 
> to take the lock prior to dereferencing it?  That's what is done in the 
> oom killer with task_lock().  Perhaps you could introduce new 
> task_comm_lock() and task_comm_unlock() to prevent the extra stack usage 
> in over 300 locations within the kernel?

16 bytes isn't all that much.  It's just two longs worth.

I'm suspecting that approximately 100% of the get_task_comm() callsites
are using it for a printk, so how about we add a %p thingy for it then
zap lots of code?

I read the changelogs and can't work out why a seqlock was added.  What
was wrong with the task_lock()?

Re: [PATCH 3/3] comm: ext4: Protect task->comm access by using get_task_comm()

[Andrew Morton]

On Thu, 28 Apr 2011 14:35:32 -0700 (PDT)
David Rientjes <rientjes@google.com> wrote:

> On Wed, 27 Apr 2011, John Stultz wrote:
> 
> > diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> > index 7b80d54..d37414e 100644
> > --- a/fs/ext4/file.c
> > +++ b/fs/ext4/file.c
> > @@ -124,11 +124,15 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
> >  		static unsigned long unaligned_warn_time;
> >  
> >  		/* Warn about this once per day */
> > -		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ))
> > +		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ)) {
> > +			char comm[TASK_COMM_LEN];
> > +
> > +			get_task_comm(comm, current);
> >  			ext4_msg(inode->i_sb, KERN_WARNING,
> >  				 "Unaligned AIO/DIO on inode %ld by %s; "
> >  				 "performance will be poor.",
> > -				 inode->i_ino, current->comm);
> > +				 inode->i_ino, comm);
> > +		}
> >  		mutex_lock(ext4_aio_mutex(inode));
> >  		ext4_aiodio_wait(inode);
> >  	}
> 
> Thanks very much for looking into concurrent readers of current->comm, 
> John!
> 
> This patch in the series demonstrates one of the problems with using 
> get_task_comm(), however: we must allocate a 16-byte buffer on the stack 
> and that could become risky if we don't know its current depth.  We may be 
> particularly deep in the stack and then cause an overflow because of the 
> 16 bytes.
> 
> I'm wondering if it would be better for ->comm to be protected by a 
> spinlock (or rwlock) other than ->alloc_lock and then just require readers 
> to take the lock prior to dereferencing it?  That's what is done in the 
> oom killer with task_lock().  Perhaps you could introduce new 
> task_comm_lock() and task_comm_unlock() to prevent the extra stack usage 
> in over 300 locations within the kernel?

16 bytes isn't all that much.  It's just two longs worth.

I'm suspecting that approximately 100% of the get_task_comm() callsites
are using it for a printk, so how about we add a %p thingy for it then
zap lots of code?

I read the changelogs and can't work out why a seqlock was added.  What
was wrong with the task_lock()?

--
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] comm: ext4: Protect task->comm access by using get_task_comm()

[Andrew Morton]

Also.  As direct access to current->comm is now verboten, we should add
a checkpatch rule to shout at people when they do it.

Re: [PATCH 3/3] comm: ext4: Protect task->comm access by using get_task_comm()

[Andrew Morton]

Also.  As direct access to current->comm is now verboten, we should add
a checkpatch rule to shout at people when they do 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 3/3] comm: ext4: Protect task->comm access by using get_task_comm()

[John Stultz]

On Wed, 2011-05-04 at 16:36 -0700, Andrew Morton wrote:
> On Thu, 28 Apr 2011 14:35:32 -0700 (PDT)
> David Rientjes <rientjes@google.com> wrote:
> 
> > On Wed, 27 Apr 2011, John Stultz wrote:
> > 
> > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> > > index 7b80d54..d37414e 100644
> > > --- a/fs/ext4/file.c
> > > +++ b/fs/ext4/file.c
> > > @@ -124,11 +124,15 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
> > >  		static unsigned long unaligned_warn_time;
> > >  
> > >  		/* Warn about this once per day */
> > > -		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ))
> > > +		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ)) {
> > > +			char comm[TASK_COMM_LEN];
> > > +
> > > +			get_task_comm(comm, current);
> > >  			ext4_msg(inode->i_sb, KERN_WARNING,
> > >  				 "Unaligned AIO/DIO on inode %ld by %s; "
> > >  				 "performance will be poor.",
> > > -				 inode->i_ino, current->comm);
> > > +				 inode->i_ino, comm);
> > > +		}
> > >  		mutex_lock(ext4_aio_mutex(inode));
> > >  		ext4_aiodio_wait(inode);
> > >  	}
> > 
> > Thanks very much for looking into concurrent readers of current->comm, 
> > John!
> > 
> > This patch in the series demonstrates one of the problems with using 
> > get_task_comm(), however: we must allocate a 16-byte buffer on the stack 
> > and that could become risky if we don't know its current depth.  We may be 
> > particularly deep in the stack and then cause an overflow because of the 
> > 16 bytes.
> > 
> > I'm wondering if it would be better for ->comm to be protected by a 
> > spinlock (or rwlock) other than ->alloc_lock and then just require readers 
> > to take the lock prior to dereferencing it?  That's what is done in the 
> > oom killer with task_lock().  Perhaps you could introduce new 
> > task_comm_lock() and task_comm_unlock() to prevent the extra stack usage 
> > in over 300 locations within the kernel?
> 
> 16 bytes isn't all that much.  It's just two longs worth.
> 
> I'm suspecting that approximately 100% of the get_task_comm() callsites
> are using it for a printk, so how about we add a %p thingy for it then
> zap lots of code?

DaveH suggested the same, actually. And that would work with the
seqlocking pretty easily to avoid DavidR's issue.

> I read the changelogs and can't work out why a seqlock was added.  What
> was wrong with the task_lock()?

Sorry that wasn't clear, apparently its not always safe to grab the task
lock, as it might be held for other reasons. DavidR pointed out one such
case in Dave Hansen's "break out page allocation warning code" patch
(GFP_ATOMIC allocation).

Further, task_lock doesn't disable irqs, and there may be cases where we
access current->comm from irq context. Introducing a new seqlock
(disabling irqs on the comm write path) allows us to be sure we won't
hit such an issue when mass converting current->comm accessors.

thanks
-john

Re: [PATCH 3/3] comm: ext4: Protect task->comm access by using get_task_comm()

[John Stultz]

On Wed, 2011-05-04 at 16:36 -0700, Andrew Morton wrote:
> On Thu, 28 Apr 2011 14:35:32 -0700 (PDT)
> David Rientjes <rientjes@google.com> wrote:
> 
> > On Wed, 27 Apr 2011, John Stultz wrote:
> > 
> > > diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> > > index 7b80d54..d37414e 100644
> > > --- a/fs/ext4/file.c
> > > +++ b/fs/ext4/file.c
> > > @@ -124,11 +124,15 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
> > >  		static unsigned long unaligned_warn_time;
> > >  
> > >  		/* Warn about this once per day */
> > > -		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ))
> > > +		if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ)) {
> > > +			char comm[TASK_COMM_LEN];
> > > +
> > > +			get_task_comm(comm, current);
> > >  			ext4_msg(inode->i_sb, KERN_WARNING,
> > >  				 "Unaligned AIO/DIO on inode %ld by %s; "
> > >  				 "performance will be poor.",
> > > -				 inode->i_ino, current->comm);
> > > +				 inode->i_ino, comm);
> > > +		}
> > >  		mutex_lock(ext4_aio_mutex(inode));
> > >  		ext4_aiodio_wait(inode);
> > >  	}
> > 
> > Thanks very much for looking into concurrent readers of current->comm, 
> > John!
> > 
> > This patch in the series demonstrates one of the problems with using 
> > get_task_comm(), however: we must allocate a 16-byte buffer on the stack 
> > and that could become risky if we don't know its current depth.  We may be 
> > particularly deep in the stack and then cause an overflow because of the 
> > 16 bytes.
> > 
> > I'm wondering if it would be better for ->comm to be protected by a 
> > spinlock (or rwlock) other than ->alloc_lock and then just require readers 
> > to take the lock prior to dereferencing it?  That's what is done in the 
> > oom killer with task_lock().  Perhaps you could introduce new 
> > task_comm_lock() and task_comm_unlock() to prevent the extra stack usage 
> > in over 300 locations within the kernel?
> 
> 16 bytes isn't all that much.  It's just two longs worth.
> 
> I'm suspecting that approximately 100% of the get_task_comm() callsites
> are using it for a printk, so how about we add a %p thingy for it then
> zap lots of code?

DaveH suggested the same, actually. And that would work with the
seqlocking pretty easily to avoid DavidR's issue.

> I read the changelogs and can't work out why a seqlock was added.  What
> was wrong with the task_lock()?

Sorry that wasn't clear, apparently its not always safe to grab the task
lock, as it might be held for other reasons. DavidR pointed out one such
case in Dave Hansen's "break out page allocation warning code" patch
(GFP_ATOMIC allocation).

Further, task_lock doesn't disable irqs, and there may be cases where we
access current->comm from irq context. Introducing a new seqlock
(disabling irqs on the comm write path) allows us to be sure we won't
hit such an issue when mass converting current->comm accessors.

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] comm: ext4: Protect task->comm access by using get_task_comm()

[Ted Ts'o]

On Wed, May 04, 2011 at 04:55:10PM -0700, John Stultz wrote:
> > I'm suspecting that approximately 100% of the get_task_comm() callsites
> > are using it for a printk, so how about we add a %p thingy for it then
> > zap lots of code?
> 
> DaveH suggested the same, actually. And that would work with the
> seqlocking pretty easily to avoid DavidR's issue.

+1 for a %p thingy for printk's; although the other potential use case
that we should think about is for tracepoints.  Getting something that
works for ftrace as well as perf would be a really good thing.

I suspect what we would want to do though (since people have been
trying very hard to keep the trace records as small as possible, so we
can include as much as possible) is to only record the pid, and have a
tracepoint which reports when process's comm value has been set to a
new value.  So any objections to adding a tracepoint in
set_task_comm()?

And would you like me to send the patch, or do you want to do it since
you're putting a patch series together anyway?

    	      	      	      	  	    - Ted

Re: [PATCH 3/3] comm: ext4: Protect task->comm access by using get_task_comm()

[Ted Ts'o]

On Wed, May 04, 2011 at 04:55:10PM -0700, John Stultz wrote:
> > I'm suspecting that approximately 100% of the get_task_comm() callsites
> > are using it for a printk, so how about we add a %p thingy for it then
> > zap lots of code?
> 
> DaveH suggested the same, actually. And that would work with the
> seqlocking pretty easily to avoid DavidR's issue.

+1 for a %p thingy for printk's; although the other potential use case
that we should think about is for tracepoints.  Getting something that
works for ftrace as well as perf would be a really good thing.

I suspect what we would want to do though (since people have been
trying very hard to keep the trace records as small as possible, so we
can include as much as possible) is to only record the pid, and have a
tracepoint which reports when process's comm value has been set to a
new value.  So any objections to adding a tracepoint in
set_task_comm()?

And would you like me to send the patch, or do you want to do it since
you're putting a patch series together anyway?

    	      	      	      	  	    - Ted

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