On Thu, Jul 11, 2019 at 09:32:17AM +0800, Lijun Ou wrote:
> From: Xi Wang <wangxi11@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
