| 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
| ||
|
Message-ID: <DU2PR02MB75736A59D16334B016BFACF8D20F9@DU2PR02MB7573.eurprd02.prod.outlook.com>
Date: Wed, 16 Jun 2021 21:16:08 +0000
From: Tomer Tayar <ttayar@...ana.ai>
To: Gal Pressman <galpress@...zon.com>,
Oded Gabbay <ogabbay@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Subject: RE: [PATCH v2 2/2] habanalabs: add support for dma-buf exporter
On 16/06/2021 17:59, Gal Pressman wrote:
> On 15/06/2021 11:53, Oded Gabbay wrote:
> > @@ -1346,9 +1372,11 @@ struct hl_ctx {
> > struct mutex mem_hash_lock;
> > struct mutex mmu_lock;
> > struct mutex hw_block_list_lock;
> > + struct mutex dmabuf_list_lock;
> > struct list_head debugfs_list;
> > struct list_head pending_cb_list;
> > struct list_head hw_block_mem_list;
> > + struct list_head dmabuf_list;
>
> Am I missing where this list is used? I only see insertions and deletions, but
> what is it used for?
You are right, we will remove it.
> > +static int export_dmabuf_from_addr(struct hl_ctx *ctx, u64 device_addr,
> > + u64 size, int flags, int *dmabuf_fd) {
> > + struct hl_device *hdev = ctx->hdev;
> > + struct asic_fixed_properties *prop = &hdev->asic_prop;
> > + struct hl_dmabuf_wrapper *hl_dmabuf;
> > + int rc;
> > +
> > + dev_dbg(hdev->dev, "exporting dma-buf for addr 0x%llx, size
> 0x%llx\n",
> > + device_addr, size);
> > +
> > + if (!IS_ALIGNED(device_addr,
> DMA_BUF_MEM_ADDR_ALIGNMENT)) {
> > + dev_err(hdev->dev,
> > + "address of exported device memory should be
> aligned to 0x%x, address 0x%llx\n",
> > + DMA_BUF_MEM_ADDR_ALIGNMENT, device_addr);
> > + return -EINVAL;
> > + }
> > +
> > + if (!size) {
> > + dev_err(hdev->dev,
> > + "size of exported device memory should be greater
> than 0\n");
>
> These cases can be caused by user providing wrong input, right? Maybe
> debug level is more appropriate to prevent spamming dmesg.
As in other places in the driver, we do want to have these dev_err prints.
However it is true that we should at least use the *_ratelimited version.
Will modify it.
> > + return -EINVAL;
> > + }
> > +
> > + if (!IS_ALIGNED(size, DMA_BUF_MEM_SIZE_ALIGNMENT)) {
> > + dev_err(hdev->dev,
> > + "size of exported device memory should be aligned
> to 0x%x, size 0x%llx\n",
> > + DMA_BUF_MEM_SIZE_ALIGNMENT, device_addr);
> > + return -EINVAL;
> > + }
> > +
> > + if (device_addr < prop->dram_user_base_address ||
> > + device_addr + size > prop->dram_end_address) {
> > + dev_err(hdev->dev,
> > + "DRAM memory range is outside of DRAM
> boundaries, address 0x%llx, size 0x%llx\n",
> > + device_addr, size);
> > + return -EINVAL;
> > + }
> > +
> > + hl_dmabuf = kzalloc(sizeof(*hl_dmabuf), GFP_KERNEL);
> > + if (!hl_dmabuf)
> > + return -ENOMEM;
> > +
> > + hl_dmabuf->handle = device_addr;
> > + hl_dmabuf->total_size = size;
> > +
> > + rc = export_dmabuf_common(ctx, hl_dmabuf, &device_addr, 1, size,
> flags,
> > + dmabuf_fd);
> > + if (rc)
> > + goto err_free_dmabuf_wrapper;
> > +
> > + return 0;
> > +
> > +err_free_dmabuf_wrapper:
> > + kfree(hl_dmabuf);
> > + return rc;
> > +}
> > +
> > +/**
> > + * export_dmabuf_from_handle() - export a dma-buf object for the given
> memory
> > + * handle.
> > + * @ctx: pointer to the context structure.
> > + * @handle: device memory allocation handle.
> > + * @flags: DMA-BUF file/FD flags.
> > + * @dmabuf_fd: pointer to result FD that represents the dma-buf object.
> > + *
> > + * Create and export a dma-buf object for an existing memory
> > +allocation inside
> > + * the device memory, and return a FD which is associated with the
> > +dma-buf
> > + * object.
> > + *
> > + * Return: 0 on success, non-zero for failure.
> > + */
> > +static int export_dmabuf_from_handle(struct hl_ctx *ctx, u64 handle, int
> flags,
> > + int *dmabuf_fd)
> > +{
> > + struct hl_device *hdev = ctx->hdev;
> > + struct hl_vm_phys_pg_pack *phys_pg_pack;
> > + struct hl_dmabuf_wrapper *hl_dmabuf;
> > + struct hl_vm *vm = &hdev->vm;
> > + enum vm_type_t *vm_type;
> > + u32 idr_handle;
> > + int rc;
> > +
> > + dev_dbg(hdev->dev, "exporting dma-buf for handle 0x%llx\n",
> handle);
> > +
> > + idr_handle = lower_32_bits(handle);
> > +
> > + spin_lock(&vm->idr_lock);
> > +
> > + phys_pg_pack = idr_find(&vm->phys_pg_pack_handles,
> idr_handle);
> > + if (!phys_pg_pack) {
> > + spin_unlock(&vm->idr_lock);
> > + dev_err(hdev->dev, "no match for handle 0x%x\n",
> idr_handle);
> > + return -EINVAL;
> > + }
> > +
> > + /* increment now to avoid freeing device memory while exporting */
> > + atomic_inc(&phys_pg_pack->exporting_cnt);
> > +
> > + spin_unlock(&vm->idr_lock);
> > +
> > + vm_type = (enum vm_type_t *) phys_pg_pack;
>
> Why is this casting a struct instead of accessing its first field?
The casting is not needed ... we will fix it, thanks.
> > + if (*vm_type != VM_TYPE_PHYS_PACK) {
> > + dev_err(hdev->dev, "handle 0x%llx is not for DRAM
> memory\n",
> > + handle);
> > + rc = -EINVAL;
> > + goto err_dec_exporting_cnt;
> > + }
> > +
> > + hl_dmabuf = kzalloc(sizeof(*hl_dmabuf), GFP_KERNEL);
> > + if (!hl_dmabuf) {
> > + rc = -ENOMEM;
> > + goto err_dec_exporting_cnt;
> > + }
> > +
> > + hl_dmabuf->phys_pg_pack = phys_pg_pack;
> > + hl_dmabuf->handle = handle;
> > + hl_dmabuf->total_size = phys_pg_pack->total_size;
> > +
> > + rc = export_dmabuf_common(ctx, hl_dmabuf, phys_pg_pack-
> >pages,
> > + phys_pg_pack->npages, phys_pg_pack-
> >page_size,
> > + flags, dmabuf_fd);
> > + if (rc)
> > + goto err_free_dmabuf_wrapper;
> > +
> > + return 0;
> > +
> > +err_free_dmabuf_wrapper:
> > + kfree(hl_dmabuf);
> > +err_dec_exporting_cnt:
> > + atomic_dec(&phys_pg_pack->exporting_cnt);
> > + return rc;
> > +}
> > +
> > static int mem_ioctl_no_mmu(struct hl_fpriv *hpriv, union hl_mem_args
> > *args) {
> > struct hl_device *hdev = hpriv->hdev;
> > struct hl_ctx *ctx = hpriv->ctx;
> > u64 block_handle, device_addr = 0;
> > u32 handle = 0, block_size;
> > - int rc;
> > + int rc, dmabuf_fd = -EBADF;
> >
> > switch (args->in.op) {
> > case HL_MEM_OP_ALLOC:
> > @@ -1471,6 +1863,16 @@ static int mem_ioctl_no_mmu(struct hl_fpriv
> *hpriv, union hl_mem_args *args)
> > args->out.block_size = block_size;
> > break;
> >
> > + case HL_MEM_OP_EXPORT_DMABUF_FD:
> > + rc = export_dmabuf_from_addr(ctx,
> > + args->in.export_dmabuf_fd.handle,
> > + args->in.export_dmabuf_fd.mem_size,
> > + args->in.flags,
> > + &dmabuf_fd);
> > + memset(args, 0, sizeof(*args));
> > + args->out.fd = dmabuf_fd;
> > + break;
> > +
> > default:
> > dev_err(hdev->dev, "Unknown opcode for memory
> IOCTL\n");
> > rc = -ENOTTY;
> > @@ -1489,7 +1891,7 @@ int hl_mem_ioctl(struct hl_fpriv *hpriv, void
> *data)
> > struct hl_ctx *ctx = hpriv->ctx;
> > u64 block_handle, device_addr = 0;
> > u32 handle = 0, block_size;
> > - int rc;
> > + int rc, dmabuf_fd = -EBADF;
> >
> > if (!hl_device_operational(hdev, &status)) {
> > dev_warn_ratelimited(hdev->dev,
> > @@ -1580,6 +1982,22 @@ int hl_mem_ioctl(struct hl_fpriv *hpriv, void
> *data)
> > args->out.block_size = block_size;
> > break;
> >
> > + case HL_MEM_OP_EXPORT_DMABUF_FD:
> > + if (hdev->asic_prop.dram_supports_virtual_memory)
> > + rc = export_dmabuf_from_handle(ctx,
> > + args->in.export_dmabuf_fd.handle,
> > + args->in.flags,
> > + &dmabuf_fd);
> > + else
> > + rc = export_dmabuf_from_addr(ctx,
> > + args->in.export_dmabuf_fd.handle,
> > + args-
> >in.export_dmabuf_fd.mem_size,
> > + args->in.flags,
> > + &dmabuf_fd);
>
> So these devices either work with virtual or physical memory, but the
> memory is pinned regardless?
It is only Gaudi device with which user work with physical addresses and not virtual.
In any case the devices' memory is pinned.
>
> > + memset(args, 0, sizeof(*args));
> > + args->out.fd = dmabuf_fd;
> > + break;
> > +
> > default:
> > dev_err(hdev->dev, "Unknown opcode for memory
> IOCTL\n");
> > rc = -ENOTTY;
> > diff --git a/drivers/misc/habanalabs/gaudi/gaudi.c
> > b/drivers/misc/habanalabs/gaudi/gaudi.c
> > index a673e404f777..3a66e56d7cd5 100644
> > --- a/drivers/misc/habanalabs/gaudi/gaudi.c
> > +++ b/drivers/misc/habanalabs/gaudi/gaudi.c
> > @@ -685,6 +685,7 @@ static int gaudi_early_init(struct hl_device *hdev)
> > }
> >
> > prop->dram_pci_bar_size = pci_resource_len(pdev, HBM_BAR_ID);
> > + hdev->dram_pci_bar_start = pci_resource_start(pdev,
> HBM_BAR_ID);
> >
> > /* If FW security is enabled at this point it means no access to ELBI */
> > if (hdev->asic_prop.fw_security_enabled) { diff --git
> > a/drivers/misc/habanalabs/goya/goya.c
> > b/drivers/misc/habanalabs/goya/goya.c
> > index 5a837c0b4d76..ad2c6f788030 100644
> > --- a/drivers/misc/habanalabs/goya/goya.c
> > +++ b/drivers/misc/habanalabs/goya/goya.c
> > @@ -617,6 +617,7 @@ static int goya_early_init(struct hl_device *hdev)
> > }
> >
> > prop->dram_pci_bar_size = pci_resource_len(pdev, DDR_BAR_ID);
> > + hdev->dram_pci_bar_start = pci_resource_start(pdev,
> DDR_BAR_ID);
> >
> > /* If FW security is enabled at this point it means no access to ELBI */
> > if (hdev->asic_prop.fw_security_enabled) {
> >
>
> Pedantic checkpatch has a lot to say about this patch :).
>
> Reviewed-by: Gal Pressman <galpress@...zon.com>
Thanks
Powered by blists - more mailing lists