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

[Nicholas Piggin]

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:
>> Excerpts from Haren Myneni's message of May 21, 2021 7:31 pm:
>> > 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 at linux.ibm.com>
>> 
>> Seems like a good idea to put these into their own helper functions.
>> 
>> > ---
>> >  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)
>> 
>> 
>> > +
>> >  /*
>> >   * 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...

>> > +	/*
>> > +	 * 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