linux-kernel - Re: please revert "drm/msm: Use the correct dma_sync calls in msm

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: <20190803061916.GA29348@lst.de>
Date:   Sat, 3 Aug 2019 08:19:16 +0200
From:   Christoph Hellwig <hch@....de>
To:     Rob Clark <robdclark@...omium.org>
Cc:     Christoph Hellwig <hch@....de>,
        Stephan Gerhold <stephan@...hold.net>,
        Sean Paul <seanpaul@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        torvalds@...ux-foundation.org
Subject: Re: please revert "drm/msm: Use the correct dma_sync calls in
 msm_gem"

On Fri, Aug 02, 2019 at 03:06:10PM -0700, Rob Clark wrote:
> Sorry, this is just a temporary band-aid for v5.3 to get things
> working again.  Yes, I realize it is a complete hack.

My main problem is here that you badly hack a around a problem without
talking to the relevant maintainers, and by abusing even more internal
APIs.  As said get_dma_ops isn't really for driver use (although we
have a few that use it for not quite as bad reason we are trying to get
rid off).  And as also said your abuse of the DMA API will blow up
with dma-debug use quite badly.  You might also corrupt the dma_address
in the scatterlist in ways that aren't intended - as the call to
dma_map_sg will allocate new iova space you are getting different
results from whatever you expect to actually get from your iommu API
usage.  This might or might no matter in the end, but you really should
consult the maintainers first.

> The root problem is that I'm using the DMA API in the first place.  I
> don't actually use the DMA API to map buffers, for various reasons,
> but instead manage the iommu_domain directly.

Yes, and this has been going on for years, without any obvious attempt
to address it at the API level before..

> Because arm/arm64 cache ops are not exported to modules, so currently
> I need to abuse the DMA API for cache operations (really just to clean
> pages if I need to mmap them uncached/writecombine).  Originally I was
> doing that w/ dma_{map,unmap}_sg.  But to avoid debug splats I
> switched that to dma_sync_sg (see
> 0036bc73ccbe7e600a3468bf8e8879b122252274).  But now it seems the
> dma-direct ops are unhappy w/ dma_sync without a dma_map (AFAICT).

Russell has been very strict about not exporting the cache ops, and all
for the right reasons.  Cache maintainance for not dma coherent devices
is hard, and without a proper API that has arch input for even which
calls are used for cache flushing chances of bugs are extremely high.

I see two proper ways out of this mess:  either we actually make msm
use the DMA API, so I'd be curious of what is missing that forces you
to use the low-level iommu API.  Or we need to enhance the iommu API
with a similar ownership concepts as the DMA API.  Which probably is
a good thing even if we move msm over to the DMA API.

> (On some generations of hw, the iommu is attached to the device node
> that maps to the drm device, which is passed to dma_map/dma_sync.  On
> other generations the iommu is attached to a sub-device.  Changing
> this would break dtb compatibility.. so for now I need to handle both
> iommu-ops and direct-ops cases.)

Or you always call call on a struct device that has the iommu, that
is match on the generic, and pick a different device.  That would in
many ways seem preferably over the current hack, even if that also is
just a horribly band-aid.