Re: [PATCH v8 1/5] RDMA/umem: Support importing dma-buf as user memory region

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Nov 05, 2020 at 02:48:05PM -0800, Jianxin Xiong wrote:
> +	/* modify the sgl in-place to match umem address and length */
> +
> +	start = ALIGN_DOWN(umem_dmabuf->umem.address, PAGE_SIZE);
> +	end = ALIGN(umem_dmabuf->umem.address + umem_dmabuf->umem.length,
> +		    PAGE_SIZE);
> +	cur = 0;
> +	nmap = 0;
> +	for_each_sgtable_dma_sg(sgt, sg, i) {
> +		if (cur >= end)
> +			break;
> +		if (cur + sg_dma_len(sg) <= start) {
> +			cur += sg_dma_len(sg);
> +			continue;
> +		}

This seems like a strange way to compute interesections

  if (cur <= start && start < cur + sg_dma_len(sg))

> +		if (cur <= start) {
> +			unsigned long offset = start - cur;
> +
> +			umem_dmabuf->first_sg = sg;
> +			umem_dmabuf->first_sg_offset = offset;
> +			sg_dma_address(sg) += offset;
> +			sg_dma_len(sg) -= offset;
> +			if (&sg_dma_len(sg) != &sg->length)
> +				sg->length -= offset;

We don't need to adjust sg->length, only dma_len, so no reason for
this surprising if.

> +			cur += offset;
> +		}
> +		if (cur + sg_dma_len(sg) >= end) {

Same logic here

> +			unsigned long trim = cur + sg_dma_len(sg) - end;
> +
> +			umem_dmabuf->last_sg = sg;
> +			umem_dmabuf->last_sg_trim = trim;
> +			sg_dma_len(sg) -= trim;
> +			if (&sg_dma_len(sg) != &sg->length)
> +				sg->length -= trim;

break, things are done here

> +		}
> +		cur += sg_dma_len(sg);
> +		nmap++;
> +	}

> +	
> +	umem_dmabuf->umem.sg_head.sgl = umem_dmabuf->first_sg;
> +	umem_dmabuf->umem.sg_head.nents = nmap;
> +	umem_dmabuf->umem.nmap = nmap;
> +	umem_dmabuf->sgt = sgt;
> +
> +	page_size = ib_umem_find_best_pgsz(&umem_dmabuf->umem, PAGE_SIZE,
> +					   umem_dmabuf->umem.iova);
> +
> +	if (WARN_ON(cur != end || page_size != PAGE_SIZE)) {

Looks like nothing prevents this warn on to tigger

The user could specify a length that is beyond
the dma buf, can the dma buf length be checked during get?

Also page_size can be 0 because iova is not OK. iova should be checked
for alignment during get as well:

  iova & (PAGE_SIZE-1) == umem->addr & (PAGE_SIZE-1)

But yes, this is the right idea

Jason