linux-kernel - Re: [PATCH] IB/umem: use tgid instead of pid in ib

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180503153310.GA9738@ziepe.ca>
Date:   Thu, 3 May 2018 09:33:10 -0600
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Lidong Chen <jemmy858585@...il.com>
Cc:     dledford@...hat.com, akpm@...ux-foundation.org,
        qing.huang@...cle.com, leon@...nel.org, artemyko@...lanox.com,
        dan.j.williams@...el.com, linux-rdma@...r.kernel.org,
        linux-kernel@...r.kernel.org, adido@...lanox.com,
        galsha@...lanox.com, aviadye@...lanox.com,
        Lidong Chen <lidongchen@...cent.com>
Subject: Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

On Thu, May 03, 2018 at 10:04:34PM +0800, Lidong Chen wrote:
> The userspace may invoke ibv_reg_mr and ibv_dereg_mr by different threads.
> If when ibv_dereg_mr invoke and the thread which invoked ibv_reg_mr has
> exited, get_pid_task will return NULL, ib_umem_release does not decrease
> mm->pinned_vm. This patch fixes it by use tgid.
> 
> Signed-off-by: Lidong Chen <lidongchen@...cent.com>
> ---
>  drivers/infiniband/core/umem.c | 12 ++++++------
>  include/rdma/ib_umem.h         |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)

Why are we even using a struct pid for this? Does anyone know?

I'm surprised that struct task isn't held in the struct ib_umem..

Is group_leader really OK and always guarenteed to return the same
struct mm?? For some reason I have this recollection that the leader
can change under some situation..

> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> index 9a4e899..8813ba5 100644
> --- a/drivers/infiniband/core/umem.c
> +++ b/drivers/infiniband/core/umem.c
> @@ -119,7 +119,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
>  	umem->length     = size;
>  	umem->address    = addr;
>  	umem->page_shift = PAGE_SHIFT;
> -	umem->pid	 = get_task_pid(current, PIDTYPE_PID);
> +	umem->tgid	 = get_task_pid(current->group_leader, PIDTYPE_PID);
>  	/*
>  	 * We ask for writable memory if any of the following
>  	 * access flags are set.  "Local write" and "remote write"
> @@ -132,7 +132,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
>  		 IB_ACCESS_REMOTE_ATOMIC | IB_ACCESS_MW_BIND));
>  
>  	if (access & IB_ACCESS_ON_DEMAND) {
> -		put_pid(umem->pid);
> +		put_pid(umem->tgid);
>  		ret = ib_umem_odp_get(context, umem, access);
>  		if (ret) {
>  			kfree(umem);
> @@ -148,7 +148,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
>  
>  	page_list = (struct page **) __get_free_page(GFP_KERNEL);
>  	if (!page_list) {
> -		put_pid(umem->pid);
> +		put_pid(umem->tgid);
>  		kfree(umem);
>  		return ERR_PTR(-ENOMEM);
>  	}
> @@ -231,7 +231,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
>  	if (ret < 0) {
>  		if (need_release)
>  			__ib_umem_release(context->device, umem, 0);
> -		put_pid(umem->pid);
> +		put_pid(umem->tgid);
>  		kfree(umem);
>  	} else
>  		current->mm->pinned_vm = locked;
> @@ -274,8 +274,8 @@ void ib_umem_release(struct ib_umem *umem)
>  
>  	__ib_umem_release(umem->context->device, umem, 1);
>  
> -	task = get_pid_task(umem->pid, PIDTYPE_PID);
> -	put_pid(umem->pid);
> +	task = get_pid_task(umem->tgid, PIDTYPE_PID);
> +	put_pid(umem->tgid);
>  	if (!task)
>  		goto out;
>  	mm = get_task_mm(task);
> diff --git a/include/rdma/ib_umem.h b/include/rdma/ib_umem.h
> index 23159dd..2398849 100644
> --- a/include/rdma/ib_umem.h
> +++ b/include/rdma/ib_umem.h
> @@ -48,7 +48,7 @@ struct ib_umem {
>  	int                     writable;
>  	int                     hugetlb;
>  	struct work_struct	work;
> -	struct pid             *pid;
> +	struct pid             *tgid;
>  	struct mm_struct       *mm;
>  	unsigned long		diff;
>  	struct ib_umem_odp     *odp_data;
> -- 
> 1.8.3.1
>