Re: [PATCH for-rc] RDMA/hns: Fix sg offset non-zero issue

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Jul 11, 2019 at 09:32:17AM +0800, Lijun Ou wrote:
> From: Xi Wang <wangxi11@huawei.com>
> 
> When run perftest in many times, the system will report a BUG as follows:
> 
> [ 2312.559759] BUG: Bad rss-counter state mm:(____ptrval____) idx:0 val:-1
> [ 2312.574803] BUG: Bad rss-counter state mm:(____ptrval____) idx:1 val:1
> 
> We tested with different kernel version and found it started from the the
> following commit:
> 
> commit d10bcf947a3e ("RDMA/umem: Combine contiguous PAGE_SIZE regions in
> SGEs")
> 
> In this commit, the sg->offset is always 0 when sg_set_page() is called in
> ib_umem_get() and the drivers are not allowed to change the sgl, otherwise
> it will get bad page descriptor when unfolding SGEs in __ib_umem_release()
> as sg_page_count() will get wrong result while sgl->offset is not 0.
> 
> However, there is a weird sgl usage in the current hns driver, the driver
> modified sg->offset after calling ib_umem_get(), which caused we iterate
> past the wrong number of pages in for_each_sg_page iterator.
> 
> This patch fixes it by correcting the non-standard sgl usage found in the
> hns_roce_db_map_user() function.
> 
> Fixes: 0425e3e6e0c7 ("RDMA/hns: Support flush cqe for hip08 in kernel space")
> Signed-off-by: Xi Wang <wangxi11@huawei.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_db.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)

This looks like the right fix to the reported problem
 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_db.c b/drivers/infiniband/hw/hns/hns_roce_db.c
> index 0c6c1fe..d60453e 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_db.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_db.c
> @@ -12,13 +12,15 @@ int hns_roce_db_map_user(struct hns_roce_ucontext *context,
>  			 struct ib_udata *udata, unsigned long virt,
>  			 struct hns_roce_db *db)
>  {
> +	unsigned long page_addr = virt & PAGE_MASK;
>  	struct hns_roce_user_db_page *page;
> +	unsigned int offset;
>  	int ret = 0;
>  
>  	mutex_lock(&context->page_mutex);
>  
>  	list_for_each_entry(page, &context->page_list, list)
> -		if (page->user_virt == (virt & PAGE_MASK))
> +		if (page->user_virt == page_addr)
>  			goto found;
>  
>  	page = kmalloc(sizeof(*page), GFP_KERNEL);
> @@ -28,8 +30,8 @@ int hns_roce_db_map_user(struct hns_roce_ucontext *context,
>  	}
>  
>  	refcount_set(&page->refcount, 1);
> -	page->user_virt = (virt & PAGE_MASK);
> -	page->umem = ib_umem_get(udata, virt & PAGE_MASK, PAGE_SIZE, 0, 0);
> +	page->user_virt = page_addr;
> +	page->umem = ib_umem_get(udata, page_addr, PAGE_SIZE, 0, 0);
>  	if (IS_ERR(page->umem)) {
>  		ret = PTR_ERR(page->umem);
>  		kfree(page);
> @@ -39,10 +41,9 @@ int hns_roce_db_map_user(struct hns_roce_ucontext *context,
>  	list_add(&page->list, &context->page_list);
>  
>  found:
> -	db->dma = sg_dma_address(page->umem->sg_head.sgl) +
> -		  (virt & ~PAGE_MASK);
> -	page->umem->sg_head.sgl->offset = virt & ~PAGE_MASK;
> -	db->virt_addr = sg_virt(page->umem->sg_head.sgl);
> +	offset = virt - page_addr;
> +	db->dma = sg_dma_address(page->umem->sg_head.sgl) + offset;
> +	db->virt_addr = sg_virt(page->umem->sg_head.sgl) + offset;

However the use of sg_virt here is wrong. Please send a patch fixing
it.

You need to store the struct page * in the db and use kmap when you
want to access it.

Better would have been to create a shared kernel/user page via mmap
like the other drivers do.

Jason