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.
In addition, with this new patch set I've introduced a printk
%ptc accessor, which makes the conversion to locked access
simpler (as most uses are for printks).
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 an example how I've converted
comm access in ext4 to use %ptc method. 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: Ted Ts'o <[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 (3):
comm: Introduce comm_lock seqlock to protect task->comm access
printk: Add %ptc to safely print a task's comm
comm: ext4: Protect task->comm access by using get_task_comm()
fs/exec.c | 25 ++++++++++++++++++++-----
fs/ext4/file.c | 4 ++--
fs/ext4/super.c | 8 ++++----
include/linux/init_task.h | 1 +
include/linux/sched.h | 5 ++---
lib/vsprintf.c | 27 +++++++++++++++++++++++++++
6 files changed, 56 insertions(+), 14 deletions(-)
--
1.7.3.2.146.gca209
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 <[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 | 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
Acessing task->comm requires proper locking. However in the past
access to current->comm could be done without locking. This
is no longer the case, so all comm access needs to be done
while holding the comm_lock.
In my attempt to clean up unprotected comm access, I've noticed
most comm access is done for printk output. To simpify correct
locking in these cases, I've introduced a new %ptc format,
which will safely print the corresponding task's comm.
Example use:
printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
CC: Ted Ts'o <[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]>
---
lib/vsprintf.c | 27 +++++++++++++++++++++++++++
1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index bc0ac6b..b9c97b8 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -797,6 +797,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
return string(buf, end, uuid, spec);
}
+static noinline_for_stack
+char *task_comm_string(char *buf, char *end, u8 *addr,
+ struct printf_spec spec, const char *fmt)
+{
+ struct task_struct *tsk = (struct task_struct *) addr;
+ char *ret;
+ unsigned long seq;
+
+ do {
+ seq = read_seqbegin(&tsk->comm_lock);
+
+ ret = string(buf, end, tsk->comm, spec);
+
+ } while (read_seqretry(&tsk->comm_lock, seq));
+
+ return ret;
+}
+
+
+
int kptr_restrict = 1;
/*
@@ -864,6 +884,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
}
switch (*fmt) {
+ case 't':
+ switch (fmt[1]) {
+ case 'c':
+ return task_comm_string(buf, end, ptr, spec, fmt);
+ }
+ break;
case 'F':
case 'f':
ptr = dereference_function_descriptor(ptr);
@@ -1151,6 +1177,7 @@ qualifier:
* http://tools.ietf.org/html/draft-ietf-6man-text-addr-representation-00
* %pU[bBlL] print a UUID/GUID in big or little endian using lower or upper
* case.
+ * %ptc outputs the task's comm name
* %n is ignored
*
* The return value is the number of characters which would
--
1.7.3.2.146.gca209
Converts ext4 comm access to use the safe printk %ptc accessor.
CC: Ted Ts'o <[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/ext4/file.c | 4 ++--
fs/ext4/super.c | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 7b80d54..31438a0 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -126,9 +126,9 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
/* Warn about this once per day */
if (printk_timed_ratelimit(&unaligned_warn_time, 60*60*24*HZ))
ext4_msg(inode->i_sb, KERN_WARNING,
- "Unaligned AIO/DIO on inode %ld by %s; "
+ "Unaligned AIO/DIO on inode %ld by %ptc; "
"performance will be poor.",
- inode->i_ino, current->comm);
+ inode->i_ino, current);
mutex_lock(ext4_aio_mutex(inode));
ext4_aiodio_wait(inode);
}
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8553dfb..d4ab4c0 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -413,8 +413,8 @@ void __ext4_error(struct super_block *sb, const char *function,
va_start(args, fmt);
vaf.fmt = fmt;
vaf.va = &args;
- printk(KERN_CRIT "EXT4-fs error (device %s): %s:%d: comm %s: %pV\n",
- sb->s_id, function, line, current->comm, &vaf);
+ printk(KERN_CRIT "EXT4-fs error (device %s): %s:%d: comm %ptc: %pV\n",
+ sb->s_id, function, line, current, &vaf);
va_end(args);
ext4_handle_error(sb);
@@ -438,7 +438,7 @@ 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);
+ printk(KERN_CONT "comm %ptc: %pV\n", current, &vaf);
va_end(args);
ext4_handle_error(inode->i_sb);
@@ -468,7 +468,7 @@ 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);
+ printk(KERN_CONT "comm %ptc: path %s: %pV\n", current, path, &vaf);
va_end(args);
ext4_handle_error(inode->i_sb);
--
1.7.3.2.146.gca209
On Tue, 2011-05-10 at 17:23 -0700, John Stultz wrote:
> Acessing task->comm requires proper locking. However in the past
> access to current->comm could be done without locking. This
> is no longer the case, so all comm access needs to be done
> while holding the comm_lock.
>
> In my attempt to clean up unprotected comm access, I've noticed
> most comm access is done for printk output. To simpify correct
> locking in these cases, I've introduced a new %ptc format,
> which will safely print the corresponding task's comm.
Hi John.
Couple of tyops for Accessing and simplify in your commit message
and a few comments on the patch.
Could misuse of %ptc (not using current) cause system lockup?
> Example use:
> printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index bc0ac6b..b9c97b8 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -797,6 +797,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
> return string(buf, end, uuid, spec);
> }
>
> +static noinline_for_stack
> +char *task_comm_string(char *buf, char *end, u8 *addr,
> + struct printf_spec spec, const char *fmt)
addr should be void * not u8 *
> +{
> + struct task_struct *tsk = (struct task_struct *) addr;
no cast.
Maybe it'd be better to use current inside this routine and not
pass the pointer at all.
static noinline_for_stack
char *task_comm_string(char *buf, char *end,
struct printf_spec spec, const char *fmt)
> + char *ret;
> + unsigned long seq;
> +
> + do {
> + seq = read_seqbegin(&tsk->comm_lock);
> +
> + ret = string(buf, end, tsk->comm, spec);
> +
> + } while (read_seqretry(&tsk->comm_lock, seq));
> @@ -864,6 +884,12 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
> }
>
> switch (*fmt) {
> + case 't':
> + switch (fmt[1]) {
> + case 'c':
> + return task_comm_string(buf, end, ptr, spec, fmt);
maybe
return task_comm_string(buf, end, spec, fmt);
On Tue, 2011-05-10 at 17:51 -0700, Joe Perches wrote:
> On Tue, 2011-05-10 at 17:23 -0700, John Stultz wrote:
> > Acessing task->comm requires proper locking. However in the past
> > access to current->comm could be done without locking. This
> > is no longer the case, so all comm access needs to be done
> > while holding the comm_lock.
> >
> > In my attempt to clean up unprotected comm access, I've noticed
> > most comm access is done for printk output. To simpify correct
> > locking in these cases, I've introduced a new %ptc format,
> > which will safely print the corresponding task's comm.
>
> Hi John.
>
> Couple of tyops for Accessing and simplify in your commit message
> and a few comments on the patch.
Ah. Yes. Thanks!
> Could misuse of %ptc (not using current) cause system lockup?
It very well could. Although I don't see other %p options tring to
handle invalid pointers. Any suggestions on how to best handle this?
> > Example use:
> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
>
>
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index bc0ac6b..b9c97b8 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -797,6 +797,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
> > return string(buf, end, uuid, spec);
> > }
> >
> > +static noinline_for_stack
> > +char *task_comm_string(char *buf, char *end, u8 *addr,
> > + struct printf_spec spec, const char *fmt)
>
> addr should be void * not u8 *
>
> > +{
> > + struct task_struct *tsk = (struct task_struct *) addr;
>
> no cast.
>
> Maybe it'd be better to use current inside this routine and not
> pass the pointer at all.
That sounds reasonable. Most users are current, so forcing the more rare
non-current users to copy it to a buffer first and use the normal %s
would not be of much impact.
Although I'm not sure if there's precedent for a %p value that didn't
take a argument. Thoughts on that? Anyone else have an opinion here?
Thanks so much for the review and feedback!
-john
On Tue, 2011-05-10 at 18:10 -0700, John Stultz wrote:
> On Tue, 2011-05-10 at 17:51 -0700, Joe Perches wrote:
> > Could misuse of %ptc (not using current) cause system lockup?
>
> It very well could. Although I don't see other %p options tring to
> handle invalid pointers. Any suggestions on how to best handle this?
And just to clarify on this point, I'm responding to if a invalid
pointer was provided, causing the dereference to go awry.
If a valid non-current task was provided, the locking should be ok as we
disable irqs while the write_seqlock is held in set_task_comm().
The only places this could cause a problem was if you tried to printk
with a %ptc while holding the task->comm_lock. However, the lock is only
shortly held in task_comm_string, and get_task_comm and set_task_comm.
So it is fairly easy to audit for correctness.
If there is some other situation you had in mind, please let me know.
thanks
-john
On Tue, 2011-05-10 at 18:10 -0700, John Stultz wrote:
> On Tue, 2011-05-10 at 17:51 -0700, Joe Perches wrote:
> > On Tue, 2011-05-10 at 17:23 -0700, John Stultz wrote:
> > > Acessing task->comm requires proper locking. However in the past
> > > access to current->comm could be done without locking. This
> > > is no longer the case, so all comm access needs to be done
> > > while holding the comm_lock.
> > Could misuse of %ptc (not using current) cause system lockup?
> It very well could. Although I don't see other %p options tring to
> handle invalid pointers. Any suggestions on how to best handle this?
The only one I know of is ipv6 which copies a 16 byte buffer
in case the pointed to value is unaligned. I suppose %pI6c
could be a problem or maybe %pS too, but it hasn't been in
practice. The use of %ptc somehow seemed more error prone.
> Most users are current, so forcing the more rare
> non-current users to copy it to a buffer first and use the normal %s
> would not be of much impact.
>
> Although I'm not sure if there's precedent for a %p value that didn't
> take a argument. Thoughts on that? Anyone else have an opinion here?
The uses of %ptc must add an argument or else gcc will complain.
I suggest you just ignore the argument value and use current.
On Wed, May 11, 2011 at 8:23 AM, John Stultz <[email protected]> wrote:
> Acessing task->comm requires proper locking. However in the past
> access to current->comm could be done without locking. This
> is no longer the case, so all comm access needs to be done
> while holding the comm_lock.
>
> In my attempt to clean up unprotected comm access, I've noticed
> most comm access is done for printk output. To simpify correct
> locking in these cases, I've introduced a new %ptc format,
> which will safely print the corresponding task's comm.
>
> Example use:
> printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
>
Why do you hide current->comm behide printk?
How is this better than printk("%s: ....", task_comm(current)) ?
John Stultz <[email protected]> writes:
> Acessing task->comm requires proper locking. However in the past
> access to current->comm could be done without locking. This
> is no longer the case, so all comm access needs to be done
> while holding the comm_lock.
>
> In my attempt to clean up unprotected comm access, I've noticed
> most comm access is done for printk output. To simpify correct
> locking in these cases, I've introduced a new %ptc format,
> which will safely print the corresponding task's comm.
>
> Example use:
> printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
Neat. But you probably want a checkpatch rule for this too
to catch new offenders.
-Andi
--
[email protected] -- Speaking for myself only
John Stultz <[email protected]> writes:
>
> 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.
... and after that rename the field.
-Andi
-Andi
--
[email protected] -- Speaking for myself only
On Wed, 2011-05-11 at 17:33 +0800, Américo Wang wrote:
> On Wed, May 11, 2011 at 8:23 AM, John Stultz <[email protected]> wrote:
> > Acessing task->comm requires proper locking. However in the past
> > access to current->comm could be done without locking. This
> > is no longer the case, so all comm access needs to be done
> > while holding the comm_lock.
> >
> > In my attempt to clean up unprotected comm access, I've noticed
> > most comm access is done for printk output. To simpify correct
> > locking in these cases, I've introduced a new %ptc format,
> > which will safely print the corresponding task's comm.
> >
> > Example use:
> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> >
>
> Why do you hide current->comm behide printk?
> How is this better than printk("%s: ....", task_comm(current)) ?
So to properly access current->comm, you need to hold the task-lock (or
with my new patch set, the comm_lock). Rather then adding locking to all
the call sites that printk("%s ...", current->comm), I'm suggesting we
add a new %ptc method which will handle the locking for you.
thanks
-john
On Wed, 2011-05-11 at 10:36 -0700, Andi Kleen wrote:
> John Stultz <[email protected]> writes:
>
> > Acessing task->comm requires proper locking. However in the past
> > access to current->comm could be done without locking. This
> > is no longer the case, so all comm access needs to be done
> > while holding the comm_lock.
> >
> > In my attempt to clean up unprotected comm access, I've noticed
> > most comm access is done for printk output. To simpify correct
> > locking in these cases, I've introduced a new %ptc format,
> > which will safely print the corresponding task's comm.
> >
> > Example use:
> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
>
> Neat. But you probably want a checkpatch rule for this too
> to catch new offenders.
Yea. That's on my queue.
thanks
-john
On Thu, May 12, 2011 at 5:02 AM, John Stultz <[email protected]> wrote:
> On Wed, 2011-05-11 at 17:33 +0800, Américo Wang wrote:
>> On Wed, May 11, 2011 at 8:23 AM, John Stultz <[email protected]> wrote:
>> > Acessing task->comm requires proper locking. However in the past
>> > access to current->comm could be done without locking. This
>> > is no longer the case, so all comm access needs to be done
>> > while holding the comm_lock.
>> >
>> > In my attempt to clean up unprotected comm access, I've noticed
>> > most comm access is done for printk output. To simpify correct
>> > locking in these cases, I've introduced a new %ptc format,
>> > which will safely print the corresponding task's comm.
>> >
>> > Example use:
>> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
>> >
>>
>> Why do you hide current->comm behide printk?
>> How is this better than printk("%s: ....", task_comm(current)) ?
>
> So to properly access current->comm, you need to hold the task-lock (or
> with my new patch set, the comm_lock). Rather then adding locking to all
> the call sites that printk("%s ...", current->comm), I'm suggesting we
> add a new %ptc method which will handle the locking for you.
>
Sorry, I meant why not adding the locking into a wrapper function,
probably get_task_comm() and let the users to call it directly?
Why is %ptc better than
char comm[...];
get_task_comm(comm, current);
printk("%s: ....", comm);
?
On Thu, May 12, 2011 at 6:43 PM, Américo Wang <[email protected]> wrote:
> On Thu, May 12, 2011 at 5:02 AM, John Stultz <[email protected]> wrote:
>> On Wed, 2011-05-11 at 17:33 +0800, Américo Wang wrote:
>>> On Wed, May 11, 2011 at 8:23 AM, John Stultz <[email protected]> wrote:
>>> > Acessing task->comm requires proper locking. However in the past
>>> > access to current->comm could be done without locking. This
>>> > is no longer the case, so all comm access needs to be done
>>> > while holding the comm_lock.
>>> >
>>> > In my attempt to clean up unprotected comm access, I've noticed
>>> > most comm access is done for printk output. To simpify correct
>>> > locking in these cases, I've introduced a new %ptc format,
>>> > which will safely print the corresponding task's comm.
>>> >
>>> > Example use:
>>> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
>>> >
>>>
>>> Why do you hide current->comm behide printk?
>>> How is this better than printk("%s: ....", task_comm(current)) ?
>>
>> So to properly access current->comm, you need to hold the task-lock (or
>> with my new patch set, the comm_lock). Rather then adding locking to all
>> the call sites that printk("%s ...", current->comm), I'm suggesting we
>> add a new %ptc method which will handle the locking for you.
>>
>
> Sorry, I meant why not adding the locking into a wrapper function,
> probably get_task_comm() and let the users to call it directly?
>
Ahhh, never mind, I see the points now... Then it is fine. :)
On Thu, 2011-05-12 at 18:43 +0800, Américo Wang wrote:
> On Thu, May 12, 2011 at 5:02 AM, John Stultz <[email protected]> wrote:
> > On Wed, 2011-05-11 at 17:33 +0800, Américo Wang wrote:
> >> On Wed, May 11, 2011 at 8:23 AM, John Stultz <[email protected]> wrote:
> >> > Acessing task->comm requires proper locking. However in the past
> >> > access to current->comm could be done without locking. This
> >> > is no longer the case, so all comm access needs to be done
> >> > while holding the comm_lock.
> >> >
> >> > In my attempt to clean up unprotected comm access, I've noticed
> >> > most comm access is done for printk output. To simpify correct
> >> > locking in these cases, I've introduced a new %ptc format,
> >> > which will safely print the corresponding task's comm.
> >> >
> >> > Example use:
> >> > printk("%ptc: unaligned epc - sending SIGBUS.\n", current);
> >> >
> >>
> >> Why do you hide current->comm behide printk?
> >> How is this better than printk("%s: ....", task_comm(current)) ?
> >
> > So to properly access current->comm, you need to hold the task-lock (or
> > with my new patch set, the comm_lock). Rather then adding locking to all
> > the call sites that printk("%s ...", current->comm), I'm suggesting we
> > add a new %ptc method which will handle the locking for you.
> >
>
> Sorry, I meant why not adding the locking into a wrapper function,
> probably get_task_comm() and let the users to call it directly?
>
> Why is %ptc better than
>
> char comm[...];
> get_task_comm(comm, current);
> printk("%s: ....", comm);
There were concerns about the extra stack usage caused adding a comm
buffer to each location, which can be avoided by adding the
functionality to printk.
Further it reduces the amount of change necessary to correct invalid
usage.
thanks
-john
On Tue, 10 May 2011, John Stultz wrote:
> The implicit rules for current->comm access being safe without locking
> are no longer true. Accessing current->comm without holding the task
> lock may result in null or incomplete strings (however, access won't
> run off the end of the string).
>
> In order to properly fix this, I've introduced a comm_lock 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 <[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]>
Acked-by: David Rientjes <[email protected]>
On Tue, 10 May 2011, John Stultz wrote:
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > > index bc0ac6b..b9c97b8 100644
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -797,6 +797,26 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
> > > return string(buf, end, uuid, spec);
> > > }
> > >
> > > +static noinline_for_stack
> > > +char *task_comm_string(char *buf, char *end, u8 *addr,
> > > + struct printf_spec spec, const char *fmt)
> >
> > addr should be void * not u8 *
> >
> > > +{
> > > + struct task_struct *tsk = (struct task_struct *) addr;
> >
> > no cast.
> >
> > Maybe it'd be better to use current inside this routine and not
> > pass the pointer at all.
>
> That sounds reasonable. Most users are current, so forcing the more rare
> non-current users to copy it to a buffer first and use the normal %s
> would not be of much impact.
>
Please still require an argument, otherwise the oom killer (which could
potentially called right before a stack overflow) would be required to use
buffers for the commands printed in the tasklist dump.
> Although I'm not sure if there's precedent for a %p value that didn't
> take a argument. Thoughts on that? Anyone else have an opinion here?
>
After the cleanups are addressed:
Acked-by: David Rientjes <[email protected]>
It would have been nice if we could force %ptc to expect a
struct task_struct * rather than a void *, however.
On Tue, 10 May 2011, Joe Perches wrote:
> > Although I'm not sure if there's precedent for a %p value that didn't
> > take a argument. Thoughts on that? Anyone else have an opinion here?
>
> The uses of %ptc must add an argument or else gcc will complain.
> I suggest you just ignore the argument value and use current.
>
That doesn't make any sense, why would you needlessly restrict this to
current when accesses to other threads' ->comm needs to be protected in
the same way? I'd like to use this in the oom killer and try to get rid
of taking task_lock() for every thread group leader in the tasklist dump.
On Tue, 10 May 2011, John Stultz wrote:
> Converts ext4 comm access to use the safe printk %ptc accessor.
>
> CC: Ted Ts'o <[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]>
I like how this patch illustrates how easy it is to use the new method for
printing a task's command, but it would probably be easier to get the
first two patches in the series (those that add the seqlock and then %ptc)
merged in mainline and then break out a series of conversions such as this
that could go through the individual maintainer's trees.
On Thu, 2011-05-12 at 15:12 -0700, David Rientjes wrote:
> On Tue, 10 May 2011, Joe Perches wrote:
> > > Although I'm not sure if there's precedent for a %p value that didn't
> > > take a argument. Thoughts on that? Anyone else have an opinion here?
> > The uses of %ptc must add an argument or else gcc will complain.
> > I suggest you just ignore the argument value and use current.
> That doesn't make any sense, why would you needlessly restrict this to
> current when accesses to other threads' ->comm needs to be protected in
> the same way? I'd like to use this in the oom killer and try to get rid
> of taking task_lock() for every thread group leader in the tasklist dump.
I suppose another view is coder stuffed up, let them suffer...
At some point, gcc may let us extend printf argument type
verification so it may not be a continuing problem.
Adding a checkpatch rule for this is non-trivial as it can
be written as:
printk("%ptc\n",
current);
and checkpatch is mostly line oriented.
Andy, do you have a suggestion on how to verify
vsprintf argument types for checkpatch?
On Thu, 2011-05-12 at 15:14 -0700, David Rientjes wrote:
> On Tue, 10 May 2011, John Stultz wrote:
>
> > Converts ext4 comm access to use the safe printk %ptc accessor.
> >
> > CC: Ted Ts'o <[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]>
>
> I like how this patch illustrates how easy it is to use the new method for
> printing a task's command, but it would probably be easier to get the
> first two patches in the series (those that add the seqlock and then %ptc)
> merged in mainline and then break out a series of conversions such as this
> that could go through the individual maintainer's trees.
Agreed. I just wanted to show how it would be used compared to the
earlier approach.
I'll respin the first two patches shortly here. I also need to get the
checkpatch bit done.
Andrew, should these go upstream through you?
thanks
-john
On Thu, 12 May 2011 15:29:40 -0700
John Stultz <[email protected]> wrote:
> On Thu, 2011-05-12 at 15:14 -0700, David Rientjes wrote:
> > On Tue, 10 May 2011, John Stultz wrote:
> >
> > > Converts ext4 comm access to use the safe printk %ptc accessor.
> > >
> > > CC: Ted Ts'o <[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]>
> >
> > I like how this patch illustrates how easy it is to use the new method for
> > printing a task's command, but it would probably be easier to get the
> > first two patches in the series (those that add the seqlock and then %ptc)
> > merged in mainline and then break out a series of conversions such as this
> > that could go through the individual maintainer's trees.
>
> Agreed. I just wanted to show how it would be used compared to the
> earlier approach.
>
> I'll respin the first two patches shortly here. I also need to get the
> checkpatch bit done.
>
> Andrew, should these go upstream through you?
>
That works. I have a little pile of task->comm patches here, but I
expect that resolving everything will be pretty straightforward.
Don't forget the checkpatch patch :)
On Thu, 12 May 2011, Joe Perches wrote:
> > > > Although I'm not sure if there's precedent for a %p value that didn't
> > > > take a argument. Thoughts on that? Anyone else have an opinion here?
> > > The uses of %ptc must add an argument or else gcc will complain.
> > > I suggest you just ignore the argument value and use current.
> > That doesn't make any sense, why would you needlessly restrict this to
> > current when accesses to other threads' ->comm needs to be protected in
> > the same way? I'd like to use this in the oom killer and try to get rid
> > of taking task_lock() for every thread group leader in the tasklist dump.
>
> I suppose another view is coder stuffed up, let them suffer...
>
> At some point, gcc may let us extend printf argument type
> verification so it may not be a continuing problem.
>
I don't understand your respose, could you answer my question? Printing
the command of threads other than current isn't special.