(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