| Message ID | 16a319614a7ab4ce843f42a49c3ecf68ed03dd36.camel@linux.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Enable VAS and NX-GZIP support on powerVM | expand |
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@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? Thanks, Nick > + /* > + * 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); > + /* > + * Acquire a reference to the task's mm. > + */ > + task_ref->mm = get_task_mm(current); > + if (!task_ref->mm) { > + put_pid(task_ref->pid); > + pr_err("VAS: pid(%d): mm_struct is not found\n", > + current->pid); > + return -EPERM; > + } > + > + mmgrab(task_ref->mm); > + mmput(task_ref->mm); > + mm_context_add_vas_window(task_ref->mm); > + /* > + * Process closes window during exit. In the case of > + * multithread application, the child thread can open > + * window and can exit without closing it. Expects parent > + * thread to use and close the window. So do not need > + * to take pid reference for parent thread. > + */ > + task_ref->tgid = find_get_pid(task_tgid_vnr(current)); > + /* > + * Even a process that has no foreign real address mapping can > + * use an unpaired COPY instruction (to no real effect). Issue > + * CP_ABORT to clear any pending COPY and prevent a covert > + * channel. > + * > + * __switch_to() will issue CP_ABORT on future context switches > + * if process / thread has any open VAS window (Use > + * current->mm->context.vas_windows). > + */ > + asm volatile(PPC_CP_ABORT); > + > + return 0; > +} > + > static int coproc_open(struct inode *inode, struct file *fp) > { > struct coproc_instance *cp_inst; > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c > index 3d21fce254b7..ac3a71ec3bd5 100644 > --- a/arch/powerpc/platforms/powernv/vas-fault.c > +++ b/arch/powerpc/platforms/powernv/vas-fault.c > @@ -73,7 +73,7 @@ static void update_csb(struct vas_window *window, > * NX user space windows can not be opened for task->mm=NULL > * and faults will not be generated for kernel requests. > */ > - if (WARN_ON_ONCE(!window->mm || !window->user_win)) > + if (WARN_ON_ONCE(!window->task_ref.mm || !window->user_win)) > return; > > csb_addr = (void __user *)be64_to_cpu(crb->csb_addr); > @@ -92,7 +92,7 @@ static void update_csb(struct vas_window *window, > csb.address = crb->stamp.nx.fault_storage_addr; > csb.flags = 0; > > - pid = window->pid; > + pid = window->task_ref.pid; > tsk = get_pid_task(pid, PIDTYPE_PID); > /* > * Process closes send window after all pending NX requests are > @@ -111,7 +111,7 @@ static void update_csb(struct vas_window *window, > * a window and exits without closing it. > */ > if (!tsk) { > - pid = window->tgid; > + pid = window->task_ref.tgid; > tsk = get_pid_task(pid, PIDTYPE_PID); > /* > * Parent thread (tgid) will be closing window when it > @@ -127,7 +127,7 @@ static void update_csb(struct vas_window *window, > return; > } > > - kthread_use_mm(window->mm); > + kthread_use_mm(window->task_ref.mm); > rc = copy_to_user(csb_addr, &csb, sizeof(csb)); > /* > * User space polls on csb.flags (first byte). So add barrier > @@ -139,7 +139,7 @@ static void update_csb(struct vas_window *window, > smp_mb(); > rc = copy_to_user(csb_addr, &csb, sizeof(u8)); > } > - kthread_unuse_mm(window->mm); > + kthread_unuse_mm(window->task_ref.mm); > put_task_struct(tsk); > > /* Success */ > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c > index 3ccd3edcaf1a..ffd619e5a218 100644 > --- a/arch/powerpc/platforms/powernv/vas-window.c > +++ b/arch/powerpc/platforms/powernv/vas-window.c > @@ -1065,51 +1065,9 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop, > rc = -ENODEV; > goto free_window; > } > - > - /* > - * 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. > - */ > - txwin->pid = get_task_pid(current, PIDTYPE_PID); > - /* > - * Acquire a reference to the task's mm. > - */ > - txwin->mm = get_task_mm(current); > - > - if (!txwin->mm) { > - put_pid(txwin->pid); > - pr_err("VAS: pid(%d): mm_struct is not found\n", > - current->pid); > - rc = -EPERM; > + rc = vas_reference_pid_mm(&txwin->task_ref); > + if (rc) > goto free_window; > - } > - > - mmgrab(txwin->mm); > - mmput(txwin->mm); > - mm_context_add_vas_window(txwin->mm); > - /* > - * Process closes window during exit. In the case of > - * multithread application, the child thread can open > - * window and can exit without closing it. Expects parent > - * thread to use and close the window. So do not need > - * to take pid reference for parent thread. > - */ > - txwin->tgid = find_get_pid(task_tgid_vnr(current)); > - /* > - * Even a process that has no foreign real address mapping can > - * use an unpaired COPY instruction (to no real effect). Issue > - * CP_ABORT to clear any pending COPY and prevent a covert > - * channel. > - * > - * __switch_to() will issue CP_ABORT on future context switches > - * if process / thread has any open VAS window (Use > - * current->mm->context.vas_windows). > - */ > - asm volatile(PPC_CP_ABORT); > } > > set_vinst_win(vinst, txwin); > @@ -1339,14 +1297,9 @@ int vas_win_close(struct vas_window *window) > > /* if send window, drop reference to matching receive window */ > if (window->tx_win) { > - if (window->user_win) { > - /* Drop references to pid and mm */ > - put_pid(window->pid); > - if (window->mm) { > - mm_context_remove_vas_window(window->mm); > - mmdrop(window->mm); > - } > - } > + if (window->user_win) > + vas_drop_reference_pid_mm(&window->task_ref); > + > put_rx_win(window->rxwin); > } > > diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h > index c7db3190baca..f354dd5c51bd 100644 > --- a/arch/powerpc/platforms/powernv/vas.h > +++ b/arch/powerpc/platforms/powernv/vas.h > @@ -357,11 +357,9 @@ struct vas_window { > bool user_win; /* True if user space window */ > void *hvwc_map; /* HV window context */ > void *uwc_map; /* OS/User window context */ > - struct pid *pid; /* Linux process id of owner */ > - struct pid *tgid; /* Thread group ID of owner */ > - struct mm_struct *mm; /* Linux process mm_struct */ > int wcreds_max; /* Window credits */ > > + struct vas_user_win_ref task_ref; > char *dbgname; > struct dentry *dbgdir; > > @@ -443,7 +441,7 @@ extern void vas_win_paste_addr(struct vas_window *window, u64 *addr, > > static inline int vas_window_pid(struct vas_window *window) > { > - return pid_vnr(window->pid); > + return pid_vnr(window->task_ref.pid); > } > > static inline void vas_log_write(struct vas_window *win, char *name, > -- > 2.18.2 > > >
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@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() 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. Thanks Haren > > Thanks, > Nick > > > + /* > > + * 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); > > + /* > > + * Acquire a reference to the task's mm. > > + */ > > + task_ref->mm = get_task_mm(current); > > + if (!task_ref->mm) { > > + put_pid(task_ref->pid); > > + pr_err("VAS: pid(%d): mm_struct is not found\n", > > + current->pid); > > + return -EPERM; > > + } > > + > > + mmgrab(task_ref->mm); > > + mmput(task_ref->mm); > > + mm_context_add_vas_window(task_ref->mm); > > + /* > > + * Process closes window during exit. In the case of > > + * multithread application, the child thread can open > > + * window and can exit without closing it. Expects parent > > + * thread to use and close the window. So do not need > > + * to take pid reference for parent thread. > > + */ > > + task_ref->tgid = find_get_pid(task_tgid_vnr(current)); > > + /* > > + * Even a process that has no foreign real address mapping can > > + * use an unpaired COPY instruction (to no real effect). Issue > > + * CP_ABORT to clear any pending COPY and prevent a covert > > + * channel. > > + * > > + * __switch_to() will issue CP_ABORT on future context switches > > + * if process / thread has any open VAS window (Use > > + * current->mm->context.vas_windows). > > + */ > > + asm volatile(PPC_CP_ABORT); > > + > > + return 0; > > +} > > + > > static int coproc_open(struct inode *inode, struct file *fp) > > { > > struct coproc_instance *cp_inst; > > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c > > b/arch/powerpc/platforms/powernv/vas-fault.c > > index 3d21fce254b7..ac3a71ec3bd5 100644 > > --- a/arch/powerpc/platforms/powernv/vas-fault.c > > +++ b/arch/powerpc/platforms/powernv/vas-fault.c > > @@ -73,7 +73,7 @@ static void update_csb(struct vas_window *window, > > * NX user space windows can not be opened for task->mm=NULL > > * and faults will not be generated for kernel requests. > > */ > > - if (WARN_ON_ONCE(!window->mm || !window->user_win)) > > + if (WARN_ON_ONCE(!window->task_ref.mm || !window->user_win)) > > return; > > > > csb_addr = (void __user *)be64_to_cpu(crb->csb_addr); > > @@ -92,7 +92,7 @@ static void update_csb(struct vas_window *window, > > csb.address = crb->stamp.nx.fault_storage_addr; > > csb.flags = 0; > > > > - pid = window->pid; > > + pid = window->task_ref.pid; > > tsk = get_pid_task(pid, PIDTYPE_PID); > > /* > > * Process closes send window after all pending NX requests are > > @@ -111,7 +111,7 @@ static void update_csb(struct vas_window > > *window, > > * a window and exits without closing it. > > */ > > if (!tsk) { > > - pid = window->tgid; > > + pid = window->task_ref.tgid; > > tsk = get_pid_task(pid, PIDTYPE_PID); > > /* > > * Parent thread (tgid) will be closing window when it > > @@ -127,7 +127,7 @@ static void update_csb(struct vas_window > > *window, > > return; > > } > > > > - kthread_use_mm(window->mm); > > + kthread_use_mm(window->task_ref.mm); > > rc = copy_to_user(csb_addr, &csb, sizeof(csb)); > > /* > > * User space polls on csb.flags (first byte). So add barrier > > @@ -139,7 +139,7 @@ static void update_csb(struct vas_window > > *window, > > smp_mb(); > > rc = copy_to_user(csb_addr, &csb, sizeof(u8)); > > } > > - kthread_unuse_mm(window->mm); > > + kthread_unuse_mm(window->task_ref.mm); > > put_task_struct(tsk); > > > > /* Success */ > > diff --git a/arch/powerpc/platforms/powernv/vas-window.c > > b/arch/powerpc/platforms/powernv/vas-window.c > > index 3ccd3edcaf1a..ffd619e5a218 100644 > > --- a/arch/powerpc/platforms/powernv/vas-window.c > > +++ b/arch/powerpc/platforms/powernv/vas-window.c > > @@ -1065,51 +1065,9 @@ struct vas_window *vas_tx_win_open(int > > vasid, enum vas_cop_type cop, > > rc = -ENODEV; > > goto free_window; > > } > > - > > - /* > > - * 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. > > - */ > > - txwin->pid = get_task_pid(current, PIDTYPE_PID); > > - /* > > - * Acquire a reference to the task's mm. > > - */ > > - txwin->mm = get_task_mm(current); > > - > > - if (!txwin->mm) { > > - put_pid(txwin->pid); > > - pr_err("VAS: pid(%d): mm_struct is not > > found\n", > > - current->pid); > > - rc = -EPERM; > > + rc = vas_reference_pid_mm(&txwin->task_ref); > > + if (rc) > > goto free_window; > > - } > > - > > - mmgrab(txwin->mm); > > - mmput(txwin->mm); > > - mm_context_add_vas_window(txwin->mm); > > - /* > > - * Process closes window during exit. In the case of > > - * multithread application, the child thread can open > > - * window and can exit without closing it. Expects > > parent > > - * thread to use and close the window. So do not need > > - * to take pid reference for parent thread. > > - */ > > - txwin->tgid = find_get_pid(task_tgid_vnr(current)); > > - /* > > - * Even a process that has no foreign real address > > mapping can > > - * use an unpaired COPY instruction (to no real > > effect). Issue > > - * CP_ABORT to clear any pending COPY and prevent a > > covert > > - * channel. > > - * > > - * __switch_to() will issue CP_ABORT on future context > > switches > > - * if process / thread has any open VAS window (Use > > - * current->mm->context.vas_windows). > > - */ > > - asm volatile(PPC_CP_ABORT); > > } > > > > set_vinst_win(vinst, txwin); > > @@ -1339,14 +1297,9 @@ int vas_win_close(struct vas_window *window) > > > > /* if send window, drop reference to matching receive window */ > > if (window->tx_win) { > > - if (window->user_win) { > > - /* Drop references to pid and mm */ > > - put_pid(window->pid); > > - if (window->mm) { > > - mm_context_remove_vas_window(window- > > >mm); > > - mmdrop(window->mm); > > - } > > - } > > + if (window->user_win) > > + vas_drop_reference_pid_mm(&window->task_ref); > > + > > put_rx_win(window->rxwin); > > } > > > > diff --git a/arch/powerpc/platforms/powernv/vas.h > > b/arch/powerpc/platforms/powernv/vas.h > > index c7db3190baca..f354dd5c51bd 100644 > > --- a/arch/powerpc/platforms/powernv/vas.h > > +++ b/arch/powerpc/platforms/powernv/vas.h > > @@ -357,11 +357,9 @@ struct vas_window { > > bool user_win; /* True if user space window */ > > void *hvwc_map; /* HV window context */ > > void *uwc_map; /* OS/User window context */ > > - struct pid *pid; /* Linux process id of owner */ > > - struct pid *tgid; /* Thread group ID of owner */ > > - struct mm_struct *mm; /* Linux process mm_struct */ > > int wcreds_max; /* Window credits */ > > > > + struct vas_user_win_ref task_ref; > > char *dbgname; > > struct dentry *dbgdir; > > > > @@ -443,7 +441,7 @@ extern void vas_win_paste_addr(struct > > vas_window *window, u64 *addr, > > > > static inline int vas_window_pid(struct vas_window *window) > > { > > - return pid_vnr(window->pid); > > + return pid_vnr(window->task_ref.pid); > > } > > > > static inline void vas_log_write(struct vas_window *win, char > > *name, > > -- > > 2.18.2 > > > > > >
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@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
Excerpts from Nicholas Piggin's message of June 5, 2021 10:31 am: > 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@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); Quoted the wrong bit obviously: + /* + * Process closes window during exit. In the case of + * multithread application, the child thread can open + * window and can exit without closing it. Expects parent + * thread to use and close the window. So do not need + * to take pid reference for parent thread. + */ + task_ref->tgid = find_get_pid(task_tgid_vnr(current)); aka vas_tx_win_open() in upstream code. It's not a comment about this patch specificlaly, I just noticed it and I wanted to make sure I'm not missing somehting or the existing code isn't buggy before the patch goes in. Thanks, Nick
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); + } +} + /* * 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) +{ + /* + * 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); + /* + * Acquire a reference to the task's mm. + */ + task_ref->mm = get_task_mm(current); + if (!task_ref->mm) { + put_pid(task_ref->pid); + pr_err("VAS: pid(%d): mm_struct is not found\n", + current->pid); + return -EPERM; + } + + mmgrab(task_ref->mm); + mmput(task_ref->mm); + mm_context_add_vas_window(task_ref->mm); + /* + * Process closes window during exit. In the case of + * multithread application, the child thread can open + * window and can exit without closing it. Expects parent + * thread to use and close the window. So do not need + * to take pid reference for parent thread. + */ + task_ref->tgid = find_get_pid(task_tgid_vnr(current)); + /* + * Even a process that has no foreign real address mapping can + * use an unpaired COPY instruction (to no real effect). Issue + * CP_ABORT to clear any pending COPY and prevent a covert + * channel. + * + * __switch_to() will issue CP_ABORT on future context switches + * if process / thread has any open VAS window (Use + * current->mm->context.vas_windows). + */ + asm volatile(PPC_CP_ABORT); + + return 0; +} + static int coproc_open(struct inode *inode, struct file *fp) { struct coproc_instance *cp_inst; diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c index 3d21fce254b7..ac3a71ec3bd5 100644 --- a/arch/powerpc/platforms/powernv/vas-fault.c +++ b/arch/powerpc/platforms/powernv/vas-fault.c @@ -73,7 +73,7 @@ static void update_csb(struct vas_window *window, * NX user space windows can not be opened for task->mm=NULL * and faults will not be generated for kernel requests. */ - if (WARN_ON_ONCE(!window->mm || !window->user_win)) + if (WARN_ON_ONCE(!window->task_ref.mm || !window->user_win)) return; csb_addr = (void __user *)be64_to_cpu(crb->csb_addr); @@ -92,7 +92,7 @@ static void update_csb(struct vas_window *window, csb.address = crb->stamp.nx.fault_storage_addr; csb.flags = 0; - pid = window->pid; + pid = window->task_ref.pid; tsk = get_pid_task(pid, PIDTYPE_PID); /* * Process closes send window after all pending NX requests are @@ -111,7 +111,7 @@ static void update_csb(struct vas_window *window, * a window and exits without closing it. */ if (!tsk) { - pid = window->tgid; + pid = window->task_ref.tgid; tsk = get_pid_task(pid, PIDTYPE_PID); /* * Parent thread (tgid) will be closing window when it @@ -127,7 +127,7 @@ static void update_csb(struct vas_window *window, return; } - kthread_use_mm(window->mm); + kthread_use_mm(window->task_ref.mm); rc = copy_to_user(csb_addr, &csb, sizeof(csb)); /* * User space polls on csb.flags (first byte). So add barrier @@ -139,7 +139,7 @@ static void update_csb(struct vas_window *window, smp_mb(); rc = copy_to_user(csb_addr, &csb, sizeof(u8)); } - kthread_unuse_mm(window->mm); + kthread_unuse_mm(window->task_ref.mm); put_task_struct(tsk); /* Success */ diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c index 3ccd3edcaf1a..ffd619e5a218 100644 --- a/arch/powerpc/platforms/powernv/vas-window.c +++ b/arch/powerpc/platforms/powernv/vas-window.c @@ -1065,51 +1065,9 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop, rc = -ENODEV; goto free_window; } - - /* - * 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. - */ - txwin->pid = get_task_pid(current, PIDTYPE_PID); - /* - * Acquire a reference to the task's mm. - */ - txwin->mm = get_task_mm(current); - - if (!txwin->mm) { - put_pid(txwin->pid); - pr_err("VAS: pid(%d): mm_struct is not found\n", - current->pid); - rc = -EPERM; + rc = vas_reference_pid_mm(&txwin->task_ref); + if (rc) goto free_window; - } - - mmgrab(txwin->mm); - mmput(txwin->mm); - mm_context_add_vas_window(txwin->mm); - /* - * Process closes window during exit. In the case of - * multithread application, the child thread can open - * window and can exit without closing it. Expects parent - * thread to use and close the window. So do not need - * to take pid reference for parent thread. - */ - txwin->tgid = find_get_pid(task_tgid_vnr(current)); - /* - * Even a process that has no foreign real address mapping can - * use an unpaired COPY instruction (to no real effect). Issue - * CP_ABORT to clear any pending COPY and prevent a covert - * channel. - * - * __switch_to() will issue CP_ABORT on future context switches - * if process / thread has any open VAS window (Use - * current->mm->context.vas_windows). - */ - asm volatile(PPC_CP_ABORT); } set_vinst_win(vinst, txwin); @@ -1339,14 +1297,9 @@ int vas_win_close(struct vas_window *window) /* if send window, drop reference to matching receive window */ if (window->tx_win) { - if (window->user_win) { - /* Drop references to pid and mm */ - put_pid(window->pid); - if (window->mm) { - mm_context_remove_vas_window(window->mm); - mmdrop(window->mm); - } - } + if (window->user_win) + vas_drop_reference_pid_mm(&window->task_ref); + put_rx_win(window->rxwin); } diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h index c7db3190baca..f354dd5c51bd 100644 --- a/arch/powerpc/platforms/powernv/vas.h +++ b/arch/powerpc/platforms/powernv/vas.h @@ -357,11 +357,9 @@ struct vas_window { bool user_win; /* True if user space window */ void *hvwc_map; /* HV window context */ void *uwc_map; /* OS/User window context */ - struct pid *pid; /* Linux process id of owner */ - struct pid *tgid; /* Thread group ID of owner */ - struct mm_struct *mm; /* Linux process mm_struct */ int wcreds_max; /* Window credits */ + struct vas_user_win_ref task_ref; char *dbgname; struct dentry *dbgdir; @@ -443,7 +441,7 @@ extern void vas_win_paste_addr(struct vas_window *window, u64 *addr, static inline int vas_window_pid(struct vas_window *window) { - return pid_vnr(window->pid); + return pid_vnr(window->task_ref.pid); } static inline void vas_log_write(struct vas_window *win, char *name,
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> --- 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(-)