netdev - Re: [RFC PATCH] net/mlx4: Get rid of page operation after dma_alloc

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 18 Dec 2018 10:08:56 -0700
From:   Stephen Warren <swarren@...dotorg.org>
To:     Jason Gunthorpe <jgg@...lanox.com>
Cc:     Tariq Toukan <tariqt@...lanox.com>, xavier.huwei@...wei.com,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        Doug Ledford <dledford@...hat.com>
Subject: Re: [RFC PATCH] net/mlx4: Get rid of page operation after
 dma_alloc_coherent

On 12/18/18 9:32 AM, Jason Gunthorpe wrote:
> On Fri, Dec 14, 2018 at 04:32:54PM -0700, Stephen Warren wrote:
>> From: Stephen Warren <swarren@...dia.com>
>>
>> This is a port of commit 378efe798ecf ("RDMA/hns: Get rid of page
>> operation after dma_alloc_coherent") to the mlx4 driver. That change was
>> described as:
>>
>>> In general, dma_alloc_coherent() returns a CPU virtual address and
>>> a DMA address, and we have no guarantee that the underlying memory
>>> even has an associated struct page at all.
>>>
>>> This patch gets rid of the page operation after dma_alloc_coherent,
>>> and records the VA returned form dma_alloc_coherent in the struct
>>> of hem in hns RoCE driver.
>>
>> Differences in this port relative to the hns patch:
>>
>> 1) The hns patch only needed to fix a dma_alloc_coherent path, but this
>> patch also needs to fix an alloc_pages path. This appears to be simple
>> except for the next point.
>>
>> 2) The hns patch converted a bunch of code to consistently use
>> sg_dma_len(mem) rather than a mix of that and mem->length However, it
>> seems that sg_dma_len(mem) can be modified or zeroed at runtime, and so
>> using it when calling e.g. __free_pages is problematic.
> 
> dma_len should only ever be used when programming a HW device to do
> DMA. It certainly should never be used for anything else, so I'm not
> sure why this description veered off into talking about alloc_pages?
> 
> If pages were allocated and described in a sg list then the CPU side
> must use the pages/len part of the SGL to walk that list of pages.
> 
> I also don't really see a practical problem with putting the virtual
> address pointer of DMA coherent memory in the SGL, so long as it is
> never used in a DMA map operation or otherwise.
> 
> .. so again, what is it this is actually trying to fix in mlx4?

The same thing that the original hns patch fixed, and in the exact same 
way. Namely a crash during driver unload or system shutdown in the path 
that frees allocated memory contained in the sg list.

The reason is that the allocation does:

static int mlx4_alloc_icm_coherent(...
...
         void *buf = dma_alloc_coherent(dev, PAGE_SIZE << order,
                                        &sg_dma_address(mem), gfp_mask);
...
         sg_set_buf(mem, buf, PAGE_SIZE << order);
         sg_dma_len(mem) = PAGE_SIZE << order;

And free does:

static void mlx4_free_icm_coherent(...
...
     dma_free_coherent(&dev->persist->pdev->dev,
                       chunk->mem[i].length,
                       lowmem_page_address(sg_page(&chunk->mem[i])),

However, there's no guarantee that dma_alloc_coherent() returned memory 
for which a struct page exists, and hence the call to sg_page() and/or 
lowmem_page_address() can fail. To fix this, we add a second field to 
the mlx4 table struct which holds the return value from 
dma_alloc_coherent() so that value can be passed to dma_free_coherent() 
directly, rather than trying to re-derive the value in 
mlx4_free_icm_coherent().