Haren Myneni's on March 19, 2020 4:16 pm:
>
> When process opens a window, its pid and tgid will be saved in vas_window
> struct. This window will be closed when the process exits. Kernel handles
> NX faults by updating CSB or send SEGV signal to pid if user space csb_addr
> is invalid.
Bit of a nitpick, but can you use articles consistently ("the", "a")? I
won't keep nitpicking changelogs but I think they could be made easier
to read. I'm happy to help proof read and suggest things offline when
you're happy with the technical content of them, let me know.
>
> In multi-thread applications, a window can be opened by child thread, but
> it will not be closed when this thread exits. Expects parent to clean up
> all resources including NX windows. Child thread can send requests using
> this window and can be killed before they are completed. But the pid
> assigned to this thread can be reused for other task while requests are
> pending. If the csb_addr passed in these requests is invalid, kernel will
> end up sending signal to the wrong task.
>
> To prevent reusing the pid, take references to pid and mm when the window
> is opened and release them during window close.
We went over this together a while back, but task management isn't
something I look at every day and it's complicated and easy to introduce
bugs. I suggest if we can get the changelog and comments written well
and understandable for someone who does not know or care about vas,
then cc linux-kernel and the maintainers, and hopefully someone will
take a look. It's not a large patch so if assumptions and concurrency
etc is documented, then it shouldn't be too much work.
Thanks,
Nick
>
> Signed-off-by: Haren Myneni <ha...@linux.ibm.com>
> ---
> arch/powerpc/platforms/powernv/vas-debug.c | 2 +-
> arch/powerpc/platforms/powernv/vas-window.c | 53
> ++++++++++++++++++++++++++---
> arch/powerpc/platforms/powernv/vas.h | 9 ++++-
> 3 files changed, 57 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/vas-debug.c
> b/arch/powerpc/platforms/powernv/vas-debug.c
> index 09e63df..ef9a717 100644
> --- a/arch/powerpc/platforms/powernv/vas-debug.c
> +++ b/arch/powerpc/platforms/powernv/vas-debug.c
> @@ -38,7 +38,7 @@ static int info_show(struct seq_file *s, void *private)
>
> seq_printf(s, "Type: %s, %s\n", cop_to_str(window->cop),
> window->tx_win ? "Send" : "Receive");
> - seq_printf(s, "Pid : %d\n", window->pid);
> + seq_printf(s, "Pid : %d\n", vas_window_pid(window));
>
> unlock:
> mutex_unlock(&vas_mutex);
> diff --git a/arch/powerpc/platforms/powernv/vas-window.c
> b/arch/powerpc/platforms/powernv/vas-window.c
> index acb6a22..e7641a5 100644
> --- a/arch/powerpc/platforms/powernv/vas-window.c
> +++ b/arch/powerpc/platforms/powernv/vas-window.c
> @@ -12,6 +12,8 @@
> #include <linux/log2.h>
> #include <linux/rcupdate.h>
> #include <linux/cred.h>
> +#include <linux/sched/mm.h>
> +#include <linux/mmu_context.h>
> #include <asm/switch_to.h>
> #include <asm/ppc-opcode.h>
> #include "vas.h"
> @@ -876,8 +878,6 @@ struct vas_window *vas_rx_win_open(int vasid, enum
> vas_cop_type cop,
> rxwin->user_win = rxattr->user_win;
> rxwin->cop = cop;
> rxwin->wcreds_max = rxattr->wcreds_max ?: VAS_WCREDS_DEFAULT;
> - if (rxattr->user_win)
> - rxwin->pid = task_pid_vnr(current);
>
> init_winctx_for_rxwin(rxwin, rxattr, &winctx);
> init_winctx_regs(rxwin, &winctx);
> @@ -1027,7 +1027,6 @@ struct vas_window *vas_tx_win_open(int vasid, enum
> vas_cop_type cop,
> txwin->tx_win = 1;
> txwin->rxwin = rxwin;
> txwin->nx_win = txwin->rxwin->nx_win;
> - txwin->pid = attr->pid;
> txwin->user_win = attr->user_win;
> txwin->wcreds_max = attr->wcreds_max ?: VAS_WCREDS_DEFAULT;
>
> @@ -1068,8 +1067,43 @@ struct vas_window *vas_tx_win_open(int vasid, enum
> vas_cop_type cop,
> goto free_window;
> }
>
> - set_vinst_win(vinst, txwin);
> + if (txwin->user_win) {
> + /*
> + * Window opened by 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;
> + goto free_window;
> + }
> +
> + mmgrab(txwin->mm);
> + mmput(txwin->mm);
> + mm_context_add_copro(txwin->mm);
> + /*
> + * Process closes window during exit. In the case of
> + * multithread application, child 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));
> + }
> +
> + set_vinst_win(vinst, txwin);
> return txwin;
>
> free_window:
> @@ -1266,8 +1300,17 @@ int vas_win_close(struct vas_window *window)
> poll_window_castout(window);
>
> /* if send window, drop reference to matching receive window */
> - if (window->tx_win)
> + if (window->tx_win) {
> + if (window->user_win) {
> + /* Drop references to pid and mm */
> + put_pid(window->pid);
> + if (window->mm) {
> + mm_context_remove_copro(window->mm);
> + mmdrop(window->mm);
> + }
> + }
> put_rx_win(window->rxwin);
> + }
>
> vas_window_free(window);
>
> diff --git a/arch/powerpc/platforms/powernv/vas.h
> b/arch/powerpc/platforms/powernv/vas.h
> index 310b8a0..16aa8ec 100644
> --- a/arch/powerpc/platforms/powernv/vas.h
> +++ b/arch/powerpc/platforms/powernv/vas.h
> @@ -353,7 +353,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 */
> - pid_t pid; /* Linux process id of owner */
> + 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 */
>
> char *dbgname;
> @@ -431,6 +433,11 @@ struct vas_winctx {
> extern struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
> uint32_t pswid);
>
> +static inline int vas_window_pid(struct vas_window *window)
> +{
> + return pid_vnr(window->pid);
> +}
> +
> static inline void vas_log_write(struct vas_window *win, char *name,
> void *regptr, u64 val)
> {
> --
> 1.8.3.1
>
>
>
>
