Re: [PATCH v4 04/16] powerpc/vas: Create take/drop pid and mm references

Excerpts from Haren Myneni's message of June 4, 2021 2:08 pm:

On Thu, 2021-06-03 at 14:21 +1000, Nicholas Piggin wrote:

quoted

Excerpts from Haren Myneni's message of May 21, 2021 7:31 pm:

quoted

Take pid and mm references when each window opens and drops during
close. This functionality is needed for powerNV and pseries. So
this patch defines the existing code as functions in common book3s
platform vas-api.c

Signed-off-by: Haren Myneni <haren@linux.ibm.com>

Seems like a good idea to put these into their own helper functions.

quoted

---
 arch/powerpc/include/asm/vas.h              | 25 +++++++++
 arch/powerpc/platforms/book3s/vas-api.c     | 51
++++++++++++++++++
 arch/powerpc/platforms/powernv/vas-fault.c  | 10 ++--
 arch/powerpc/platforms/powernv/vas-window.c | 57 ++---------------
----
 arch/powerpc/platforms/powernv/vas.h        |  6 +--
 5 files changed, 88 insertions(+), 61 deletions(-)

diff --git a/arch/powerpc/include/asm/vas.h

b/arch/powerpc/include/asm/vas.h
index 668303198772..3f2b02461a76 100644

--- a/arch/powerpc/include/asm/vas.h
+++ b/arch/powerpc/include/asm/vas.h

@@ -5,6 +5,9 @@
 
 #ifndef _ASM_POWERPC_VAS_H
 #define _ASM_POWERPC_VAS_H
+#include <linux/sched/mm.h>
+#include <linux/mmu_context.h>
+#include <asm/icswx.h>
 #include <uapi/asm/vas-api.h>
 
 struct vas_window;

@@ -49,6 +52,17 @@ enum vas_cop_type {
 	VAS_COP_TYPE_MAX,
 };
 
+/*
+ * User space VAS windows are opened by tasks and take references
+ * to pid and mm until windows are closed.
+ * Stores pid, mm, and tgid for each window.
+ */
+struct vas_user_win_ref {
+	struct pid *pid;	/* PID of owner */
+	struct pid *tgid;	/* Thread group ID of owner */
+	struct mm_struct *mm;	/* Linux process mm_struct */
+};
+
 /*
  * User space window operations used for powernv and powerVM
  */

@@ -59,6 +73,16 @@ struct vas_user_win_ops {
 	int (*close_win)(void *);
 };
 
+static inline void vas_drop_reference_pid_mm(struct

vas_user_win_ref *ref)
+{
+	/* Drop references to pid and mm */
+	put_pid(ref->pid);
+	if (ref->mm) {
+		mm_context_remove_vas_window(ref->mm);
+		mmdrop(ref->mm);
+	}
+}

You don't have to make up a new name for such a thing because you 
already have one

put_vas_user_win_ref(struct vas_user_win_ref *ref)

quoted

+
 /*
  * Receive window attributes specified by the (in-kernel) owner of
window.
  */

@@ -192,4 +216,5 @@ int vas_register_coproc_api(struct module *mod,

enum vas_cop_type cop_type,
 			    struct vas_user_win_ops *vops);
 void vas_unregister_coproc_api(void);
 
+int vas_reference_pid_mm(struct vas_user_win_ref *task_ref);
 #endif /* __ASM_POWERPC_VAS_H */

diff --git a/arch/powerpc/platforms/book3s/vas-api.c

b/arch/powerpc/platforms/book3s/vas-api.c
index 6c39320bfb9b..a0141bfb2e4b 100644

--- a/arch/powerpc/platforms/book3s/vas-api.c
+++ b/arch/powerpc/platforms/book3s/vas-api.c

@@ -55,6 +55,57 @@ static char *coproc_devnode(struct device *dev,

umode_t *mode)
 	return kasprintf(GFP_KERNEL, "crypto/%s", dev_name(dev));
 }
 
+/*
+ * Take reference to pid and mm
+ */
+int vas_reference_pid_mm(struct vas_user_win_ref *task_ref)
+{

So this is quite different from a typical refcount object in that
it's 
opening it for access as well. I would split it in two functions, one
matching put_vas_user_win_ref() and appearing in the same place in
code,
which is up to about mmput and another function that adds the window
and
does the CP_ABORT etc... hmm, where do you release tgid?

Basically copied the existing code in to these functions
(vas_reference_pid_mm/vas_drop_reference_pid_mm) so that useful for
both platforms. 

mm_context_add/remove_vas_window() is also like taking reference. So
instead of adding 2 seperate functions, how about naming
get/put_vas_user_win_ref() 

It's actually different though. What I'm asking is the parts where you 
interact with core kernel data structure refcounts go into their own 
get/put functions.

Someone who understands that refcounting and looks at the code will care 
about those bits, so having them all together I think is helpful. They 
don't know about adding vas windows or CP_ABORT.

Regarding tgid, the reference is taking only with pid, but not tgid.
pid reuse can happen only in the case of multithread applications when
the child that opened VAS window exits. But these windows will be
closed when tgid exists. So do not need tgid reference.

I don't understand you.  The code you added does take a reference to 
tgid...

quoted

quoted

+	/*
+	 * Window opened by a child thread may not be closed when
+	 * it exits. So take reference to its pid and release it
+	 * when the window is free by parent thread.
+	 * Acquire a reference to the task's pid to make sure
+	 * pid will not be re-used - needed only for multithread
+	 * applications.
+	 */
+	task_ref->pid = get_task_pid(current, PIDTYPE_PID);

My question is, where is this reference released. I can't see it. I'm 
asking about existing upstream code really because this patch is just 
copying existing code around.

Thanks,
Nick