Bug 1430828
| Summary: | Ensure mm cannot be unmapped in task_exe_file() | ||
|---|---|---|---|
| Product: | Red Hat Enterprise Linux 7 | Reporter: | Aaron Tomlin <atomlin> |
| Component: | systemtap | Assignee: | David Smith <dsmith> |
| Status: | CLOSED ERRATA | QA Contact: | Martin Cermak <mcermak> |
| Severity: | low | Docs Contact: | |
| Priority: | low | ||
| Version: | 7.4 | CC: | atomlin, dsmith, fche, jistone, kwalker, lberk, mcermak, mjw |
| Target Milestone: | rc | ||
| Target Release: | --- | ||
| Hardware: | Unspecified | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | systemtap-3.1-3.el7 | Doc Type: | If docs needed, set a value |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2017-08-01 09:34:40 UTC | Type: | Bug |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
| Bug Depends On: | 1430861 | ||
| Bug Blocks: | |||
(In reply to Aaron Tomlin from comment #0) > Problem description: > -------------------- > > The use of get_task_struct(p) in tapset/linux/task.stp:task_exe_file() is > primarily to ensure p->mm cannot be unexpectedly unmapped. > However this assumption is not true since get_task_struct() does not > increment mm_user. So task_exe_file() can race with mmput() which can > lead to an Oops. Also task_exe_file() should have /* guru */ added. I think we're going to need to back up here. Can you show me a reproducer of this oops you mentioned? Why do you think task_exe_file() should have guru added? > Version-Release number of selected component (if applicable): > > systemtap-client-3.0-7.el7 (*) > > [*] Note: Upstream version is also affected i.e. > since [to date] commit de25b7b ("For custom built upstream > kernels on Debian, fix version detection") > > Additional info: > > In task_exe_file(), we never invoke fput() due to the 'goto out' statement > (see the definition of the STAP_RETURN macro) therefore the file object is > leaked. This is to be resolved by another bug report. That does appear to be a problem. I don't mind fixing that one here or in a different bug report - whatever makes the most sense. (In reply to David Smith from comment #4) [ ... ] > I think we're going to need to back up here. Can you show me a reproducer of > this oops you mentioned? No actual reproducer was used. This was found by code review. I can see about reproducing this for real if required. > Why do you think task_exe_file() should have guru added? In my opinion, extra care should be taken when this function is used since as is, it is not safe. > > Version-Release number of selected component (if applicable): > > > > systemtap-client-3.0-7.el7 (*) > > > > [*] Note: Upstream version is also affected i.e. > > since [to date] commit de25b7b ("For custom built upstream > > kernels on Debian, fix version detection") > > > > Additional info: > > > > In task_exe_file(), we never invoke fput() due to the 'goto out' statement > > (see the definition of the STAP_RETURN macro) therefore the file object is > > leaked. This is to be resolved by another bug report. > > That does appear to be a problem. I don't mind fixing that one here or in a > different bug report - whatever makes the most sense. This is being tracked via bug 1430861. (In reply to Aaron Tomlin from comment #6) > (In reply to David Smith from comment #4) > [ ... ] > > I think we're going to need to back up here. Can you show me a reproducer of > > this oops you mentioned? > > No actual reproducer was used. This was found by code review. > I can see about reproducing this for real if required. Code inspection is fine. We actually test this function in the testsuite, but only against 'current'. > > Why do you think task_exe_file() should have guru added? > > In my opinion, extra care should be taken when this function is used since > as is, it is not safe. So if it was (somehow) safe, it wouldn't need guru? If so, the problem isn't the lack of 'guru', it is the lack of safety. > > > Version-Release number of selected component (if applicable): > > > > > > systemtap-client-3.0-7.el7 (*) > > > > > > [*] Note: Upstream version is also affected i.e. > > > since [to date] commit de25b7b ("For custom built upstream > > > kernels on Debian, fix version detection") > > > > > > Additional info: > > > > > > In task_exe_file(), we never invoke fput() due to the 'goto out' statement > > > (see the definition of the STAP_RETURN macro) therefore the file object is > > > leaked. This is to be resolved by another bug report. > > > > That does appear to be a problem. I don't mind fixing that one here or in a > > different bug report - whatever makes the most sense. > > This is being tracked via bug 1430861. One possible solution here would be to rewrite/rename this function to something like: 'current_exe_file'. It would only return the file struct of current's exe. If we're stopped inside current, current's mm isn't going anywhere. (In reply to David Smith from comment #8) > Code inspection is fine. We actually test this function in the testsuite, > but only against 'current'. OK. > One possible solution here would be to rewrite/rename this function to > something like: 'current_exe_file'. It would only return the file struct of > current's exe. If we're stopped inside current, current's mm isn't going > anywhere. How about the following: diff --git a/tapset/linux/task.stp b/tapset/linux/task.stp index 12e2868..589992c 100644 --- a/tapset/linux/task.stp +++ b/tapset/linux/task.stp @@ -781,39 +781,23 @@ function task_cwd_path:long(task:long) %} /** - * sfunction task_cwd_path - get the file struct pointer for a task's executable file - * - * @task: task_struct pointer. + * sfunction current_exe_file - Returns the file struct pointer of the current + * process' executable file */ function task_exe_file:long(task:long) %{ /* pure */ - struct task_struct *task - = (struct task_struct *)(unsigned long)STAP_ARG_task; - struct mm_struct *mm = NULL; - struct file *exe_file = NULL; - - // Before using the task_struct pointer, make sure it is valid - // to read. - (void)kderef_buffer(NULL, task, sizeof(struct task_struct)); + struct mm_struct *mm; + struct file *exe_file; - // OK, we now know it is valid to read. But, is it really a - // task struct? - if (!_stp_task_struct_valid(task)) { - STAP_ERROR ("invalid task struct pointer"); - } + mm = exe_file = NULL; - // We'd like to call get_task_mm()/mmput() here, but they can - // sleep. So, let's hope incrementing the task's usage (by - // calling get_task_struct) is enough to keep the mm around. - get_task_struct(task); - mm = task->mm; + mm = task_current()->mm; if (mm) exe_file = stap_find_exe_file(mm); - put_task_struct(task); - if (exe_file) { - STAP_RETURN((unsigned long)exe_file); + if (exe_file) fput(exe_file); - } + + STAP_RETURN((unsigned long)exe_file); CATCH_DEREF_FAULT(); %} (In reply to Aaron Tomlin from comment #9) > How about the following: Correction to the patch in comment #9 in particular to the function name and parameter: diff --git a/tapset/linux/task.stp b/tapset/linux/task.stp index 12e2868..4974df5 100644 --- a/tapset/linux/task.stp +++ b/tapset/linux/task.stp @@ -781,39 +781,23 @@ function task_cwd_path:long(task:long) %} /** - * sfunction task_cwd_path - get the file struct pointer for a task's executable file - * - * @task: task_struct pointer. + * sfunction current_exe_file - Returns the file struct pointer of the current + * process' executable file */ -function task_exe_file:long(task:long) +function current_exe_file:long() %{ /* pure */ - struct task_struct *task - = (struct task_struct *)(unsigned long)STAP_ARG_task; - struct mm_struct *mm = NULL; - struct file *exe_file = NULL; - - // Before using the task_struct pointer, make sure it is valid - // to read. - (void)kderef_buffer(NULL, task, sizeof(struct task_struct)); + struct mm_struct *mm; + struct file *exe_file; - // OK, we now know it is valid to read. But, is it really a - // task struct? - if (!_stp_task_struct_valid(task)) { - STAP_ERROR ("invalid task struct pointer"); - } + mm = exe_file = NULL; - // We'd like to call get_task_mm()/mmput() here, but they can - // sleep. So, let's hope incrementing the task's usage (by - // calling get_task_struct) is enough to keep the mm around. - get_task_struct(task); - mm = task->mm; + mm = task_current()->mm; if (mm) exe_file = stap_find_exe_file(mm); - put_task_struct(task); - if (exe_file) { - STAP_RETURN((unsigned long)exe_file); + if (exe_file) fput(exe_file); - } + + STAP_RETURN((unsigned long)exe_file); CATCH_DEREF_FAULT(); %} I took the 'current_exe_file' route in the following upstream commit (before I saw your patch): <https://www.sourceware.org/git/gitweb.cgi?p=systemtap.git;a=commit;h=4ed3fbc366d168806f94a615f3339b4d2fbd5a75> This should fix the problem. (In reply to Aaron Tomlin from comment #10) Finally compile tested version (XXX can we add /* unprivileged */ to current_exe_file()? ...): diff --git a/tapset/linux/task.stp b/tapset/linux/task.stp index 12e2868..3395c16 100644 --- a/tapset/linux/task.stp +++ b/tapset/linux/task.stp @@ -781,39 +781,21 @@ function task_cwd_path:long(task:long) %} /** - * sfunction task_cwd_path - get the file struct pointer for a task's executable file - * - * @task: task_struct pointer. + * sfunction current_exe_file - Returns the file struct pointer of the current + * process' executable file */ -function task_exe_file:long(task:long) +function current_exe_file:long() %{ /* pure */ - struct task_struct *task - = (struct task_struct *)(unsigned long)STAP_ARG_task; struct mm_struct *mm = NULL; struct file *exe_file = NULL; - // Before using the task_struct pointer, make sure it is valid - // to read. - (void)kderef_buffer(NULL, task, sizeof(struct task_struct)); - - // OK, we now know it is valid to read. But, is it really a - // task struct? - if (!_stp_task_struct_valid(task)) { - STAP_ERROR ("invalid task struct pointer"); - } - - // We'd like to call get_task_mm()/mmput() here, but they can - // sleep. So, let's hope incrementing the task's usage (by - // calling get_task_struct) is enough to keep the mm around. - get_task_struct(task); - mm = task->mm; + mm = current->mm; if (mm) exe_file = stap_find_exe_file(mm); - put_task_struct(task); - if (exe_file) { - STAP_RETURN((unsigned long)exe_file); + if (exe_file) fput(exe_file); - } + + STAP_RETURN((unsigned long)exe_file); CATCH_DEREF_FAULT(); %} (In reply to David Smith from comment #11) > I took the 'current_exe_file' route in the following upstream commit (before > I saw your patch): OK. > <https://www.sourceware.org/git/gitweb.cgi?p=systemtap.git;a=commit; > h=4ed3fbc366d168806f94a615f3339b4d2fbd5a75> > > This should fix the problem. Yes, your patch does resolve the issue. Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory, and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. https://access.redhat.com/errata/RHBA-2017:2301 |
Problem description: -------------------- The use of get_task_struct(p) in tapset/linux/task.stp:task_exe_file() is primarily to ensure p->mm cannot be unexpectedly unmapped. However this assumption is not true since get_task_struct() does not increment mm_user. So task_exe_file() can race with mmput() which can lead to an Oops. Also task_exe_file() should have /* guru */ added. Version-Release number of selected component (if applicable): systemtap-client-3.0-7.el7 (*) [*] Note: Upstream version is also affected i.e. since [to date] commit de25b7b ("For custom built upstream kernels on Debian, fix version detection") Additional info: In task_exe_file(), we never invoke fput() due to the 'goto out' statement (see the definition of the STAP_RETURN macro) therefore the file object is leaked. This is to be resolved by another bug report.