> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Thursday, November 05, 2020 4:09 PM
> To: Xiong, Jianxin <jianxin.xiong@xxxxxxxxx>
> Cc: linux-rdma@xxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky
> <leon@xxxxxxxxxx>; Sumit Semwal <sumit.semwal@xxxxxxxxxx>; Christian Koenig <christian.koenig@xxxxxxx>; Vetter, Daniel
> <daniel.vetter@xxxxxxxxx>
> 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
