v6 tries to address the latest round of issues. Again, hopefully
this is getting close to something that can be queued for 2.6.40.
Since my commit 4614a696bd1c3a9af3a08f0e5874830a85b889d4, the
current->comm value could be changed by other threads.
This changed the comm locking rules, which previously allowed for
unlocked current->comm access, since only the thread itself could
change its comm.
While this was brought up at the time, it was not considered
problematic, as the comm writing was done in such a way that
only null or incomplete comms could be read. However, recently
folks have made it clear they want to see this issue resolved.
So fair enough, as I opened this can of worms, I should work
to resolve it and this patchset is my initial attempt.
The proposed solution here is to introduce a new spinlock that
exclusively protects the comm value. We use it to serialize
access via get_task_comm() and set_task_comm(). Since some
comm access is open-coded using the task lock, we preserve
the task locking in set_task_comm for now. Once all comm
access is converted to using get_task_comm, we can clean that
up as well.
I've also introduced a printk %ptc accessor, which makes the
conversion to locked access simpler (as most uses are for printks)
as well as a checkpatch rule to try to catch any new current->comm
users from being introduced.
New in this version: More tweaks to the checkpatch regex, and
added a unlocked task->comm accessor for performance critical
code paths that can handle the potential null or incomplete comm.
Hopefully this will allow for a smooth transition, where we can
slowly fix up the unlocked current->comm access bit by bit,
reducing the race window with each patch, while not making the
situation any worse then it was yesterday.
Thanks for the comments and feedback so far.
Any additional comments/feedback would still be appreciated.
thanks
-john
CC: Joe Perches <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Michal Nazarewicz <[email protected]>
CC: Andy Whitcroft <[email protected]>
CC: Jiri Slaby <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: David Rientjes <[email protected]>
CC: Dave Hansen <[email protected]>
CC: Andrew Morton <[email protected]>
CC: [email protected]
John Stultz (4):
comm: Introduce comm_lock spinlock to protect task->comm access
comm: Add lock-free task->comm accessor
printk: Add %ptc to safely print a task's comm
checkpatch.pl: Add check for task comm references
fs/exec.c | 32 +++++++++++++++++++++++++++++---
include/linux/init_task.h | 1 +
include/linux/sched.h | 6 +++---
kernel/fork.c | 1 +
lib/vsprintf.c | 24 ++++++++++++++++++++++++
scripts/checkpatch.pl | 6 ++++++
6 files changed, 64 insertions(+), 6 deletions(-)
--
1.7.3.2.146.gca209
Since my commit 4614a696bd1c3a9af3a08f0e5874830a85b889d4, the
task->comm value could be changed by other threads.
Thus, the implicit rules for current->comm access being safe without
locking are no longer true. Accessing current->comm without holding
the task lock may result in null or incomplete strings (however,
access won't run off the end of the string).
In order to properly fix this, I've introduced a comm_lock spinlock
which will protect comm access and modified get_task_comm() and
set_task_comm() to use it.
Since there are a number of cases where comm access is open-coded
safely grabbing the task_lock(), we preserve the task locking in
set_task_comm, so those users are also safe.
With this patch, users that access current->comm without a lock
are still prone to null/incomplete comm strings, but it should
be no worse then it is now.
The next step is to go through and convert all comm accesses to
use get_task_comm(). This is substantial, but can be done bit by
bit, reducing the race windows with each patch.
CC: Joe Perches <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Michal Nazarewicz <[email protected]>
CC: Andy Whitcroft <[email protected]>
CC: Jiri Slaby <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: David Rientjes <[email protected]>
CC: Dave Hansen <[email protected]>
CC: Andrew Morton <[email protected]>
CC: [email protected]
Acked-by: David Rientjes <[email protected]>
Reviewed-by: KOSAKI Motohiro <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
fs/exec.c | 19 ++++++++++++++++---
include/linux/init_task.h | 1 +
include/linux/sched.h | 5 ++---
kernel/fork.c | 1 +
4 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 5e62d26..34fa611 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -998,17 +998,28 @@ static void flush_old_files(struct files_struct * files)
char *get_task_comm(char *buf, struct task_struct *tsk)
{
- /* buf must be at least sizeof(tsk->comm) in size */
- task_lock(tsk);
+ unsigned long flags;
+
+ spin_lock_irqsave(&tsk->comm_lock, flags);
strncpy(buf, tsk->comm, sizeof(tsk->comm));
- task_unlock(tsk);
+ spin_unlock_irqrestore(&tsk->comm_lock, flags);
return buf;
}
void set_task_comm(struct task_struct *tsk, char *buf)
{
+ unsigned long flags;
+
+ /*
+ * XXX - Even though comm is protected by comm_lock,
+ * we take the task_lock here to serialize against
+ * current users that directly access comm.
+ * Once those users are removed, we can drop the
+ * task locking & memsetting.
+ */
task_lock(tsk);
+ spin_lock_irqsave(&tsk->comm_lock, flags);
/*
* Threads may access current->comm without holding
* the task lock, so write the string carefully.
@@ -1018,6 +1029,8 @@ void set_task_comm(struct task_struct *tsk, char *buf)
memset(tsk->comm, 0, TASK_COMM_LEN);
wmb();
strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+ spin_unlock_irqrestore(&tsk->comm_lock, flags);
+
task_unlock(tsk);
perf_event_comm(tsk);
}
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index caa151f..b69d94b 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -161,6 +161,7 @@ extern struct cred init_cred;
.group_leader = &tsk, \
RCU_INIT_POINTER(.real_cred, &init_cred), \
RCU_INIT_POINTER(.cred, &init_cred), \
+ .comm_lock = __SPIN_LOCK_UNLOCKED(tsk.comm_lock), \
.comm = "swapper", \
.thread = INIT_THREAD, \
.fs = &init_fs, \
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 18d63ce..f8a7cdf 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1333,10 +1333,9 @@ struct task_struct {
const struct cred __rcu *cred; /* effective (overridable) subjective task
* credentials (COW) */
struct cred *replacement_session_keyring; /* for KEYCTL_SESSION_TO_PARENT */
-
+ spinlock_t comm_lock; /* protect's comm */
char comm[TASK_COMM_LEN]; /* executable name excluding path
- - access with [gs]et_task_comm (which lock
- it with task_lock())
+ - access with [gs]et_task_comm
- initialized normally by setup_new_exec */
/* file system info */
int link_count, total_link_count;
diff --git a/kernel/fork.c b/kernel/fork.c
index e7548de..f53bf29 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1080,6 +1080,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
rcu_copy_process(p);
p->vfork_done = NULL;
spin_lock_init(&p->alloc_lock);
+ spin_lock_init(&p->comm_lock);
init_sigpending(&p->pending);
--
1.7.3.2.146.gca209
This patch adds __get_task_comm() which returns the task->comm value
without taking the comm_lock. This function may return null or
incomplete comm values, and is only present for performance critical
paths that can handle these pitfalls.
CC: Joe Perches <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Michal Nazarewicz <[email protected]>
CC: Andy Whitcroft <[email protected]>
CC: Jiri Slaby <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: David Rientjes <[email protected]>
CC: Dave Hansen <[email protected]>
CC: Andrew Morton <[email protected]>
CC: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
fs/exec.c | 13 +++++++++++++
include/linux/sched.h | 1 +
2 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 34fa611..7e79c97 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -996,6 +996,19 @@ static void flush_old_files(struct files_struct * files)
spin_unlock(&files->file_lock);
}
+/**
+ * __get_task_comm - Unlocked accessor to task comm value
+ *
+ * This function returns the task->comm value without
+ * taking the comm_lock. This method is only for performance
+ * critical paths, and may return a null or incomplete comm
+ * value.
+ */
+char *__get_task_comm(struct task_struct *tsk)
+{
+ return tsk->comm;
+}
+
char *get_task_comm(char *buf, struct task_struct *tsk)
{
unsigned long flags;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index f8a7cdf..5e3c25a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2189,6 +2189,7 @@ struct task_struct *fork_idle(int);
extern void set_task_comm(struct task_struct *tsk, char *from);
extern char *get_task_comm(char *to, struct task_struct *tsk);
+extern char *__get_task_comm(struct task_struct *tsk);
#ifdef CONFIG_SMP
extern unsigned long wait_task_inactive(struct task_struct *, long match_state);
--
1.7.3.2.146.gca209
Accessing task->comm requires proper locking. However in the past
access to current->comm could be done without locking. This
is no longer the case, so all comm access needs to be done
while holding the comm_lock.
In my attempt to clean up unprotected comm access, I've noticed
most comm access is done for printk output. To simplify correct
locking in these cases, I've introduced a new %ptc format,
which will print the corresponding task's comm.
Example use:
printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
CC: Joe Perches <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Michal Nazarewicz <[email protected]>
CC: Andy Whitcroft <[email protected]>
CC: Jiri Slaby <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: David Rientjes <[email protected]>
CC: Dave Hansen <[email protected]>
CC: Andrew Morton <[email protected]>
CC: [email protected]
Reviewed-by: KOSAKI Motohiro <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
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
Now that accessing current->comm needs to be protected,
avoid new current->comm or other task->comm usage by adding
a warning to checkpatch.pl.
Fair warning: I know zero perl, so this was written in the
style of "monkey see, monkey do". It does appear to work
in my testing though.
Thanks to Jiri Slaby, Michal Nazarewicz and Joe Perches
for help improving the regex!
Close review and feedback would be appreciated.
CC: Joe Perches <[email protected]>
CC: Michal Nazarewicz <[email protected]>
CC: Andy Whitcroft <[email protected]>
CC: Jiri Slaby <[email protected]>
CC: KOSAKI Motohiro <[email protected]>
CC: David Rientjes <[email protected]>
CC: Dave Hansen <[email protected]>
CC: Andrew Morton <[email protected]>
CC: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
scripts/checkpatch.pl | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d867081..a16ded7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2868,6 +2868,12 @@ 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
+ my $comm_vars = qr/current|tsk|p|task|curr|t|me/;
+ if ($line =~ /\b$comm_vars\s*->\s*comm\b/) {
+ WARN("task 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
> 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.
> + */
If we provide __get_task_comm(), we can't remove memset() forever.
> task_lock(tsk);
> + spin_lock_irqsave(&tsk->comm_lock, flags);
This is strange order. task_lock() doesn't disable interrupt.
And, can you please document why we need interrupt disabling?
> /*
> * Threads may access current->comm without holding
> * the task lock, so write the string carefully.
On Wed, 2011-05-18 at 11:01 +0900, KOSAKI Motohiro wrote:
> > 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.
> > + */
>
> If we provide __get_task_comm(), we can't remove memset() forever.
True enough. I'll fix that comment up then.
>
> > task_lock(tsk);
> > + spin_lock_irqsave(&tsk->comm_lock, flags);
>
> This is strange order. task_lock() doesn't disable interrupt.
Strange order? Can you explain why you think that is? Having comm_lock
as an inner-most lock seems quite reasonable, given the limited nature
of what it protects.
> And, can you please document why we need interrupt disabling?
Since we might access current->comm from irq context. Where would you
like this documented? Just there in the code?
thanks
-john
>> If we provide __get_task_comm(), we can't remove memset() forever.
>
> True enough. I'll fix that comment up then.
>
>>
>>> task_lock(tsk);
>>> + spin_lock_irqsave(&tsk->comm_lock, flags);
>>
>> This is strange order. task_lock() doesn't disable interrupt.
>
> Strange order? Can you explain why you think that is? Having comm_lock
> as an inner-most lock seems quite reasonable, given the limited nature
> of what it protects.
spinlock -> irq_disable is wrong order.
local_irq_save()
task_lock()
spin_lock(task->comm)
is better. I think.
I mean if the task get interrupt at following point,
task_lock(tsk);
// HERE
spin_lock_irqsave(&tsk->comm_lock, flags);
the task hold task-lock long time rather than expected.
>> And, can you please document why we need interrupt disabling?
>
> Since we might access current->comm from irq context. Where would you
> like this documented? Just there in the code?
I'm prefer code comment. but another way is also good.
Thanks.
* John Stultz <[email protected]> wrote:
> v6 tries to address the latest round of issues. Again, hopefully this is
> getting close to something that can be queued for 2.6.40.
We are far away from thinking about upstreaming any of this ...
> 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.
The commit is from 2.5 years ago:
4614a696bd1c3a9af3a08f0e5874830a85b889d4
Author: john stultz <[email protected]>
Date: Mon Dec 14 18:00:05 2009 -0800
procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm
So we are *way* beyond the time frame where this could be declared urgent.
So is there any actual motivation beyond:
" Hey, this looks a bit racy and 'top' very rarely, on rare workloads that
play with ->comm[], might display a weird reading task name for a second,
amongst the many other temporarily nonsensical statistical things it
already prints every now and then. "
?
> So fair enough, as I opened this can of worms, I should work
> to resolve it and this patchset is my initial attempt.
This patch set does not address the many places that deal with ->comm so it
does not even approximate the true scope of the change!
I.e. you are doing *another* change without fully seeing/showing the
consequences ...
Thanks,
Ingo
On Wed, 18 May 2011 08:25:54 +0200 Ingo Molnar <[email protected]> wrote:
> " Hey, this looks a bit racy and 'top' very rarely, on rare workloads that
> play with ->comm[], might display a weird reading task name for a second,
> amongst the many other temporarily nonsensical statistical things it
> already prints every now and then. "
Well we should at least make sure that `top' won't run off the end of
comm[] and go oops. I think that's guaranteed by the fact(s) that
init_tasks's comm[15] is zero and is always copied-by-value across
fork and can never be overwritten in any task_struct.
But I didn't check that.
* Andrew Morton <[email protected]> wrote:
> On Wed, 18 May 2011 08:25:54 +0200 Ingo Molnar <[email protected]> wrote:
>
> > " Hey, this looks a bit racy and 'top' very rarely, on rare workloads that
> > play with ->comm[], might display a weird reading task name for a second,
> > amongst the many other temporarily nonsensical statistical things it
> > already prints every now and then. "
>
> Well we should at least make sure that `top' won't run off the end of comm[]
> and go oops. I think that's guaranteed by the fact(s) that init_tasks's
> comm[15] is zero and is always copied-by-value across fork and can never be
> overwritten in any task_struct.
Correct.
> But I didn't check that.
I actually have a highly threaded app that uses PR_SET_NAME heavily and would
have noticed any oopsing potential long ago.
Since ->comm is often observed from other tasks, regardless whether it's set
from the prctl() or from the newfangled /proc vector, the race for seeing
partial updates to ->comm always existed - for more than 10 years.
So the premise of the whole series is wrong: temporarily incomplete ->comm[]s
were *always* possible and did not start 1.5+ years ago with:
4614a696bd1c: procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm
when i see series being built on a fundamentally wrong premise i get a bit sad!
Thanks,
Ingo
On Wed, 2011-05-18 at 08:25 +0200, Ingo Molnar wrote:
> * John Stultz <[email protected]> wrote:
> > 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.
>
> The commit is from 2.5 years ago:
> 4614a696bd1c3a9af3a08f0e5874830a85b889d4
> Author: john stultz <[email protected]>
> Date: Mon Dec 14 18:00:05 2009 -0800
>
> procfs: allow threads to rename siblings via /proc/pid/tasks/tid/comm
>
> So we are *way* beyond the time frame where this could be declared urgent.
Oh yes. I'm not declaring it urgent. I'm just trying to get the
groundwork in so the "cleanup" can happen over time.
> So is there any actual motivation beyond:
>
> " Hey, this looks a bit racy and 'top' very rarely, on rare workloads that
> play with ->comm[], might display a weird reading task name for a second,
> amongst the many other temporarily nonsensical statistical things it
> already prints every now and then. "
>
> ?
To my knowledge no. Basically folks were grumbling about the issue, and
so being that I opened the issue up, I figured I'd try to address their
concerns. While specific examples of the problem were not raised
(despite asking for them), I figured a good faith attempt at providing a
path to proper locking for comm was a more productive step then getting
into "prove its safe" / "no, you prove its unsafe" type debates.
My motivation here is just to try to do the right thing and move on to
other work, and that is maybe why I seem hurried to get the patches
queued.
The other reasonable argument in my mind would be: Even if there is no
existing issue, comm locking rules are subtle and by formalizing them
we avoid future problems being introduced (probably by folks like me :).
> > So fair enough, as I opened this can of worms, I should work
> > to resolve it and this patchset is my initial attempt.
>
> This patch set does not address the many places that deal with ->comm so it
> does not even approximate the true scope of the change!
>
> I.e. you are doing *another* change without fully seeing/showing the
> consequences ...
Well, is requiring all the comm changes in one patch set really
reasonable? I'm aware its a large scope of changes. It touches
everything and will take quite a while to in order to get all of the
changes pushed through the various relevant maintainers. A path for
gradual "improvement" seems like the only reasonable approach.
Or do you have another suggestion in mind?
But given that I'm providing both locked and unlocked accessors, doesn't
it seem that by working through the tree converting to those accessors
would help actually audit the users, so we can address that each call
site can either deal with the locking or handle incomplete comms?
Further, if other locking approaches (such as RCU) are to be tried, it
would greatly ease doing so, since the access is centralized to a few
functions.
So is such a change not somewhat worthwhile?
But, the net of this is that it seems everyone else is way more
passionate about this issue then I am, so I'm starting to wonder if it
would be better for someone who has more of a dog in the fight to be
pushing these?
thanks
-john
On Wed, 18 May 2011 12:03:29 -0700
John Stultz <[email protected]> wrote:
> But, the net of this is that it seems everyone else is way more
> passionate about this issue then I am, so I'm starting to wonder if it
> would be better for someone who has more of a dog in the fight to be
> pushing these?
I like the %p thingy - it's neat and is an overall improvement. If it
dies I shall stick another pin in my Ingo doll.
Providing an unlocked accessor for super-special applications which
know what they're doing seems an adequate compromise.
(Linus Cc:-ed)
* Andrew Morton <[email protected]> wrote:
> On Wed, 18 May 2011 12:03:29 -0700
> John Stultz <[email protected]> wrote:
>
> > But, the net of this is that it seems everyone else is way more passionate
> > about this issue then I am, so I'm starting to wonder if it would be better
> > for someone who has more of a dog in the fight to be pushing these?
>
> I like the %p thingy - it's neat and is an overall improvement.
> [...]
>
> Providing an unlocked accessor for super-special applications which know what
> they're doing seems an adequate compromise.
Dunno, %ptc ties into lowlevel sprintf() and takes a spinlock! We are
unrobustizing an important lowlevel function that until today could always be
used lockless for debugging, in any context, under any circumstance.
We do that just to solve something that occurs rather rarely and has no
functional effect just some temporarily confusing looking string descriptor
output.
The *last* place i'd put this into is vsprintf(), really. Make the procfs
output methods atomic against ->comm update, sure. But put a lock like that
into kernel debug output? No way!
(Btw, i find %ptc OK if it comes with no lock. %pt would be nicer as a name?)
I'm uneasy about it if i think how many hairy places handle task->comm[].
Anyway, vsprintf() is Linus code, so i can take the easy road, chicken out and
punt this to Linus - instead of risking a needle from Andrew! :)
If Linus likes this approach we should do it with a lock.
> [...] If it dies I shall stick another pin in my Ingo doll.
Oh, out of morbid curiosity, mind providing a log of bigger past incidents
where you had to stick pins into a doll of me? (In private mail, if the list is
too long ;-)
(Does every lockdep report that catches a real bug unpull a needle? ;-)
Thanks,
Ingo
On Wed, 18 May 2011 21:48:11 +0200
Ingo Molnar <[email protected]> wrote:
> Dunno, %ptc ties into lowlevel sprintf() and takes a spinlock!
yup, that's a problem.
> Oh, out of morbid curiosity, mind providing a log of bigger past incidents
> where you had to stick pins into a doll of me? (In private mail, if the list is
> too long ;-)
http://0.tqn.com/d/urbanlegends/1/0/m/B/porcupine2.jpg
On Tue, May 17, 2011 at 6:41 PM, John Stultz <[email protected]> wrote:
>
> 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.
What folks?
I don't think a new lock (or any lock) is at all appropriate.
There's just no point. Just guarantee that the last byte is always
zero, and you're done.
If you just guarantee that, THERE IS NO RACE. The last byte never
changes. You may get odd half-way strings, but you've trivially
guaranteed that they are C NUL-terminated, with no locking, no memory
ordering, no nothing.
Anybody who asks for any locking is just being a silly git. Tell them
to man the f*ck up.
So I'm not going to apply anything like this for 2.6.39, but I'm also
not going to apply it for 40 or 41 or anything else.
I refuse to accept just stupid unnecessary crap.
Linus
* Andrew Morton <[email protected]> wrote:
> On Wed, 18 May 2011 21:48:11 +0200
> Ingo Molnar <[email protected]> wrote:
>
> > Dunno, %ptc ties into lowlevel sprintf() and takes a spinlock!
>
> yup, that's a problem.
If the lock is removed it looks useful.
> > Oh, out of morbid curiosity, mind providing a log of bigger past incidents
> > where you had to stick pins into a doll of me? (In private mail, if the list is
> > too long ;-)
>
> http://0.tqn.com/d/urbanlegends/1/0/m/B/porcupine2.jpg
That looks manageable! I was hoping not to end up like this:
http://i.telegraph.co.uk/multimedia/archive/00675/china_needle404_675809c.jpg
Thanks,
Ingo
(2011/05/19 4:58), Linus Torvalds wrote:
> On Tue, May 17, 2011 at 6:41 PM, John Stultz<[email protected]> wrote:
>>
>> 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.
>
> What folks?
>
> I don't think a new lock (or any lock) is at all appropriate.
>
> There's just no point. Just guarantee that the last byte is always
> zero, and you're done.
>
> If you just guarantee that, THERE IS NO RACE. The last byte never
> changes. You may get odd half-way strings, but you've trivially
> guaranteed that they are C NUL-terminated, with no locking, no memory
> ordering, no nothing.
>
> Anybody who asks for any locking is just being a silly git. Tell them
> to man the f*ck up.
>
> So I'm not going to apply anything like this for 2.6.39, but I'm also
> not going to apply it for 40 or 41 or anything else.
>
> I refuse to accept just stupid unnecessary crap.
Do every body agree this conclusion? If so, I'd like to propose
documentation update patch. Because I recently observed Dave Hansen
and David Rientjes discussed task->comm locking rule. So, I guess
current code comments is misleading. It doesn't describe why almost
all task->comm user don't take task_lock() at all.
What do you think?
From e96571a8d470156d6ab7f3656d938aab126f17e8 Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <[email protected]>
Date: Fri, 20 May 2011 19:26:12 +0900
Subject: [PATCH] add comments for task->comm locking rule
Now, sched.h says, we should use [gs]et_task_comm for task->comm
access. but almost all actual code don't take task_lock(). It
brought repeated almost same locking rule discussion. Probably
we have to write exact current locking rule explicitly.
Signed-off-by: KOSAKI Motohiro <[email protected]>
Cc: John Stultz <[email protected]>
Cc: Dave Hansen <[email protected]>,
---
fs/exec.c | 19 ++++++++++++++++++-
include/linux/sched.h | 5 ++---
2 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 3d48ac6..bce64bb 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -995,9 +995,26 @@ static void flush_old_files(struct files_struct * files)
spin_unlock(&files->file_lock);
}
+/**
+ * get_task_comm - get task name
+ * @buf: buffer to store result. must be at least sizeof(tsk->comm) in size
+ * @tsk: the task in question
+ *
+ * Note: task->comm has slightly complex locking rule.
+ *
+ * 1) write own or another task's name
+ * -> must use set_task_comm()
+ * 2) read another task's name
+ * -> must use get_task_comm() or take task_lock() manually.
+ * 3) read own task's name
+ * -> recommend to use get_task_comm() or take task_lock() manually.
+ * If you don't take task_lock(), you may see incomplete or empty string.
+ * But it's guaranteed to keep valid C NUL-terminated string.
+ * (ie never be crash)
+ * So, debugging printk may be ok to read it without lock.
+ */
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);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 275c1a1..3e86500 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1334,9 +1334,8 @@ struct task_struct {
struct cred *replacement_session_keyring; /* for KEYCTL_SESSION_TO_PARENT */
char comm[TASK_COMM_LEN]; /* executable name excluding path
- - access with [gs]et_task_comm (which lock
- it with task_lock())
- - initialized normally by setup_new_exec */
+ detailed locking rule is described at
+ get_task_comm() */
/* file system info */
int link_count, total_link_count;
#ifdef CONFIG_SYSVIPC
--
1.7.3.1
