From: Tony Battersby <tonyb@cybernetics.com>
To: Robin Murphy <robin.murphy@arm.com>,
Krzysztof Kozlowski <krzk@kernel.org>,
tony@atomide.com, akpm@linux-foundation.org
Cc: Stephen Rothwell <sfr@canb.auug.org.au>,
john.garry@huawei.com, linux@armlinux.org.uk,
linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
linux-omap@vger.kernel.org, hch@lst.de,
linux-arm-kernel@lists.infradead.org,
Marek Szyprowski <m.szyprowski@samsung.com>,
Matthew Wilcox <willy@infradead.org>
Subject: Re: dmapool regression in next
Date: Thu, 6 Dec 2018 11:13:16 -0500 [thread overview]
Message-ID: <9187bbd9-3aaf-c1b3-16a3-4e1b3356c52e@cybernetics.com> (raw)
In-Reply-To: <451215b8-548a-eff7-9e96-0ff5f8cbb614@arm.com>
On 12/6/18 10:51 AM, Robin Murphy wrote:
>> Here is the prototype:
>>
>> void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma);
>>
>> With the old code, the 'dma' value had to be correct for use with
>> pool_find_page(), or else you would get an error. If the 'vaddr' value
>> was incorrect, it would corrupt the dmapool freelist, but you wouldn't
>> get an error unless DMAPOOL_DEBUG was enabled.
>>
>> With my patch applied, 'vaddr' has to be correct for virt_to_page(). My
>> code also checks that 'dma' is consistent with 'vaddr' even if
>> DMAPOOL_DEBUG is disabled, since the check is fast and it will prevent
>> problems like this in the future.
> Unfortunately that logic has a fatal flaw - DMA pools are backed by
> dma_alloc_coherent(), and there is absolutely no guarantee that the
> memory dma_alloc_coherent() returns is backed by a struct page at all.
> Even if it is, there is still absolutely no guarantee that the vaddr
> value it returns is valid for virt_to_page() - on many systems it will
> be in vmalloc or some architecture-specific region of address space.
>
> The problem is not that these drivers are buggy (they're not - the arch
> code is returning a vmalloc()ed non-cacheable remap in the first place),
> it's that 26abe88e830d is fundamentally unworkable and needs reverting.
> Apparently the original patches managed not to catch my eye as something
> I needed to review, sorry about that :(
>
> Robin.
>
Thanks for the info; the inner workings of the vm system are a bit out
of my area of expertise. My first version of the patch series used a
different method that didn't rely on virt_to_page(); I will go back to
that version, clean it up, and resubmit when I have time.
Andrew, please revert all 9 patches. I will resubmit the set when I
have a workable solution.
Tony Battersby
next prev parent reply other threads:[~2018-12-06 16:13 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-06 1:30 dmapool regression in next Tony Lindgren
2018-12-06 9:25 ` Krzysztof Kozlowski
2018-12-06 15:11 ` Tony Battersby
2018-12-06 15:51 ` Robin Murphy
2018-12-06 16:13 ` Tony Battersby [this message]
2018-12-06 16:33 ` Tony Lindgren
2018-12-06 22:10 ` Stephen Rothwell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9187bbd9-3aaf-c1b3-16a3-4e1b3356c52e@cybernetics.com \
--to=tonyb@cybernetics.com \
--cc=akpm@linux-foundation.org \
--cc=andy.shevchenko@gmail.com \
--cc=hch@lst.de \
--cc=john.garry@huawei.com \
--cc=krzk@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=m.szyprowski@samsung.com \
--cc=robin.murphy@arm.com \
--cc=sfr@canb.auug.org.au \
--cc=tony@atomide.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).