linux-kernel - Re: [RFC PATCH v2 06/11] dma-direct: Support PCI P2PDMA pages in dma-direct map

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: <d7ead722-7356-8e0f-22de-cb9dea12b556@deltatee.com>
Date:   Fri, 12 Mar 2021 09:24:51 -0700
From:   Logan Gunthorpe <logang@...tatee.com>
To:     Robin Murphy <robin.murphy@....com>, linux-kernel@...r.kernel.org,
        linux-nvme@...ts.infradead.org, linux-block@...r.kernel.org,
        linux-pci@...r.kernel.org, linux-mm@...ck.org,
        iommu@...ts.linux-foundation.org
Cc:     Minturn Dave B <dave.b.minturn@...el.com>,
        John Hubbard <jhubbard@...dia.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Ira Weiny <iweiny@...el.com>,
        Matthew Wilcox <willy@...radead.org>,
        Christian König <christian.koenig@....com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Jason Ekstrand <jason@...kstrand.net>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Dan Williams <dan.j.williams@...el.com>,
        Stephen Bates <sbates@...thlin.com>,
        Jakowski Andrzej <andrzej.jakowski@...el.com>,
        Christoph Hellwig <hch@....de>,
        Xiong Jianxin <jianxin.xiong@...el.com>
Subject: Re: [RFC PATCH v2 06/11] dma-direct: Support PCI P2PDMA pages in
 dma-direct map_sg



On 2021-03-12 8:52 a.m., Robin Murphy wrote:
>> +
>>           sg->dma_address = dma_direct_map_page(dev, sg_page(sg),
>>                   sg->offset, sg->length, dir, attrs);
>>           if (sg->dma_address == DMA_MAPPING_ERROR)
>> @@ -411,7 +440,7 @@ int dma_direct_map_sg(struct device *dev, struct
>> scatterlist *sgl, int nents,
>>     out_unmap:
>>       dma_direct_unmap_sg(dev, sgl, i, dir, attrs |
>> DMA_ATTR_SKIP_CPU_SYNC);
>> -    return 0;
>> +    return ret;
>>   }
>>     dma_addr_t dma_direct_map_resource(struct device *dev, phys_addr_t
>> paddr,
>> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
>> index b6a633679933..adc1a83950be 100644
>> --- a/kernel/dma/mapping.c
>> +++ b/kernel/dma/mapping.c
>> @@ -178,8 +178,15 @@ void dma_unmap_page_attrs(struct device *dev,
>> dma_addr_t addr, size_t size,
>>   EXPORT_SYMBOL(dma_unmap_page_attrs);
>>     /*
>> - * dma_maps_sg_attrs returns 0 on error and > 0 on success.
>> - * It should never return a value < 0.
>> + * dma_maps_sg_attrs returns 0 on any resource error and > 0 on success.
>> + *
>> + * If 0 is returned, the mapping can be retried and will succeed once
>> + * sufficient resources are available.
> 
> That's not a guarantee we can uphold. Retrying forever in the vain hope
> that a device might evolve some extra address bits, or a bounce buffer
> might magically grow big enough for a gigantic mapping, isn't
> necessarily the best idea.

Perhaps this is just poorly worded. Returning 0 is the normal case and
nothing has changed there. The block layer, for example, will retry if
zero is returned as this only happens if it failed to allocate resources
for the mapping. The reason we have to return -1 is to tell the block
layer not to retry these requests as they will never succeed in the future.

>> + *
>> + * If there are P2PDMA pages in the scatterlist then this function may
>> + * return -EREMOTEIO to indicate that the pages are not mappable by the
>> + * device. In this case, an error should be returned for the IO as it
>> + * will never be successfully retried.
>>    */
>>   int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int
>> nents,
>>           enum dma_data_direction dir, unsigned long attrs)
>> @@ -197,7 +204,7 @@ int dma_map_sg_attrs(struct device *dev, struct
>> scatterlist *sg, int nents,
>>           ents = dma_direct_map_sg(dev, sg, nents, dir, attrs);
>>       else
>>           ents = ops->map_sg(dev, sg, nents, dir, attrs);
>> -    BUG_ON(ents < 0);
>> +
> 
> This scares me - I hesitate to imagine the amount of driver/subsystem
> code out there that will see nonzero and merrily set off iterating a
> negative number of segments, if we open the floodgates of allowing
> implementations to return error codes here.

Yes, but it will never happen on existing drivers/subsystems. The only
way it can return a negative number is if the driver passes in P2PDMA
pages which can't happen without changes in the driver. We are careful
about where P2PDMA pages can get into so we don't have to worry about
all the existing driver code out there.

Logan