linux-kernel - Re: [PATCH 09/16] 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: <7a44c5b2-2ca8-514f-0952-711166acb502@nvidia.com>
Date:   Mon, 3 May 2021 17:12:32 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Logan Gunthorpe <logang@...tatee.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:     Stephen Bates <sbates@...thlin.com>,
        Christoph Hellwig <hch@....de>,
        Dan Williams <dan.j.williams@...el.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Christian König <christian.koenig@....com>,
        Don Dutile <ddutile@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Jakowski Andrzej <andrzej.jakowski@...el.com>,
        Minturn Dave B <dave.b.minturn@...el.com>,
        Jason Ekstrand <jason@...kstrand.net>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Xiong Jianxin <jianxin.xiong@...el.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Ira Weiny <ira.weiny@...el.com>,
        Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH 09/16] dma-direct: Support PCI P2PDMA pages in dma-direct
 map_sg

On 5/3/21 9:55 AM, Logan Gunthorpe wrote:
...
>> The same thing can be achieved with fewer lines and a bit more clarity.
>> Can we please do it like this instead:
>>
>> 	for_each_sg(sgl, sg, nents, i) {
>> 		if (sg_is_pci_p2pdma(sg))
>> 			sg_unmark_pci_p2pdma(sg);
>> 		else
>> 			dma_direct_unmap_page(dev, sg->dma_address,
>> 					      sg_dma_len(sg), dir, attrs);
>> 	}
>>
>>
> 
> That's debatable (the way I did it emphasizes the common case). But I'll
> consider changing it.
> 

Thanks for considering it.

>>
>> Also here, a block comment for the function would be nice. How about
>> approximately this:
>>
>> /*
>>    * Maps each SG segment. Returns the number of entries mapped, or 0 upon
>>    * failure. If any entry could not be mapped, then no entries are mapped.
>>    */
>>
>> I'll stop complaining about the pre-existing return code conventions,
>> since by now you know what I was thinking of saying. :)
> 
> Not really part of this patchset... Seems like if you think there should
> be a comment like that here, you should send a patch. But this patch
> starts returning a negative value here.

OK, that's fine. Like you say, that comment is rather beyond this patchset.

>>>    	for_each_sg(sgl, sg, nents, i) {
>>> +		if (is_pci_p2pdma_page(sg_page(sg))) {
>>> +			ret = pci_p2pdma_map_segment(&p2pdma_state, dev, sg,
>>> +						     attrs);
>>> +			if (ret < 0) {
>>> +				goto out_unmap;
>>> +			} else if (ret) {
>>> +				ret = 0;
>>> +				continue;
>>
>> Is this a bug? If neither of those "if" branches fires (ret == 0), then
>> the code (probably unintentionally) falls through and continues on to
>> attempt to call dma_direct_map_page()--despite being a PCI_P2PDMA page!
> 
> No, it's not a bug. Per the documentation of pci_p2pdma_map_segment(),
> if it returns zero the segment should be mapped normally. P2PDMA pages
> must be mapped with physical addresses (or IOVA addresses) if the TLPS
> for the transaction will go through the host bridge.

Could we maybe put a little comment there, to that effect? It would be
nice to call out that point, especially since it is common to miss one
case (negative, 0, positive) when handling return values. Sort of like
we used to put "// fallthrough" in the case statements. Not a big deal
of course.

> 
>> See below for suggestions:
>>
>>> +			}
>>> +		}
>>> +
>>>    		sg->dma_address = dma_direct_map_page(dev, sg_page(sg),
>>>    				sg->offset, sg->length, dir, attrs);
>>>    		if (sg->dma_address == DMA_MAPPING_ERROR)
>>
>> This is another case in which "continue" is misleading and not as good
>> as "else". Because unless I'm wrong above, you really only want to take
>> one path *or* the other.
> 
> No, per above, it's not one path or the other. If it's a P2PDMA page it
> may still need to be mapped normally.
> 

Right. That follows.

thanks,
-- 
John Hubbard
NVIDIA