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

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

I've also introduced a printk %ptc accessor, which makes the
conversion to locked access simpler (as most uses are for printks).

And new in this version: I've added a checkpatch rule to try
to catch any new current->comm users from being introduced.
Although I suspect the script will need some additional work.

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: 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 current->comm references

 fs/exec.c                 |   25 ++++++++++++++++++++-----
 include/linux/init_task.h |    1 +
 include/linux/sched.h     |    5 ++---
 lib/vsprintf.c            |   27 +++++++++++++++++++++++++++
 scripts/checkpatch.pl     |    4 ++++
 5 files changed, 54 insertions(+), 8 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: 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                 |   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>

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

[KOSAKI Motohiro]

Hi

Sorry for the long delay.

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

Can you please explain why we should use seqlock? That said,
we didn't use seqlock for /proc items. because, plenty seqlock
write may makes readers busy wait. Then, if we don't have another
protection, we give the local DoS attack way to attackers.

task->comm is used for very fundamentally. then, I doubt we can
assume write is enough rare. Why can't we use normal spinlock?


--
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 Fri, 2011-05-13 at 20:13 +0900, KOSAKI Motohiro wrote:
> Hi
> 
> Sorry for the long delay.
> 
> >   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;
> >   }
> 
> Can you please explain why we should use seqlock? That said,
> we didn't use seqlock for /proc items. because, plenty seqlock
> write may makes readers busy wait. Then, if we don't have another
> protection, we give the local DoS attack way to attackers.

So you're saying that heavy write contention can cause reader
starvation? 

> task->comm is used for very fundamentally. then, I doubt we can
> assume write is enough rare. Why can't we use normal spinlock?

I think writes are likely to be fairly rare. Tasks can only name
themselves or sibling threads, so I'm not sure I see the risk here.

Mind going into more detail?

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

[KOSAKI Motohiro]

>> Can you please explain why we should use seqlock? That said,
>> we didn't use seqlock for /proc items. because, plenty seqlock
>> write may makes readers busy wait. Then, if we don't have another
>> protection, we give the local DoS attack way to attackers.
>
> So you're saying that heavy write contention can cause reader
> starvation?

Yes.

>> task->comm is used for very fundamentally. then, I doubt we can
>> assume write is enough rare. Why can't we use normal spinlock?
>
> I think writes are likely to be fairly rare. Tasks can only name
> themselves or sibling threads, so I'm not sure I see the risk here.

reader starvation may cause another task's starvation if reader have
an another lock.
And, "only sibling" don't make any security gurantee as I said past.

Thanks.

--
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 Sat, 2011-05-14 at 20:12 +0900, KOSAKI Motohiro wrote:
> >> Can you please explain why we should use seqlock? That said,
> >> we didn't use seqlock for /proc items. because, plenty seqlock
> >> write may makes readers busy wait. Then, if we don't have another
> >> protection, we give the local DoS attack way to attackers.
> >
> > So you're saying that heavy write contention can cause reader
> > starvation?
> 
> Yes.
> 
> >> task->comm is used for very fundamentally. then, I doubt we can
> >> assume write is enough rare. Why can't we use normal spinlock?
> >
> > I think writes are likely to be fairly rare. Tasks can only name
> > themselves or sibling threads, so I'm not sure I see the risk here.
> 
> reader starvation may cause another task's starvation if reader have
> an another lock.

So the risk is a thread rewriting its own comm over and over could
starve some other critical task trying to read the comm.

Ok. It makes it a little more costly, but fair enough.

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>

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

[John Stultz]

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

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

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

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

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

--
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 current->comm references

[John Stultz]

Now that accessing current->comm needs to be protected,
avoid new current->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.

Close review and feedback would be appreciated.

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>
---
 scripts/checkpatch.pl |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d867081..9d2eab5 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 =~ /current->comm/) {
+			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 current->comm references

[Jiri Slaby]

On 05/13/2011 01:02 AM, John Stultz wrote:
> Now that accessing current->comm needs to be protected,
> avoid new current->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.
> 
> Close review and feedback would be appreciated.
> 
> 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>
> ---
>  scripts/checkpatch.pl |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d867081..9d2eab5 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 =~ /current->comm/) {

This should be something like \b(current|task|tsk|t)->comm\b to catch
also non-current comm accesses...

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


-- 
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 current->comm references

[Michal Nazarewicz]

> On 05/13/2011 01:02 AM, John Stultz wrote:
>> @@ -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 =~ /current->comm/) {

On Fri, 13 May 2011 08:33:39 +0200, Jiri Slaby wrote:
> This should be something like \b(current|task|tsk|t)->comm\b to catch
> also non-current comm accesses...

Or \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);
>> +		}

-- 
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 current->comm references

[David Rientjes]

On Fri, 13 May 2011, Michal Nazarewicz wrote:

> On Fri, 13 May 2011 08:33:39 +0200, Jiri Slaby wrote:
> > This should be something like \b(current|task|tsk|t)->comm\b to catch
> > also non-current comm accesses...
> 
> Or \b(?:current|task|tsk|t)\s*->\s*comm\b.
> 

Add 'p' to the mix :)

$ grep -r "struct task_struct \*p[; ]" * | wc -l
119

--
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 current->comm references

[KOSAKI Motohiro]

>   scripts/checkpatch.pl |    4 ++++
>   1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d867081..9d2eab5 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 =~ /current->comm/) {
> +			WARN("comm access needs to be protected. Use get_task_comm, or printk's \%ptc formatting.\n" . $herecurr);

I think we should convert all of task->comm usage. not only current. At least, you plan to remove task_lock() from
%ptc patch later.

thanks.



--
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 current->comm references

[John Stultz]

On Fri, 2011-05-13 at 20:13 +0900, KOSAKI Motohiro wrote:
> >   scripts/checkpatch.pl |    4 ++++
> >   1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index d867081..9d2eab5 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 =~ /current->comm/) {
> > +			WARN("comm access needs to be protected. Use get_task_comm, or printk's \%ptc formatting.\n" . $herecurr);
> 
> I think we should convert all of task->comm usage. not only current. At least, you plan to remove task_lock() from
> %ptc patch later.

Yea, I'll be updating the patch to try to catch more then just
current->comm.

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>