[PATCH] lib/vsprintf: add %pT format specifier

[PATCH] lib/vsprintf: add %pT format specifier

[Tetsuo Handa]

Hello.

Since addition of %pT itself seems to be agreed, I refreshed this patch using
linux-next-20140109. Please pick up if this patch is OK for you; I will start
making patches for killing most of direct ->comm readers.

Regards.
----------------------------------------
>From 0d1f03d59a477459f3d3c190593d9e78f5d67de8 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Thu, 9 Jan 2014 21:32:22 +0900
Subject: [PATCH] lib/vsprintf: add %pT format specifier

Since task_struct->comm can be modified by other threads while the current
thread is reading it, it is recommended to use get_task_comm() for reading it.

However, since get_task_comm() holds task_struct->alloc_lock spinlock,
some users cannot use get_task_comm(). Also, a lot of users are directly
reading from task_struct->comm even if they can use get_task_comm().
Such users might obtain inconsistent result.

This patch introduces %pT format specifier for printing task_struct->comm.
Currently %pT does not provide consistency. I'm planning to change to use RCU
in the future. By using RCU, the comm name read from task_struct->comm will be
guaranteed to be consistent. But before modifying set_task_comm() to use RCU,
we need to kill direct ->comm users who do not use get_task_comm().

An example for converting direct ->comm users is shown below. Since many debug
printings use p == current, you can pass NULL instead of p if p == current.

  pr_info("comm=%s\n", p->comm);       => pr_info("comm=%pT\n", p);
  pr_info("comm=%s\n", current->comm); => pr_info("comm=%pT\n", NULL);

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
---
 Documentation/printk-formats.txt |    6 ++++++
 lib/vsprintf.c                   |   20 +++++++++++++++++++-
 2 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index 6f4eb32..94459b4 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -184,6 +184,12 @@ dentry names:
 	equivalent of %s dentry->d_name.name we used to use, %pd<n> prints
 	n last components.  %pD does the same thing for struct file.
 
+task_struct comm name:
+
+        %pT
+
+        For printing task_struct->comm.
+
 struct va_format:
 
 	%pV
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 185b6d3..a97f18b 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1179,6 +1179,21 @@ char *address_val(char *buf, char *end, const void *addr,
 	return number(buf, end, num, spec);
 }
 
+static noinline_for_stack
+char *comm_name(char *buf, char *end, struct task_struct *tsk,
+		struct printf_spec spec, const char *fmt)
+{
+	char name[TASK_COMM_LEN];
+
+	/* Caller can pass NULL instead of current. */
+	if (!tsk)
+		tsk = current;
+	/* Not using get_task_comm() in case I'm in IRQ context. */
+	memcpy(name, tsk->comm, TASK_COMM_LEN);
+	name[sizeof(name) - 1] = '\0';
+	return string(buf, end, name, spec);
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1246,6 +1261,7 @@ int kptr_restrict __read_mostly;
  *           (default assumed to be phys_addr_t, passed by reference)
  * - 'd[234]' For a dentry name (optionally 2-4 last components)
  * - 'D[234]' Same as 'd' but for a struct file
+ * - 'T' task_struct->comm
  *
  * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
  * function pointers are really function descriptors, which contain a
@@ -1257,7 +1273,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 {
 	int default_width = 2 * sizeof(void *) + (spec.flags & SPECIAL ? 2 : 0);
 
-	if (!ptr && *fmt != 'K') {
+	if (!ptr && *fmt != 'K' && *fmt != 'T') {
 		/*
 		 * Print (null) with the same width as a pointer so it makes
 		 * tabular output look nice.
@@ -1385,6 +1401,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return dentry_name(buf, end,
 				   ((const struct file *)ptr)->f_path.dentry,
 				   spec, fmt);
+	case 'T':
+		return comm_name(buf, end, ptr, spec, fmt);
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {
-- 
1.7.1

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Kees Cook]

On Thu, Jan 9, 2014 at 4:52 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Hello.
>
> Since addition of %pT itself seems to be agreed, I refreshed this patch using
> linux-next-20140109. Please pick up if this patch is OK for you; I will start
> making patches for killing most of direct ->comm readers.

Looks good; thanks for chasing this. :)

>
> Regards.
> ----------------------------------------
> >From 0d1f03d59a477459f3d3c190593d9e78f5d67de8 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Thu, 9 Jan 2014 21:32:22 +0900
> Subject: [PATCH] lib/vsprintf: add %pT format specifier
>
> Since task_struct->comm can be modified by other threads while the current
> thread is reading it, it is recommended to use get_task_comm() for reading it.
>
> However, since get_task_comm() holds task_struct->alloc_lock spinlock,
> some users cannot use get_task_comm(). Also, a lot of users are directly
> reading from task_struct->comm even if they can use get_task_comm().
> Such users might obtain inconsistent result.
>
> This patch introduces %pT format specifier for printing task_struct->comm.
> Currently %pT does not provide consistency. I'm planning to change to use RCU
> in the future. By using RCU, the comm name read from task_struct->comm will be
> guaranteed to be consistent. But before modifying set_task_comm() to use RCU,
> we need to kill direct ->comm users who do not use get_task_comm().
>
> An example for converting direct ->comm users is shown below. Since many debug
> printings use p == current, you can pass NULL instead of p if p == current.
>
>   pr_info("comm=%s\n", p->comm);       => pr_info("comm=%pT\n", p);
>   pr_info("comm=%s\n", current->comm); => pr_info("comm=%pT\n", NULL);
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Andrew Morton]

On Thu, 9 Jan 2014 21:52:00 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> Hello.
> 
> Since addition of %pT itself seems to be agreed,

sort-of.  The reason I suggested inventing a new token was code
density: avoid pointlessly passing current all the time.

Oh well, whatever - this patch has other intentions.
	
> I refreshed this patch using
> linux-next-20140109. Please pick up if this patch is OK for you; I will start
> making patches for killing most of direct ->comm readers.
> 
> Regards.
> ----------------------------------------
> >From 0d1f03d59a477459f3d3c190593d9e78f5d67de8 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Date: Thu, 9 Jan 2014 21:32:22 +0900
> Subject: [PATCH] lib/vsprintf: add %pT format specifier
> 
> Since task_struct->comm can be modified by other threads while the current
> thread is reading it, it is recommended to use get_task_comm() for reading it.
> 
> However, since get_task_comm() holds task_struct->alloc_lock spinlock,
> some users cannot use get_task_comm(). Also, a lot of users are directly
> reading from task_struct->comm even if they can use get_task_comm().
> Such users might obtain inconsistent result.
> 
> This patch introduces %pT format specifier for printing task_struct->comm.
> Currently %pT does not provide consistency. I'm planning to change to use RCU
> in the future. By using RCU, the comm name read from task_struct->comm will be
> guaranteed to be consistent.

Not completely accurate - RCU won't protect code which accesses ->comm
from interrupts.  Printing current->comm from irq is quite daft, but I
bet there's code that does it :(

As long as the kernel doesn't crash or otherwise misbehave when this
happens, I think we're OK.

(And I guess there's also non-daft code which accesses current->comm
from interrupt context: oops, panic, etc).

> But before modifying set_task_comm() to use RCU,
> we need to kill direct ->comm users who do not use get_task_comm().
> 
> An example for converting direct ->comm users is shown below. Since many debug
> printings use p == current, you can pass NULL instead of p if p == current.
> 
>   pr_info("comm=%s\n", p->comm);       => pr_info("comm=%pT\n", p);
>   pr_info("comm=%s\n", current->comm); => pr_info("comm=%pT\n", NULL);
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> ---
>  Documentation/printk-formats.txt |    6 ++++++
>  lib/vsprintf.c                   |   20 +++++++++++++++++++-
>  2 files changed, 25 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
> index 6f4eb32..94459b4 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -184,6 +184,12 @@ dentry names:
>  	equivalent of %s dentry->d_name.name we used to use, %pd<n> prints
>  	n last components.  %pD does the same thing for struct file.
>  
> +task_struct comm name:
> +
> +        %pT
> +
> +        For printing task_struct->comm.
> +
>  struct va_format:
>  
>  	%pV
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 185b6d3..a97f18b 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1179,6 +1179,21 @@ char *address_val(char *buf, char *end, const void *addr,
>  	return number(buf, end, num, spec);
>  }
>  
> +static noinline_for_stack

hm, does noinline_for_stack actually do anything useful here?  I
suspect it *increases* stack depth in the way comm_name() is used here.

> +char *comm_name(char *buf, char *end, struct task_struct *tsk,
> +		struct printf_spec spec, const char *fmt)
> +{
> +	char name[TASK_COMM_LEN];
> +
> +	/* Caller can pass NULL instead of current. */
> +	if (!tsk)
> +		tsk = current;
> +	/* Not using get_task_comm() in case I'm in IRQ context. */
> +	memcpy(name, tsk->comm, TASK_COMM_LEN);
> +	name[sizeof(name) - 1] = '\0';

get_task_comm() uses strncpy()?

> +	return string(buf, end, name, spec);
> +}
> +

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Tetsuo Handa]

Andrew Morton wrote:
> > This patch introduces %pT format specifier for printing task_struct->comm.
> > Currently %pT does not provide consistency. I'm planning to change to use RCU
> > in the future. By using RCU, the comm name read from task_struct->comm will be
> > guaranteed to be consistent.
> 
> Not completely accurate - RCU won't protect code which accesses ->comm
> from interrupts.  Printing current->comm from irq is quite daft, but I
> bet there's code that does it :(
> 
> As long as the kernel doesn't crash or otherwise misbehave when this
> happens, I think we're OK.
> 
> (And I guess there's also non-daft code which accesses current->comm
> from interrupt context: oops, panic, etc).

Excuse me, but are interrupts relevant?

I think that readers that do

  rcu_read_lock();
  use p->comm here
  rcu_read_unlock();

will be protected from writers that wait freeing p->comm using
synchronize_rcu() or call_rcu().

Is synchronize_rcu() or call_rcu() insufficient for protecting readers that do

  rcu_read_lock();
  use p->comm here
  rcu_read_unlock();

 from interrupts?

> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1179,6 +1179,21 @@ char *address_val(char *buf, char *end, const void *addr,
> >  	return number(buf, end, num, spec);
> >  }
> >  
> > +static noinline_for_stack
> 
> hm, does noinline_for_stack actually do anything useful here?  I
> suspect it *increases* stack depth in the way comm_name() is used here.
> 
I just added noinline_for_stack as with other functions does.
But indeed, stack used by name[] is only 16 bytes but stack used by function
arguments are larger than 16 bytes. We should remove noinline_for_stack ?

> > +char *comm_name(char *buf, char *end, struct task_struct *tsk,
> > +		struct printf_spec spec, const char *fmt)
> > +{
> > +	char name[TASK_COMM_LEN];
> > +
> > +	/* Caller can pass NULL instead of current. */
> > +	if (!tsk)
> > +		tsk = current;
> > +	/* Not using get_task_comm() in case I'm in IRQ context. */
> > +	memcpy(name, tsk->comm, TASK_COMM_LEN);
> > +	name[sizeof(name) - 1] = '\0';
> 
> get_task_comm() uses strncpy()?

I think unconditionally copying 16 bytes using memcpy() is faster than
conditionally copying until '\0'.

> 
> > +	return string(buf, end, name, spec);
> > +}
> > +

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Joe Perches]

On Sat, 2014-01-11 at 10:59 +0900, Tetsuo Handa wrote:
> I just added noinline_for_stack as with other functions does.
> But indeed, stack used by name[] is only 16 bytes but stack used by function
> arguments are larger than 16 bytes. We should remove noinline_for_stack ?

My recollection is that certain gcc versions don't do
well with multiple inline functions that have stack
variables.

Instead of collapsing the variables, gcc will
accumulate the stack depth of all the inlines
args instead of reusing the stack.

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Tetsuo Handa]

Joe Perches wrote:
> On Sat, 2014-01-11 at 10:59 +0900, Tetsuo Handa wrote:
> > I just added noinline_for_stack as with other functions does.
> > But indeed, stack used by name[] is only 16 bytes but stack used by function
> > arguments are larger than 16 bytes. We should remove noinline_for_stack ?
> 
> My recollection is that certain gcc versions don't do
> well with multiple inline functions that have stack
> variables.
> 
> Instead of collapsing the variables, gcc will
> accumulate the stack depth of all the inlines
> args instead of reusing the stack.
> 

I tried what commit cf3b429b "vsprintf.c: use noinline_for_stack" says
using gcc 4.4.7, but it made no difference for stack usage.
Thus, it seems harmless to keep noinline_for_stack keyword.

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Geert Uytterhoeven]

On Sat, Jan 11, 2014 at 12:59 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>> +char *comm_name(char *buf, char *end, struct task_struct *tsk,
>> +             struct printf_spec spec, const char *fmt)
>> +{
>> +     char name[TASK_COMM_LEN];
>> +
>> +     /* Caller can pass NULL instead of current. */
>> +     if (!tsk)
>> +             tsk = current;
>> +     /* Not using get_task_comm() in case I'm in IRQ context. */
>> +     memcpy(name, tsk->comm, TASK_COMM_LEN);

So this may copy more bytes than the actual string length of tsk->comm.
As this is a temporary buffer, that just wastes cycles.
And even if it wasn't, data between the string zero terminator and the end of
the buffer wouild be leaked.

>> +     name[sizeof(name) - 1] = '\0';

You can use strlcpy() here instead of memcpy and clear.

> get_task_comm() uses strncpy()?

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);
        return buf;
}

Is get_task_comm() used to prepare data for userspace?
strncpy() fills the remaining of the buffer with zeroes, so it avoids leaking
data.

Note that strncpy() may leave the buffer non-zero-terminated if the source
string is too long, but as set_task_comm() uses strlcpy(), this should never
be the case:

void set_task_comm(struct task_struct *tsk, char *buf)
{
        task_lock(tsk);
        trace_task_rename(tsk, buf);
        strlcpy(tsk->comm, buf, sizeof(tsk->comm));
        task_unlock(tsk);
        perf_event_comm(tsk);
}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Tetsuo Handa]

Geert Uytterhoeven wrote:
> On Sat, Jan 11, 2014 at 12:59 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> >> +char *comm_name(char *buf, char *end, struct task_struct *tsk,
> >> +             struct printf_spec spec, const char *fmt)
> >> +{
> >> +     char name[TASK_COMM_LEN];
> >> +
> >> +     /* Caller can pass NULL instead of current. */
> >> +     if (!tsk)
> >> +             tsk = current;
> >> +     /* Not using get_task_comm() in case I'm in IRQ context. */
> >> +     memcpy(name, tsk->comm, TASK_COMM_LEN);
> 
> So this may copy more bytes than the actual string length of tsk->comm.
> As this is a temporary buffer, that just wastes cycles.

For example, strncpy() in arch/x86/lib/string_32.c is

char *strncpy(char *dest, const char *src, size_t count)
{
	int d0, d1, d2, d3;
        asm volatile("1:\tdecl %2\n\t"
                "js 2f\n\t"
                "lodsb\n\t"
                "stosb\n\t"
                "testb %%al,%%al\n\t"
                "jne 1b\n\t"
                "rep\n\t"
                "stosb\n"
                "2:"
                : "=&S" (d0), "=&D" (d1), "=&c" (d2), "=&a" (d3)
                : "" (src), "1" (dest), "2" (count) : "memory");
        return dest;
}

and strncpy() in lib/string.c is

char *strncpy(char *dest, const char *src, size_t count)
{
        char *tmp = dest;

        while (count) {
                if ((*tmp = *src) != 0)
                        src++;
                tmp++;
                count--;
        }
        return dest;
}

while memcpy(name, tsk->comm, TASK_COMM_LEN) is

  u64 *dest = (u64 *) name;
  u64 *src = (u64 *) tsk->comm;
  *dest++ = *src++;
  *dest = *src;

if sizeof(long) == 64. I can't understand why unconditionally copying 8 bytes *
2 consumes more cycles than conditionally copying up to 16 bytes...

Also, strncpy() in lib/string.c is not safe for copying task_struct->comm, for
task_struct->comm can change at any moment.

  Initial state:

    p->comm contains "secret_commname\0"

  A reader calls strncpy(buf, p->comm, 16)
  In strncpy() does

    char *dest = buf
    char *src = tsk->comm
    char *tmp = dest
    while (16)
      if ((buf[0] = 's') != 0)
        src++
      tmp++;
      15
    while (15)
      if ((buf[1] = 'e') != 0)
        src++
      tmp++
      14

  At this moment preemption happens, and a writer jumps in.
  The writer calls set_task_comm(p, "x").
  Now p->comm contains "x\0cret_commname\0".
  The preemption ends and the reader continues the loop.
  Now *src == '\0' but continues copying.

    while (14)
      if ((buf[2] = 'c') != 0)
        src++
      tmp++
      13
    (...snipped...)
    while (1)
      if ((buf[15] = '\0') != 0)
      tmp++
      0
    return dest

  and gets "xecret_commname\0" in the buf.
  The reader got some garbage bytes.

What is worse, there are some writers who changes p->comm without using
set_task_comm(). This means that we cannot prove that p->comm contains '\0',
making readers to get non-'\0' terminated comm name.

> And even if it wasn't, data between the string zero terminator and the end of
> the buffer wouild be leaked.
> 
> >> +     name[sizeof(name) - 1] = '\0';
> 
> You can use strlcpy() here instead of memcpy and clear.

For example, strlcpy() in lib/string.c is

size_t strlcpy(char *dest, const char *src, size_t size)
{
        size_t ret = strlen(src);

        if (size) {
                size_t len = (ret >= size) ? size - 1 : ret;
                memcpy(dest, src, len);
                dest[len] = '\0';
        }
        return ret;
}

and strlen(p->comm) can change after ret is calculated, leading to the result
as with strncpy(buf, p->comm, TASK_COMM_LEN).

> strncpy() fills the remaining of the buffer with zeroes, so it avoids leaking
> data.

I don't think that is true, as described above.

> 
> Note that strncpy() may leave the buffer non-zero-terminated if the source
> string is too long, but as set_task_comm() uses strlcpy(), this should never
> be the case:

I don't think that is true, as described above.

Trying to copy p->comm using strncpy() or strlcpy() is not safe. Copy 16 bytes
using memcpy(), and explicitly terminate with '\0' is the safer way, although
any approach may get some garbage bytes.

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Andrew Morton]

On Thu, 9 Jan 2014 21:52:00 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> This patch introduces %pT format specifier for printing task_struct->comm.
> Currently %pT does not provide consistency. I'm planning to change to use RCU
> in the future. By using RCU, the comm name read from task_struct->comm will be
> guaranteed to be consistent. But before modifying set_task_comm() to use RCU,
> we need to kill direct ->comm users who do not use get_task_comm().

On reflection...

It isn't obvious that this patch is sufficiently beneficial until we
have that RCU code in place.

So I could retain this patch in -mm until we have that all sorted out. 
And I'll have to avoid merging %pT users into mainline in the
meanwhile!

Am I wrong?  The patch seems fairly pointless as a standalone thing?

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Tetsuo Handa]

Andrew Morton wrote:
> On Thu, 9 Jan 2014 21:52:00 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> 
> > This patch introduces %pT format specifier for printing task_struct->comm.
> > Currently %pT does not provide consistency. I'm planning to change to use RCU
> > in the future. By using RCU, the comm name read from task_struct->comm will be
> > guaranteed to be consistent. But before modifying set_task_comm() to use RCU,
> > we need to kill direct ->comm users who do not use get_task_comm().
> 
> On reflection...
> 
> It isn't obvious that this patch is sufficiently beneficial until we
> have that RCU code in place.
> 
> So I could retain this patch in -mm until we have that all sorted out. 
> And I'll have to avoid merging %pT users into mainline in the
> meanwhile!
> 
> Am I wrong?  The patch seems fairly pointless as a standalone thing?
> 

  Step 1: (targeted to 3.14-rc1)
    Add "%pT" format specifier and commcpy() wrapper function.

  Step 2: (started after step 1 is reflected to other git trees)
    Replace printk("%s", p->comm) with printk("%pT", p).
    Replace strcpy(buf, p->comm) with commcpy(buf, p).

  Step 3: (started after step 2 is reflected to other git trees)
    Add rcu_read_lock()/rcu_read_unlock() into commcpy().
    Modify set_task_comm() etc. to replace ->comm using RCU.

This patch and commcpy() patch ( https://lkml.org/lkml/2014/1/10/217 ) are
targeted to be merged into 3.14-rc1. This is because we can start writing
patches to use "%pT" or commcpy() only after other git trees have rebased
using 3.14-rc1. After all direct ->comm readers are replaced to use "%pT" or
commcpy(), I can start ->comm modifying functions to use RCU.

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Joe Perches]

On Sat, 2014-01-11 at 10:28 +0900, Tetsuo Handa wrote:
>   Step 1: (targeted to 3.14-rc1)
>     Add "%pT" format specifier and commcpy() wrapper function.
> 
>   Step 2: (started after step 1 is reflected to other git trees)
>     Replace printk("%s", p->comm) with printk("%pT", p).

Replace printk("%s", current->comm) with printk("%pT", NULL);

?

>     Replace strcpy(buf, p->comm) with commcpy(buf, p).

>   Step 3: (started after step 2 is reflected to other git trees)
>     Add rcu_read_lock()/rcu_read_unlock() into commcpy().
>     Modify set_task_comm() etc. to replace ->comm using RCU.

Aren't steps 2 and 3 independent?

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Tetsuo Handa]

Joe Perches wrote:
> On Sat, 2014-01-11 at 10:28 +0900, Tetsuo Handa wrote:
> >   Step 1: (targeted to 3.14-rc1)
> >     Add "%pT" format specifier and commcpy() wrapper function.
> > 
> >   Step 2: (started after step 1 is reflected to other git trees)
> >     Replace printk("%s", p->comm) with printk("%pT", p).
> 
> Replace printk("%s", current->comm) with printk("%pT", NULL);
> 
> ?

Right. I forgot to add it.

> 
> >     Replace strcpy(buf, p->comm) with commcpy(buf, p).
> 
> >   Step 3: (started after step 2 is reflected to other git trees)
> >     Add rcu_read_lock()/rcu_read_unlock() into commcpy().
> >     Modify set_task_comm() etc. to replace ->comm using RCU.
> 
> Aren't steps 2 and 3 independent?
> 

If step 3 is merged before step 2 complete, those who are not using
"%pT" or commcpy() might crash due to reading RCU protected ->comm without
rcu_read_lock()/rcu_read_unlock(). 

Step 2 depends on step 1 for avoiding compile time errors.
Step 3 depends on step 2 for avoiding run time errors.

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Andrew Morton]

On Sat, 11 Jan 2014 10:28:51 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> Andrew Morton wrote:
> > On Thu, 9 Jan 2014 21:52:00 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > 
> > > This patch introduces %pT format specifier for printing task_struct->comm.
> > > Currently %pT does not provide consistency. I'm planning to change to use RCU
> > > in the future. By using RCU, the comm name read from task_struct->comm will be
> > > guaranteed to be consistent. But before modifying set_task_comm() to use RCU,
> > > we need to kill direct ->comm users who do not use get_task_comm().
> > 
> > On reflection...
> > 
> > It isn't obvious that this patch is sufficiently beneficial until we
> > have that RCU code in place.
> > 
> > So I could retain this patch in -mm until we have that all sorted out. 
> > And I'll have to avoid merging %pT users into mainline in the
> > meanwhile!
> > 
> > Am I wrong?  The patch seems fairly pointless as a standalone thing?
> > 
> 
>   Step 1: (targeted to 3.14-rc1)
>     Add "%pT" format specifier and commcpy() wrapper function.
> 
>   Step 2: (started after step 1 is reflected to other git trees)
>     Replace printk("%s", p->comm) with printk("%pT", p).
>     Replace strcpy(buf, p->comm) with commcpy(buf, p).
> 
>   Step 3: (started after step 2 is reflected to other git trees)
>     Add rcu_read_lock()/rcu_read_unlock() into commcpy().
>     Modify set_task_comm() etc. to replace ->comm using RCU.

In the absence of step 3, steps 1 and 2 are rather pointless churn.

So I think it would be better to merge (into mainline) steps 1 and 3
first and at the same time.  Then start thinking about step 2.

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Tetsuo Handa]

Andrew Morton wrote:
> In the absence of step 3, steps 1 and 2 are rather pointless churn.
> 
> So I think it would be better to merge (into mainline) steps 1 and 3
> first and at the same time.  Then start thinking about step 2.

Unfortunately we can't.
Step 2 depends on step 1 for avoiding compile time errors.
Step 3 depends on step 2 for avoiding run time errors.

  Step 1: (targeted to 3.14-rc1)
    Add "%pT" format specifier and commcpy() wrapper function.

  Step 2: (started after step 1 is reflected to other git trees)
    Replace printk("%s", current->comm) with printk("%pT", NULL).
    Replace printk("%s", p->comm) with printk("%pT", p).
    Replace strcpy(buf, p->comm) with commcpy(buf, p).

  Step 3: (started after step 2 is reflected to other git trees)
    Add rcu_read_lock()/rcu_read_unlock() into commcpy().
    Modify set_task_comm() etc. to replace ->comm using RCU.

If step 3 is merged into mainline before step 2 complete, those who are not
using "%pT" or commcpy() might crash due to reading RCU protected ->comm
without rcu_read_lock()/rcu_read_unlock().


Let me confirm, Paul.

  I'm trying to change task_struct->comm to use RCU.
  At step 3, I'm planning to do

  static inline void *commcpy(void *buf, const struct task_struct *tsk)
  {
  	rcu_read_lock();
  	memcpy(buf, tsk->comm, TASK_COMM_LEN);
  	rcu_read_unlock();
  	return buf;
  }

  and let set_task_comm() wait for readers using synchronize_rcu() or
  call_rcu().

  Given that commcpy() can be called from any context, are synchronize_rcu()
  and call_rcu() sufficient for waiting for commcpy() users?

Regards.

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Paul E. McKenney]

On Sat, Jan 11, 2014 at 12:09:46PM +0900, Tetsuo Handa wrote:
> Andrew Morton wrote:
> > In the absence of step 3, steps 1 and 2 are rather pointless churn.
> > 
> > So I think it would be better to merge (into mainline) steps 1 and 3
> > first and at the same time.  Then start thinking about step 2.
> 
> Unfortunately we can't.
> Step 2 depends on step 1 for avoiding compile time errors.
> Step 3 depends on step 2 for avoiding run time errors.
> 
>   Step 1: (targeted to 3.14-rc1)
>     Add "%pT" format specifier and commcpy() wrapper function.
> 
>   Step 2: (started after step 1 is reflected to other git trees)
>     Replace printk("%s", current->comm) with printk("%pT", NULL).
>     Replace printk("%s", p->comm) with printk("%pT", p).
>     Replace strcpy(buf, p->comm) with commcpy(buf, p).
> 
>   Step 3: (started after step 2 is reflected to other git trees)
>     Add rcu_read_lock()/rcu_read_unlock() into commcpy().
>     Modify set_task_comm() etc. to replace ->comm using RCU.
> 
> If step 3 is merged into mainline before step 2 complete, those who are not
> using "%pT" or commcpy() might crash due to reading RCU protected ->comm
> without rcu_read_lock()/rcu_read_unlock().
> 
> 
> Let me confirm, Paul.
> 
>   I'm trying to change task_struct->comm to use RCU.
>   At step 3, I'm planning to do
> 
>   static inline void *commcpy(void *buf, const struct task_struct *tsk)
>   {
>   	rcu_read_lock();
>   	memcpy(buf, tsk->comm, TASK_COMM_LEN);
>   	rcu_read_unlock();
>   	return buf;
>   }
> 
>   and let set_task_comm() wait for readers using synchronize_rcu() or
>   call_rcu().
> 
>   Given that commcpy() can be called from any context, are synchronize_rcu()
>   and call_rcu() sufficient for waiting for commcpy() users?

Yep, that should work.

							Thanx, Paul

Re: [PATCH] lib/vsprintf: add %pT format specifier

[Pavel Machek]

On Fri 2014-01-10 17:57:30, Andrew Morton wrote:
> On Sat, 11 Jan 2014 10:28:51 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> 
> > Andrew Morton wrote:
> > > On Thu, 9 Jan 2014 21:52:00 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > > 
> > > > This patch introduces %pT format specifier for printing task_struct->comm.
> > > > Currently %pT does not provide consistency. I'm planning to change to use RCU
> > > > in the future. By using RCU, the comm name read from task_struct->comm will be
> > > > guaranteed to be consistent. But before modifying set_task_comm() to use RCU,
> > > > we need to kill direct ->comm users who do not use get_task_comm().
> > > 
> > > On reflection...
> > > 
> > > It isn't obvious that this patch is sufficiently beneficial until we
> > > have that RCU code in place.
> > > 
> > > So I could retain this patch in -mm until we have that all sorted out. 
> > > And I'll have to avoid merging %pT users into mainline in the
> > > meanwhile!
> > > 
> > > Am I wrong?  The patch seems fairly pointless as a standalone thing?
> > > 
> > 
> >   Step 1: (targeted to 3.14-rc1)
> >     Add "%pT" format specifier and commcpy() wrapper function.
> > 
> >   Step 2: (started after step 1 is reflected to other git trees)
> >     Replace printk("%s", p->comm) with printk("%pT", p).
> >     Replace strcpy(buf, p->comm) with commcpy(buf, p).
> > 
> >   Step 3: (started after step 2 is reflected to other git trees)
> >     Add rcu_read_lock()/rcu_read_unlock() into commcpy().
> >     Modify set_task_comm() etc. to replace ->comm using RCU.
> 
> In the absence of step 3, steps 1 and 2 are rather pointless churn.

Dunno. Given how often it appears in the code, it looks to me like
step 1 is worthwile cleanup on its own...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html