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

[Xiong, Jianxin]

> -----Original Message-----
> From: Jason Gunthorpe <jgg at ziepe.ca>
> Sent: Thursday, November 05, 2020 4:09 PM
> To: Xiong, Jianxin <jianxin.xiong at intel.com>
> Cc: linux-rdma at vger.kernel.org; dri-devel at lists.freedesktop.org; Doug Ledford <dledford at redhat.com>; Leon Romanovsky
> <leon at kernel.org>; Sumit Semwal <sumit.semwal at linaro.org>; Christian Koenig <christian.koenig at amd.com>; Vetter, Daniel
> <daniel.vetter at intel.com>
> Subject: Re: [PATCH v8 1/5] RDMA/umem: Support importing dma-buf as user memory region
> 
> 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 

I can rework that.

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

In order to check the length, the buffer needs to be mapped. That can be done.

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

If ib_umem_dmabuf_map_pages is called during get this error is automatically caught.

> But yes, this is the right idea
> 
> Jason