From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Subject: Re: IOAT DMA w/IOMMU To: Logan Gunthorpe , Kit Chow , Dave Jiang , Eric Pilmore , Bjorn Helgaas Cc: "linux-pci@vger.kernel.org" , Alex Williamson , David Woodhouse , "iommu@lists.linux-foundation.org" References: <1170c096-a749-6ee3-32d4-ebba1d12adff@gigaio.com> <929f3dfa-fbff-c582-3be5-c368f11feb7e@deltatee.com> <460cc47f-6a8e-67b0-01a0-02f0c51df258@intel.com> <0f653f8f-c7ae-831d-e238-2742590d9ea1@deltatee.com> <6525fa4d-7d9b-0136-7206-f8351230af46@intel.com> <499934e7-3734-1aee-37dd-b42a5d2a2608@intel.com> <9d135f61-20c2-e966-4d7d-3e154384ea08@gigaio.com> <0a9fa9c3-dbe4-6451-9c1a-06b662702f61@deltatee.com> <3772d88a-e9ad-465a-c91f-df8f9c6219be@gigaio.com> <0e839acf-5948-42aa-2ad6-72ca0d84a8c3@deltatee.com> <712ecb24-8919-05a5-f64c-6a1faa971edc@deltatee.com> From: Robin Murphy Message-ID: Date: Tue, 14 Aug 2018 15:03:42 +0100 MIME-Version: 1.0 In-Reply-To: <712ecb24-8919-05a5-f64c-6a1faa971edc@deltatee.com> Content-Type: text/plain; charset=utf-8; format=flowed List-ID: On 14/08/18 00:50, Logan Gunthorpe wrote: > On 13/08/18 05:48 PM, Kit Chow wrote: >> On 08/13/2018 04:39 PM, Logan Gunthorpe wrote: >>> >>> On 13/08/18 05:30 PM, Kit Chow wrote: >>>> In arch/x86/include/asm/page.h, there is the following comment in >>>> regards to validating the virtual address. >>>> >>>> /* >>>>   * virt_to_page(kaddr) returns a valid pointer if and only if >>>>   * virt_addr_valid(kaddr) returns true. >>>>   */ >>>> #define virt_to_page(kaddr)     pfn_to_page(__pa(kaddr) >> PAGE_SHIFT) >>>> >>>> So it looks like the validation by virt_addr_valid was somehow dropped >>>> from the virt_to_page code path. Does anyone have any ideas what >>>> happended to it? >>> I don't think it was ever validated (though I haven't been around long >>> enough to say for certain). What the comment is saying is that you >>> shouldn't rely on virt_to_page() unless you know virt_addr_valid() is >>> true (which in most cases you can without the extra check). virt_to_page >>> is meant to be really fast so adding an extra validation would probably >>> be a significant performance regression for the entire kernel. >>> >>> The fact that this can happen through dma_map_single() is non-ideal at >>> best. It assumes the caller is mapping regular memory and doesn't check >>> this at all. It may make sense to fix that but I think people expect >>> dma_map_single() to be as fast as possible as well... dma_map_single() is already documented as only supporting lowmem (for which virt_to_page() can be assumed to be valid). You might get away with feeding it bogus addresses on x86, but on non-coherent architectures which convert the page back to a virtual address to perform cache maintenance you can expect that to crash and burn rapidly. There may be some minimal-overhead sanity checking of fundamentals, but in general it's not really the DMA API's job to police its callers exhaustively; consider that the mm layer doesn't go out of its way to stop you from doing things like "kfree(kfree);" either. >>> >> Perhaps include the validation with some debug turned on? > > The problem is how often do you develop code with any of the debug > config options turned on? > > There's already a couple of BUG_ONs in dma_map_single so maybe another > one with virt_addr_valid wouldn't be so bad. Note that virt_addr_valid() may be pretty heavyweight in itself. For example the arm64 implementation involves memblock_search(); that really isn't viable in a DMA mapping fastpath. Robin.