Re: [PATCH] Change task_struct->comm to use RCU.

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

Lai Jiangshan wrote:
> CC scheduler people.
> 
> I can't figure out what we get with this patch.
> 
OK. Welcome to this thread. I'll explain you what is going on.

Current problem:

  printk("%s\n", task->comm) is racy because "%s" format specifier assumes that
  the corresponding argument does not change between strnlen() and the for loop
  at string() in lib/vsnprintf.c . If task->comm was "Hello Linux" until
  strnlen() and becomes "Penguin" before the for loop, "%s" will emit
  "Penguin\0nux" (note the unexpected '\0' byte and the garbage bytes).

  Likewise, audit_log_untrustedstring(ab, current->comm) is racy.
  If task->comm was "Hello Linux" until audit_string_contains_control() in
  audit_log_n_untrustedstring() returns false, and becomes "Penguin" before
  memcpy() in audit_log_n_string() is called, memcpy() will emit "Penguin\0nux"
  into the audit log, which results in loss of information (e.g. SELinux
  context) due to the unexpected '\0' byte.

Proposed solution:

  To fix abovementioned problem, I proposed commcpy() and "%pT" format
  specifier which does

    char tmp[16];
    memcpy(tmp, task->comm, 16);
    tmp[15] = '\0';
    sprintf(buf, "%s", tmp);

  instead of

    sprintf(buf, "%s", task->comm);

  .

Remaining problem:

  Although the proposed solution will prevent the caller from emitting the
  unexpected '\0' byte and the garbage bytes, memcpy(tmp, task->comm, 16) in
  the proposed solution is not atomic. That is, "%pT" does not emit the '\0'
  byte like "Penguin\0nux" but "%pT" still might emit "Penguininux".

  To fix this problem, I proposed protecting memcpy(tmp, task->comm, 16) part
  using RCU. This patch is a design for how the update side of task->comm will
  look like if we use RCU approach.

  Of course, this approach depends on that nobody prefers the speed of reading
  task->comm over the atomicity of reading task->comm . If somebody strongly
  objects on the cost of calling rcu_read_lock()/rcu_read_unlock() for the
  atomicity, I'm fine without this patch.