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: <20210705165226.GJ4604@ziepe.ca>
Date:   Mon, 5 Jul 2021 13:52:26 -0300
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Oded Gabbay <ogabbay@...nel.org>
Cc:     linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        sumit.semwal@...aro.org, christian.koenig@....com,
        daniel.vetter@...ll.ch, galpress@...zon.com, sleybo@...zon.com,
        dri-devel@...ts.freedesktop.org, linux-rdma@...r.kernel.org,
        linux-media@...r.kernel.org, dledford@...hat.com,
        airlied@...il.com, alexander.deucher@....com, leonro@...dia.com,
        hch@....de, amd-gfx@...ts.freedesktop.org,
        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 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.

> +
> +	/* 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.

> +	/* 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.

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

Jason