On Thu, 2019-12-12 at 05:02 -0800, Christoph Hellwig wrote:
> > + 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.
> > + */
> > + 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) {
> > + mmput(txwin->mm);
> > + mmgrab(txwin->mm);
>
> Doesn't the mmgrab need to come before the mmput?
>
> > + mm_context_add_copro(txwin->mm);
> > + } else {
> > + put_pid(txwin->pid);
> > + pr_err("VAS: pid(%d): mm_struct is not found\n",
> > + current->pid);
> > + rc = -EPERM;
> > + goto free_window;
> > + }
>
> Also the code is much easier to follow if you handle the error
> first and avoid the else:
>
> 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);
>
> Also don't you need to take a reference to the struct pid for the
> tgid as well?
Process opens window and closed it upon its exit. We do not have to take
pid reference in this case. But in multithread applications, child
thread can open window, can exit without closing it. Parent thread can
use this window and expects to close it later. So in this case pid
reference for child thread is needed. Where as parent thread is the one
who is closing the window later, do not to have to take its pid
reference.
Basically we are taking pid reference to the process who opens the
window to cover multithread applications.
Thanks for your review comments. I will post V3 patches.
