[PATCH] drm/etnaviv: remove BUG_ON in MMU unmap path

[PATCH] drm/etnaviv: remove BUG_ON in MMU unmap path

[Lucas Stach]

If the MMU map fails caused by an unaligned SG entry, the unmap path
is called to undo all already setup SG mappings. When encountering the
unaligned SG the unmap path hangs the kernel with a BUG(), while the
error is recoverable by just failing the submit that references the
faulty object.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
index 29a723fabc17..ebbb31ec9feb 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
@@ -83,8 +83,6 @@ int etnaviv_iommu_unmap(struct etnaviv_iommu *iommu, u32 iova,
 
 		VERB("unmap[%d]: %08x(%zx)", i, iova, bytes);
 
-		BUG_ON(!PAGE_ALIGNED(bytes));
-
 		da += bytes;
 	}
 
-- 
2.8.0.rc3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/etnaviv: remove BUG_ON in MMU unmap path

[Russell King - ARM Linux]

On Wed, Apr 27, 2016 at 02:38:18PM +0200, Lucas Stach wrote:
> If the MMU map fails caused by an unaligned SG entry, the unmap path
> is called to undo all already setup SG mappings. When encountering the
> unaligned SG the unmap path hangs the kernel with a BUG(), while the
> error is recoverable by just failing the submit that references the
> faulty object.

I'm very tempted to NAK this, because I introduced it with good reason:
we should never see anything except a page-aligned number of bytes here.

Are you seeing this trigger?

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/etnaviv: remove BUG_ON in MMU unmap path

[Lucas Stach]

Am Donnerstag, den 28.04.2016, 14:55 +0100 schrieb Russell King - ARM
Linux:
> On Wed, Apr 27, 2016 at 02:38:18PM +0200, Lucas Stach wrote:
> > If the MMU map fails caused by an unaligned SG entry, the unmap path
> > is called to undo all already setup SG mappings. When encountering the
> > unaligned SG the unmap path hangs the kernel with a BUG(), while the
> > error is recoverable by just failing the submit that references the
> > faulty object.
> 
> I'm very tempted to NAK this, because I introduced it with good reason:
> we should never see anything except a page-aligned number of bytes here.
> 
> Are you seeing this trigger?
> 
I've only seen this once and I don't know what caused it really. Maybe
some unrelated corruption.

The observation was that the common code in iommu_map() rightfully
rejected to map things, as mapping something unaligned to the page size
is totally bogus. iommu_unmap will detect this condition in the same
way. So without this BUG_ON() the kernel would have been able to recover
from the bogus input data (maybe allowing to debug things further), but
with the BUG_ON() in place it just dies.

Regards,
Lucas

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/etnaviv: remove BUG_ON in MMU unmap path

[Russell King - ARM Linux]

On Thu, Apr 28, 2016 at 04:04:58PM +0200, Lucas Stach wrote:
> The observation was that the common code in iommu_map() rightfully
> rejected to map things, as mapping something unaligned to the page size
> is totally bogus.

Shouldn't iommu_map() detect this?

        /*
         * both the virtual address and the physical one, as well as
         * the size of the mapping, must be aligned (at least) to the
         * size of the smallest page supported by the hardware
         */
        if (!IS_ALIGNED(iova | paddr | size, min_pagesz)) {
                pr_err("unaligned: iova 0x%lx pa %pa size 0x%zx min_pagesz 0x%x\n",
                       iova, &paddr, size, min_pagesz);
                return -EINVAL;
        }

where min_pagesz is derived from:

                .pgsize_bitmap = SZ_4K,

and will be 4096.  So, iommu_map() should reject this, and
etnaviv_iommu_map() will tear down the partially created mapping, and
propagate the error code to its caller, that being etnaviv_iommu_map_gem().

etnaviv_iommu_map_gem() will remove the drm_mm node, propagating the
failure up to etnaviv_gem_mapping_get(), which will free the
etnaviv_vram_mapping structure.

I fail to see how we could get into etnaviv_iommu_unmap() with a bad
mapping, other than if there's memory corruption, and if there is
memory corruption, hanging the kernel with a BUG_ON() is totally the
right thing to do.  Better that than a corrupted filesystem.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/etnaviv: remove BUG_ON in MMU unmap path

[Lucas Stach]

Am Donnerstag, den 28.04.2016, 15:37 +0100 schrieb Russell King - ARM
Linux:
> On Thu, Apr 28, 2016 at 04:04:58PM +0200, Lucas Stach wrote:
> > The observation was that the common code in iommu_map() rightfully
> > rejected to map things, as mapping something unaligned to the page size
> > is totally bogus.
> 
> Shouldn't iommu_map() detect this?
> 
>         /*
>          * both the virtual address and the physical one, as well as
>          * the size of the mapping, must be aligned (at least) to the
>          * size of the smallest page supported by the hardware
>          */
>         if (!IS_ALIGNED(iova | paddr | size, min_pagesz)) {
>                 pr_err("unaligned: iova 0x%lx pa %pa size 0x%zx min_pagesz 0x%x\n",
>                        iova, &paddr, size, min_pagesz);
>                 return -EINVAL;
>         }
> 
> where min_pagesz is derived from:
> 
>                 .pgsize_bitmap = SZ_4K,
> 
> and will be 4096.  So, iommu_map() should reject this, and
> etnaviv_iommu_map() will tear down the partially created mapping, and
> propagate the error code to its caller, that being etnaviv_iommu_map_gem().
> 
> etnaviv_iommu_map_gem() will remove the drm_mm node, propagating the
> failure up to etnaviv_gem_mapping_get(), which will free the
> etnaviv_vram_mapping structure.
> 
> I fail to see how we could get into etnaviv_iommu_unmap() with a bad
> mapping, other than if there's memory corruption, and if there is
> memory corruption, hanging the kernel with a BUG_ON() is totally the
> right thing to do.  Better that than a corrupted filesystem.
> 
Right, if something goes wrong in the unmap() path, something is screwed
with kernel internal state. I'll drop the patch.

Thanks,
Lucas

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel