> While I agree an executable heap is pretty weird, I'd prefer making this
> explicit - i.e. failing the allocation if the flags don't make sense.
The only use case for an executable heap I can think of is an attacker
trying to exploit a GPU-side heap overflow, and that's seriously
stretching it ;)
Making executable? mutually exclusive with growable? is quite sane to
me.
>
> >
> > ret = panfrost_gem_map(pfdev, bo);
> > if (ret)
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> > index 132f02399b7b..c500ca6b9072 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> > @@ -13,6 +13,7 @@ struct panfrost_gem_object {
> > struct drm_mm_node node;
> > bool is_mapped :1;
> > bool noexec :1;
> > + bool is_heap :1;
> > };
> >
> > static inline
> > @@ -21,6 +22,13 @@ struct panfrost_gem_object *to_panfrost_bo(struct drm_gem_object *obj)
> > return container_of(to_drm_gem_shmem_obj(obj), struct panfrost_gem_object, base);
> > }
> >
> > +static inline
> > +struct panfrost_gem_object *drm_mm_node_to_panfrost_bo(struct drm_mm_node *node)
> > +{
> > + return container_of(node, struct panfrost_gem_object, node);
> > +}
> > +
> > +
>
> NIT: Extra blank line
>
> > struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size);
> >
> > struct panfrost_gem_object *
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > index d18484a07bfa..3b95c7027188 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> > @@ -3,6 +3,7 @@
> > /* Copyright (C) 2019 Arm Ltd. */
> > #include <linux/bitfield.h>
> > #include <linux/delay.h>
> > +#include <linux/dma-mapping.h>
> > #include <linux/interrupt.h>
> > #include <linux/io.h>
> > #include <linux/iopoll.h>
> > @@ -10,6 +11,7 @@
> > #include <linux/iommu.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_runtime.h>
> > +#include <linux/shmem_fs.h>
> > #include <linux/sizes.h>
> >
> > #include "panfrost_device.h"
> > @@ -257,12 +259,12 @@ void panfrost_mmu_unmap(struct panfrost_gem_object *bo)
> > size_t unmapped_page;
> > size_t pgsize = get_pgsize(iova, len - unmapped_len);
> >
> > - unmapped_page = ops->unmap(ops, iova, pgsize);
> > - if (!unmapped_page)
> > - break;
> > -
> > - iova += unmapped_page;
> > - unmapped_len += unmapped_page;
> > + if (ops->iova_to_phys(ops, iova)) {
> > + unmapped_page = ops->unmap(ops, iova, pgsize);
> > + WARN_ON(unmapped_page != pgsize);
> > + }
> > + iova += pgsize;
> > + unmapped_len += pgsize;
> > }
> >
> > mmu_hw_do_operation(pfdev, 0, bo->node.start << PAGE_SHIFT,
> > @@ -298,6 +300,86 @@ static const struct iommu_gather_ops mmu_tlb_ops = {
> > .tlb_sync = mmu_tlb_sync_context,
> > };
> >
> > +static struct drm_mm_node *addr_to_drm_mm_node(struct panfrost_device *pfdev, int as, u64 addr)
> > +{
> > + struct drm_mm_node *node;
> > + u64 offset = addr >> PAGE_SHIFT;
> > +
> > + drm_mm_for_each_node(node, &pfdev->mm) {
> > + if (offset >= node->start && offset < (node->start + node->size))
> > + return node;
> > + }
> > + return NULL;
> > +}
> > +
> > +#define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
> > +
> > +int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as, u64 addr)
> > +{
> > + int ret, i;
> > + struct drm_mm_node *node;
> > + struct panfrost_gem_object *bo;
> > + struct address_space *mapping;
> > + pgoff_t page_offset;
> > + struct sg_table sgt = {};
> > + struct page **pages;
> > +
> > + node = addr_to_drm_mm_node(pfdev, as, addr);
> > + if (!node)
> > + return -ENOENT;
> > +
> > + bo = drm_mm_node_to_panfrost_bo(node);
> > + if (!bo->is_heap) {
> > + dev_WARN(pfdev->dev, "matching BO is not heap type (GPU VA = %llx)",
> > + node->start << PAGE_SHIFT);
> > + return -EINVAL;
> > + }
> > + /* Assume 2MB alignment and size multiple */
> > + addr &= ~((u64)SZ_2M - 1);
> > + page_offset = addr >> PAGE_SHIFT;
> > + page_offset -= node->start;
> > +
> > + pages = kvmalloc_array(NUM_FAULT_PAGES, sizeof(struct page *), GFP_KERNEL);
> > + if (!pages)
> > + return -ENOMEM;
> > +
> > + mapping = bo->base.base.filp->f_mapping;
> > + mapping_set_unevictable(mapping);
> > +
> > + for (i = 0; i < NUM_FAULT_PAGES; i++) {
> > + pages[i] = shmem_read_mapping_page(mapping, page_offset + i);
> > + if (IS_ERR(pages[i])) {
> > + ret = PTR_ERR(pages[i]);
> > + goto err_pages;
> > + }
> > + }
> > +
> > + ret = sg_alloc_table_from_pages(&sgt, pages, NUM_FAULT_PAGES, 0,
> > + SZ_2M, GFP_KERNEL);
> > + if (ret)
> > + goto err_pages;
> > +
> > + if (dma_map_sg(pfdev->dev, sgt.sgl, sgt.nents, DMA_BIDIRECTIONAL) == 0) {
> > + ret = -EINVAL;
> > + goto err_map;
> > + }
> > +
> > + mmu_map_sg(pfdev, addr, IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, &sgt);
> > +
> > + mmu_write(pfdev, MMU_INT_CLEAR, BIT(as));
> > + bo->is_mapped = true;
> > +
> > + dev_dbg(pfdev->dev, "mapped page fault @ %llx", addr);
> > +
> > + return 0;
>
> You still need to free sgt and pages - so this should be:
>
> ret = 0;
>
> to fall through to the clean up below:
>
> > +
> > +err_map:
> > + sg_free_table(&sgt);
> > +err_pages:
> > + kvfree(pages);
> > + return ret;
> > +}
> > +
>
> But actually, you need to store the pages allocated in the buffer object
> so that they can be freed later. At the moment you have a big memory leak.
>
> > static const char *access_type_name(struct panfrost_device *pfdev,
> > u32 fault_status)
> > {
> > @@ -323,13 +405,11 @@ static irqreturn_t panfrost_mmu_irq_handler(int irq, void *data)
> > {
> > struct panfrost_device *pfdev = data;
> > u32 status = mmu_read(pfdev, MMU_INT_STAT);
> > - int i;
> > + int i, ret;
> >
> > if (!status)
> > return IRQ_NONE;
> >
> > - dev_err(pfdev->dev, "mmu irq status=%x\n", status);
> > -
> > for (i = 0; status; i++) {
> > u32 mask = BIT(i) | BIT(i + 16);
> > u64 addr;
> > @@ -350,6 +430,17 @@ static irqreturn_t panfrost_mmu_irq_handler(int irq, void *data)
> > access_type = (fault_status >> 8) & 0x3;
> > source_id = (fault_status >> 16);
> >
> > + /* Page fault only */
> > + if ((status & mask) == BIT(i)) {
> > + WARN_ON(exception_type < 0xC1 || exception_type > 0xC4);
> > +
> > + ret = panfrost_mmu_map_fault_addr(pfdev, i, addr);
> > + if (!ret) {
> > + status &= ~mask;
> > + continue;
>
> In this case the IRQ isn't handled and will remain asserted, which
> probably isn't going to end particularly well.
>
> Ideally you would switch the address space to UNMAPPED to kill off the
> job, but at the very least we should acknowledge the interrupt and let
> the GPU timeout reset the GPU to recover (which is equivalent while we
> still only use the one AS on the GPU).
>
> Steve
>
> > + }
> > + }
> > +
> > /* terminal fault, print info about the fault */
> > dev_err(pfdev->dev,
> > "Unhandled Page fault in AS%d at VA 0x%016llX\n"
> > @@ -391,8 +482,9 @@ int panfrost_mmu_init(struct panfrost_device *pfdev)
> > if (irq <= 0)
> > return -ENODEV;
> >
> > - err = devm_request_irq(pfdev->dev, irq, panfrost_mmu_irq_handler,
> > - IRQF_SHARED, "mmu", pfdev);
> > + err = devm_request_threaded_irq(pfdev->dev, irq, NULL,
> > + panfrost_mmu_irq_handler,
> > + IRQF_ONESHOT, "mmu", pfdev);
> >
> > if (err) {
> > dev_err(pfdev->dev, "failed to request mmu irq");
> > diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> > index 17fb5d200f7a..9150dd75aad8 100644
> > --- a/include/uapi/drm/panfrost_drm.h
> > +++ b/include/uapi/drm/panfrost_drm.h
> > @@ -83,6 +83,7 @@ struct drm_panfrost_wait_bo {
> > };
> >
> > #define PANFROST_BO_NOEXEC 1
> > +#define PANFROST_BO_HEAP 2
> >
> > /**
> > * struct drm_panfrost_create_bo - ioctl argument for creating Panfrost BOs.
> >
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
