Messages in this thread | | | Subject | Re: [PATCH] Change task_struct->comm to use RCU. | From | Tetsuo Handa <> | Date | Tue, 25 Feb 2014 21:54:01 +0900 |
| |
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.
| |