linux-kernel - Re: [PATCH v4 2/2] habanalabs: add support for dma-buf exporter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFCwf100mkROMw9+2LgW7d3jKnaeZ4nmfWm7HtXuUE7NF4B8pg@mail.gmail.com>
Date:   Tue, 6 Jul 2021 12:44:49 +0300
From:   Oded Gabbay <oded.gabbay@...il.com>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     Oded Gabbay <ogabbay@...nel.org>,
        "Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Christian König <christian.koenig@....com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Gal Pressman <galpress@...zon.com>, sleybo@...zon.com,
        Maling list - DRI developers 
        <dri-devel@...ts.freedesktop.org>,
        linux-rdma <linux-rdma@...r.kernel.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Doug Ledford <dledford@...hat.com>,
        Dave Airlie <airlied@...il.com>,
        Alex Deucher <alexander.deucher@....com>,
        Leon Romanovsky <leonro@...dia.com>,
        Christoph Hellwig <hch@....de>,
        amd-gfx list <amd-gfx@...ts.freedesktop.org>,
        "moderated list:DMA BUFFER SHARING FRAMEWORK" 
        <linaro-mm-sig@...ts.linaro.org>, Tomer Tayar <ttayar@...ana.ai>
Subject: Re: [PATCH v4 2/2] habanalabs: add support for dma-buf exporter

On Mon, Jul 5, 2021 at 7:52 PM Jason Gunthorpe <jgg@...pe.ca> wrote:
>
> On Mon, Jul 05, 2021 at 04:03:14PM +0300, Oded Gabbay wrote:
>
> > +     rc = sg_alloc_table(*sgt, nents, GFP_KERNEL | __GFP_ZERO);
> > +     if (rc)
> > +             goto error_free;
>
> If you are not going to include a CPU list then I suggest setting
> sg_table->orig_nents == 0
>
> And using only the nents which is the length of the DMA list.
>
> At least it gives some hope that other parts of the system could
> detect this.

Will do.
>
> > +
> > +     /* Merge pages and put them into the scatterlist */
> > +     cur_page = 0;
> > +     for_each_sgtable_sg((*sgt), sg, i) {
>
> for_each_sgtable_sg should never be used when working with
> sg_dma_address() type stuff, here and everywhere else. The DMA list
> should be iterated using the for_each_sgtable_dma_sg() macro.

Thanks, will change that.

>
> > +     /* In case we got a large memory area to export, we need to divide it
> > +      * to smaller areas because each entry in the dmabuf sgt can only
> > +      * describe unsigned int.
> > +      */
>
> Huh? This is forming a SGL, it should follow the SGL rules which means
> you have to fragment based on the dma_get_max_seg_size() of the
> importer device.
>
hmm
I don't see anyone in drm checking this value (and using it) when
creating the SGL when exporting dmabuf. (e.g.
amdgpu_vram_mgr_alloc_sgt)
Are you sure we need this check ? Maybe I'm mistaken but if that's a
real concern, then most of the drm drivers are broken.

> > +     hl_dmabuf->pages = kcalloc(hl_dmabuf->npages, sizeof(*hl_dmabuf->pages),
> > +                                                             GFP_KERNEL);
> > +     if (!hl_dmabuf->pages) {
> > +             rc = -ENOMEM;
> > +             goto err_free_dmabuf_wrapper;
> > +     }
>
> Why not just create the SGL directly? Is there a reason it needs to
> make a page list?
Well the idea behind it was that we have two points of entry to this
code path (export dmabuf from address, and export dmabuf from handle)
and
we want that the map function later on will be agnostic to it and
behave the same in both cases.

Having said that, if we need to consider the max segment size
according to dma_get_max_seg_size() when creating the SGL, I agree
this code is redundant and I will remove it.

>
> Jason