[PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[Lidong Chen]

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@tencent.com>
---
 drivers/infiniband/core/umem.c | 12 ++++++------
 include/rdma/ib_umem.h         |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

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

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[Jason Gunthorpe]

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@tencent.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
> 

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

On Thu, May 03, 2018 at 09:33:10AM -0600, Jason Gunthorpe wrote:
> 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@tencent.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?
>

Can it be related to "fork" support?

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

I think that this code can be removed and all accesses to mm_struct can
be done with "current->mm".

Thanks

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[Jason Gunthorpe]

On Thu, May 03, 2018 at 09:12:35PM +0300, Leon Romanovsky wrote:
> On Thu, May 03, 2018 at 09:33:10AM -0600, Jason Gunthorpe wrote:
> > 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@tencent.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?
> >
> 
> Can it be related to "fork" support?

Not sure..

Ideally we want to hold the struct mm, but we can't hold it long
term, so pid is a surrogate for that.

> > I'm surprised that struct task isn't held in the struct ib_umem..
> >
> 
> I think that this code can be removed and all accesses to mm_struct can
> be done with "current->mm".

That sounds wrong for fork support, as the mm used in destroy MUST
exactly match the mm used in create..

How does this accounting work in fork anyhow?

Jason

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

On Thu, May 03, 2018 at 12:26:56PM -0600, Jason Gunthorpe wrote:
> On Thu, May 03, 2018 at 09:12:35PM +0300, Leon Romanovsky wrote:
> > On Thu, May 03, 2018 at 09:33:10AM -0600, Jason Gunthorpe wrote:
> > > 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@tencent.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?
> > >
> >
> > Can it be related to "fork" support?
>
> Not sure..
>
> Ideally we want to hold the struct mm, but we can't hold it long
> term, so pid is a surrogate for that.
>
> > > I'm surprised that struct task isn't held in the struct ib_umem..
> > >
> >
> > I think that this code can be removed and all accesses to mm_struct can
> > be done with "current->mm".
>
> That sounds wrong for fork support, as the mm used in destroy MUST
> exactly match the mm used in create..
>
> How does this accounting work in fork anyhow?

We are not supporting fork, so this is why I proposed to remove it.

Thanks

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[Jason Gunthorpe]

On Thu, May 03, 2018 at 09:43:01PM +0300, Leon Romanovsky wrote:
> On Thu, May 03, 2018 at 12:26:56PM -0600, Jason Gunthorpe wrote:
> > On Thu, May 03, 2018 at 09:12:35PM +0300, Leon Romanovsky wrote:
> > > On Thu, May 03, 2018 at 09:33:10AM -0600, Jason Gunthorpe wrote:
> > > > 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@tencent.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?
> > > >
> > >
> > > Can it be related to "fork" support?
> >
> > Not sure..
> >
> > Ideally we want to hold the struct mm, but we can't hold it long
> > term, so pid is a surrogate for that.
> >
> > > > I'm surprised that struct task isn't held in the struct ib_umem..
> > > >
> > >
> > > I think that this code can be removed and all accesses to mm_struct can
> > > be done with "current->mm".
> >
> > That sounds wrong for fork support, as the mm used in destroy MUST
> > exactly match the mm used in create..
> >
> > How does this accounting work in fork anyhow?
> 
> We are not supporting fork, so this is why I proposed to remove it.

Er, the new kabi certainly can call reg and dereg across a fork

Jason

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[858585 jemmy]

On Thu, May 3, 2018 at 11:33 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 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@tencent.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..

I read kernel code, it seems that the userspace thread can guarantee
to return the same struct mm.
in the copy_process function, it checks the CLONE_THREAD,
CLONE_SIGHAND, CLONE_VM must use
at the same time.
And I do not find any other part code change the mm and group_leader
for userspace thread.

>
>> 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
>>

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[858585 jemmy]

On Thu, May 3, 2018 at 11:33 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> 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@tencent.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?

commit 87773dd56d5405ac28119fcfadacefd35877c18f add pid in ib_umem structure.

and the comment has such information:
Later a different process with a different mm_struct than the one that
allocated the ib_umem struct
ends up releasing it which results in decrementing the new processes
mm->pinned_vm count past
zero and wrapping.

>
> 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
>>

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[858585 jemmy]

On Fri, May 4, 2018 at 6:01 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Thu, May 03, 2018 at 09:43:01PM +0300, Leon Romanovsky wrote:
>> On Thu, May 03, 2018 at 12:26:56PM -0600, Jason Gunthorpe wrote:
>> > On Thu, May 03, 2018 at 09:12:35PM +0300, Leon Romanovsky wrote:
>> > > On Thu, May 03, 2018 at 09:33:10AM -0600, Jason Gunthorpe wrote:
>> > > > 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@tencent.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?
>> > > >
>> > >
>> > > Can it be related to "fork" support?
>> >
>> > Not sure..
>> >
>> > Ideally we want to hold the struct mm, but we can't hold it long
>> > term, so pid is a surrogate for that.
>> >
>> > > > I'm surprised that struct task isn't held in the struct ib_umem..
>> > > >
>> > >
>> > > I think that this code can be removed and all accesses to mm_struct can
>> > > be done with "current->mm".
>> >
>> > That sounds wrong for fork support, as the mm used in destroy MUST
>> > exactly match the mm used in create..
>> >
>> > How does this accounting work in fork anyhow?
>>
>> We are not supporting fork, so this is why I proposed to remove it.
>
> Er, the new kabi certainly can call reg and dereg across a fork

what is the expect behavior after fork?
I write a test code, the dereg just return EACCES in the child
process. and have no effect.

>
> Jason

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[858585 jemmy]

On Fri, May 4, 2018 at 11:14 AM, 858585 jemmy <jemmy858585@gmail.com> wrote:
> On Thu, May 3, 2018 at 11:33 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>> 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@tencent.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?
>
> commit 87773dd56d5405ac28119fcfadacefd35877c18f add pid in ib_umem structure.
>
> and the comment has such information:
> Later a different process with a different mm_struct than the one that
> allocated the ib_umem struct
> ends up releasing it which results in decrementing the new processes
> mm->pinned_vm count past
> zero and wrapping.
>

I think a different process should not have the permission to release ib_umem.
so maybe the reason is not a different process?
can ib_umem_release be invoked in interrupt context?

>>
>> 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
>>>

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]

On Fri, May 04, 2018 at 04:32:38PM +0800, 858585 jemmy wrote:
> On Fri, May 4, 2018 at 6:01 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > On Thu, May 03, 2018 at 09:43:01PM +0300, Leon Romanovsky wrote:
> >> On Thu, May 03, 2018 at 12:26:56PM -0600, Jason Gunthorpe wrote:
> >> > On Thu, May 03, 2018 at 09:12:35PM +0300, Leon Romanovsky wrote:
> >> > > On Thu, May 03, 2018 at 09:33:10AM -0600, Jason Gunthorpe wrote:
> >> > > > 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@tencent.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?
> >> > > >
> >> > >
> >> > > Can it be related to "fork" support?
> >> >
> >> > Not sure..
> >> >
> >> > Ideally we want to hold the struct mm, but we can't hold it long
> >> > term, so pid is a surrogate for that.
> >> >
> >> > > > I'm surprised that struct task isn't held in the struct ib_umem..
> >> > > >
> >> > >
> >> > > I think that this code can be removed and all accesses to mm_struct can
> >> > > be done with "current->mm".
> >> >
> >> > That sounds wrong for fork support, as the mm used in destroy MUST
> >> > exactly match the mm used in create..
> >> >
> >> > How does this accounting work in fork anyhow?
> >>
> >> We are not supporting fork, so this is why I proposed to remove it.
> >
> > Er, the new kabi certainly can call reg and dereg across a fork
>
> what is the expect behavior after fork?
> I write a test code, the dereg just return EACCES in the child
> process. and have no effect.

Did you do reg/dereg over write() interface? If yes, this is expected
behaviour of "not-supported fork()". A couple of months/years ago, your
test program would work, but we closed this option due to security
constraints.

Thanks

>
> >
> > Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[lidongchen(陈立东)]

> 在 2018年5月4日，21:39，Leon Romanovsky <leon@kernel.org> 写道：
> 
>> On Fri, May 04, 2018 at 04:32:38PM +0800, 858585 jemmy wrote:
>>> On Fri, May 4, 2018 at 6:01 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>>>> On Thu, May 03, 2018 at 09:43:01PM +0300, Leon Romanovsky wrote:
>>>>> On Thu, May 03, 2018 at 12:26:56PM -0600, Jason Gunthorpe wrote:
>>>>>> On Thu, May 03, 2018 at 09:12:35PM +0300, Leon Romanovsky wrote:
>>>>>>> On Thu, May 03, 2018 at 09:33:10AM -0600, Jason Gunthorpe wrote:
>>>>>>>> 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@tencent.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?
>>>>>>> 
>>>>>> 
>>>>>> Can it be related to "fork" support?
>>>>> 
>>>>> Not sure..
>>>>> 
>>>>> Ideally we want to hold the struct mm, but we can't hold it long
>>>>> term, so pid is a surrogate for that.
>>>>> 
>>>>>>> I'm surprised that struct task isn't held in the struct ib_umem..
>>>>>>> 
>>>>>> 
>>>>>> I think that this code can be removed and all accesses to mm_struct can
>>>>>> be done with "current->mm".
>>>>> 
>>>>> That sounds wrong for fork support, as the mm used in destroy MUST
>>>>> exactly match the mm used in create..
>>>>> 
>>>>> How does this accounting work in fork anyhow?
>>>> 
>>>> We are not supporting fork, so this is why I proposed to remove it.
>>> 
>>> Er, the new kabi certainly can call reg and dereg across a fork
>> 
>> what is the expect behavior after fork?
>> I write a test code, the dereg just return EACCES in the child
>> process. and have no effect.
> 
> Did you do reg/dereg over write() interface? If yes, this is expected
> behaviour of "not-supported fork()". A couple of months/years ago, your
> test program would work, but we closed this option due to security
> constraints.

the parent process call ibv_reg_mr, and the child process call ibv_dereg_mr.

If fork is not supported now, so use tgid to get mm structure is fine for multithread.

> 
> Thanks
> 
>> 
>>> 
>>> Jason

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[Jason Gunthorpe]

On Fri, May 04, 2018 at 04:51:15PM +0800, 858585 jemmy wrote:
> On Fri, May 4, 2018 at 11:14 AM, 858585 jemmy <jemmy858585@gmail.com> wrote:
> > On Thu, May 3, 2018 at 11:33 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >> 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@tencent.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?
> >
> > commit 87773dd56d5405ac28119fcfadacefd35877c18f add pid in ib_umem structure.
> >
> > and the comment has such information:
> > Later a different process with a different mm_struct than the one that
> > allocated the ib_umem struct
> > ends up releasing it which results in decrementing the new processes
> > mm->pinned_vm count past
> > zero and wrapping.
> 
> I think a different process should not have the permission to release ib_umem.
> so maybe the reason is not a different process?
> can ib_umem_release be invoked in interrupt context?

We plan to restore fork support and add some way to share MRs between
processes, so we must consider having a different process release the
umem than acquired it.

Jason

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[858585 jemmy]

On Sat, May 5, 2018 at 2:23 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Fri, May 04, 2018 at 04:51:15PM +0800, 858585 jemmy wrote:
>> On Fri, May 4, 2018 at 11:14 AM, 858585 jemmy <jemmy858585@gmail.com> wrote:
>> > On Thu, May 3, 2018 at 11:33 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>> >> 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@tencent.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?
>> >
>> > commit 87773dd56d5405ac28119fcfadacefd35877c18f add pid in ib_umem structure.
>> >
>> > and the comment has such information:
>> > Later a different process with a different mm_struct than the one that
>> > allocated the ib_umem struct
>> > ends up releasing it which results in decrementing the new processes
>> > mm->pinned_vm count past
>> > zero and wrapping.
>>
>> I think a different process should not have the permission to release ib_umem.
>> so maybe the reason is not a different process?
>> can ib_umem_release be invoked in interrupt context?
>
> We plan to restore fork support and add some way to share MRs between
> processes, so we must consider having a different process release the
> umem than acquired it.

If restore fork support, what is the expected behavior?
If parent process pinned_vm is x, what is the child process pinned_vm
value after fork? It reset to zero now.
If the parent process call ibv_dereg_mr after fork, should the child
process decrease pinned_vm?
If the child process call  ibv_dereg_mr after fork, should the parent
process decrease pinned_vm?


>
> Jason

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[Jason Gunthorpe]

On Mon, May 07, 2018 at 09:38:53AM +0800, 858585 jemmy wrote:
> On Sat, May 5, 2018 at 2:23 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > On Fri, May 04, 2018 at 04:51:15PM +0800, 858585 jemmy wrote:
> >> On Fri, May 4, 2018 at 11:14 AM, 858585 jemmy <jemmy858585@gmail.com> wrote:
> >> > On Thu, May 3, 2018 at 11:33 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >> >> 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@tencent.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?
> >> >
> >> > commit 87773dd56d5405ac28119fcfadacefd35877c18f add pid in ib_umem structure.
> >> >
> >> > and the comment has such information:
> >> > Later a different process with a different mm_struct than the one that
> >> > allocated the ib_umem struct
> >> > ends up releasing it which results in decrementing the new processes
> >> > mm->pinned_vm count past
> >> > zero and wrapping.
> >>
> >> I think a different process should not have the permission to release ib_umem.
> >> so maybe the reason is not a different process?
> >> can ib_umem_release be invoked in interrupt context?
> >
> > We plan to restore fork support and add some way to share MRs between
> > processes, so we must consider having a different process release the
> > umem than acquired it.
> 
> If restore fork support, what is the expected behavior?
> If parent process pinned_vm is x, what is the child process pinned_vm
> value after fork? It reset to zero now.
> If the parent process call ibv_dereg_mr after fork, should the child
> process decrease pinned_vm?
> If the child process call  ibv_dereg_mr after fork, should the parent
> process decrease pinned_vm?

If I recall the purpose of accessing the MM during de-register is to
undo the pinned pages change (pinned_vm) that register performed.

So, the semantic is simple, during deregister we must access excatly
the same MM that was used during register and undo the change to
pinned_vm.

The approach should be to find the most reliably way to hold a
reference to the MM that was used during register.

Apparently we can't just hold a ref on the mm (according to mm_get's
comment at least)

tgid is clearly a better indirect reference to the mm than pid (pid is
so obviously wrong)

But I am wondering why not just hold struct task here instead of tgid?
Isn't task->mm going to be more reliably than tgid->task->mm ??

Jason

Re: [PATCH] IB/umem: use tgid instead of pid in ib_umem structure

[858585 jemmy]

On Tue, May 8, 2018 at 2:30 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> On Mon, May 07, 2018 at 09:38:53AM +0800, 858585 jemmy wrote:
>> On Sat, May 5, 2018 at 2:23 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>> > On Fri, May 04, 2018 at 04:51:15PM +0800, 858585 jemmy wrote:
>> >> On Fri, May 4, 2018 at 11:14 AM, 858585 jemmy <jemmy858585@gmail.com> wrote:
>> >> > On Thu, May 3, 2018 at 11:33 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>> >> >> 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@tencent.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?
>> >> >
>> >> > commit 87773dd56d5405ac28119fcfadacefd35877c18f add pid in ib_umem structure.
>> >> >
>> >> > and the comment has such information:
>> >> > Later a different process with a different mm_struct than the one that
>> >> > allocated the ib_umem struct
>> >> > ends up releasing it which results in decrementing the new processes
>> >> > mm->pinned_vm count past
>> >> > zero and wrapping.
>> >>
>> >> I think a different process should not have the permission to release ib_umem.
>> >> so maybe the reason is not a different process?
>> >> can ib_umem_release be invoked in interrupt context?
>> >
>> > We plan to restore fork support and add some way to share MRs between
>> > processes, so we must consider having a different process release the
>> > umem than acquired it.
>>
>> If restore fork support, what is the expected behavior?
>> If parent process pinned_vm is x, what is the child process pinned_vm
>> value after fork? It reset to zero now.
>> If the parent process call ibv_dereg_mr after fork, should the child
>> process decrease pinned_vm?
>> If the child process call  ibv_dereg_mr after fork, should the parent
>> process decrease pinned_vm?
>
> If I recall the purpose of accessing the MM during de-register is to
> undo the pinned pages change (pinned_vm) that register performed.
>
> So, the semantic is simple, during deregister we must access excatly
> the same MM that was used during register and undo the change to
> pinned_vm.
>
> The approach should be to find the most reliably way to hold a
> reference to the MM that was used during register.
>
> Apparently we can't just hold a ref on the mm (according to mm_get's
> comment at least)
>
> tgid is clearly a better indirect reference to the mm than pid (pid is
> so obviously wrong)
>
> But I am wondering why not just hold struct task here instead of tgid?
> Isn't task->mm going to be more reliably than tgid->task->mm ??

I think get_task_struct(current->group_leader) is also work.
But I find ib_ucontext structure already have a tgid field, so I think this not
necessary to ib_umem have tgid again. we can use ib_ucontext->tgid.

I will send a v2 patch.


>
> Jason