linux-kernel - Re: [PATCH v4 01/38] dma-mapping: add generic helpers for mapping sgtable objects

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: <1a80255d-81de-2c5d-6d06-ea126fd7f994@samsung.com>
Date:   Tue, 12 May 2020 15:00:31 +0200
From:   Marek Szyprowski <m.szyprowski@...sung.com>
To:     Christoph Hellwig <hch@....de>
Cc:     dri-devel@...ts.freedesktop.org, iommu@...ts.linux-foundation.org,
        linaro-mm-sig@...ts.linaro.org, linux-kernel@...r.kernel.org,
        Robin Murphy <robin.murphy@....com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        linux-arm-kernel@...ts.infradead.org,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH v4 01/38] dma-mapping: add generic helpers for mapping
 sgtable objects

Hi Christoph,

On 12.05.2020 14:18, Christoph Hellwig wrote:
> On Tue, May 12, 2020 at 11:00:21AM +0200, Marek Szyprowski wrote:
>> struct sg_table is a common structure used for describing a memory
>> buffer. It consists of a scatterlist with memory pages and DMA addresses
>> (sgl entry), as well as the number of scatterlist entries: CPU pages
>> (orig_nents entry) and DMA mapped pages (nents entry).
>>
>> It turned out that it was a common mistake to misuse nents and orig_nents
>> entries, calling DMA-mapping functions with a wrong number of entries or
>> ignoring the number of mapped entries returned by the dma_map_sg
>> function.
>>
>> To avoid such issues, lets introduce a common wrappers operating directly
>> on the struct sg_table objects, which take care of the proper use of
>> the nents and orig_nents entries.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@...sung.com>
>> ---
>> For more information, see '[PATCH v4 00/38] DRM: fix struct sg_table nents
>> vs. orig_nents misuse' thread:
>> https://lore.kernel.org/dri-devel/20200512085710.14688-1-m.szyprowski@samsung.com/T/
>> ---
>>   include/linux/dma-mapping.h | 79 +++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 79 insertions(+)
>>
>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>> index b43116a..88f01cc 100644
>> --- a/include/linux/dma-mapping.h
>> +++ b/include/linux/dma-mapping.h
>> @@ -609,6 +609,85 @@ static inline void dma_sync_single_range_for_device(struct device *dev,
>>   	return dma_sync_single_for_device(dev, addr + offset, size, dir);
>>   }
>>   
>> +/**
>> + * dma_map_sgtable - Map the given buffer for the DMA operations
>> + * @dev:	The device to perform a DMA operation
>> + * @sgt:	The sg_table object describing the buffer
>> + * @dir:	DMA direction
>> + * @attrs:	Optional DMA attributes for the map operation
>> + *
>> + * Maps a buffer described by a scatterlist stored in the given sg_table
>> + * object for the @dir DMA operation by the @dev device. After success
>> + * the ownership for the buffer is transferred to the DMA domain. One has
>> + * to call dma_sync_sgtable_for_cpu() or dma_unmap_sgtable() to move the
>> + * ownership of the buffer back to the CPU domain before touching the
>> + * buffer by the CPU.
>> + * Returns 0 on success or -EINVAL on error during mapping the buffer.
>> + */
>> +static inline int dma_map_sgtable(struct device *dev, struct sg_table *sgt,
>> +		enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +	int n = dma_map_sg_attrs(dev, sgt->sgl, sgt->orig_nents, dir, attrs);
>> +
>> +	if (n > 0) {
>> +		sgt->nents = n;
>> +		return 0;
>> +	}
>> +	return -EINVAL;
> Nit:  code tend to be a tad easier to read if the the exceptional
> condition is inside the branch block, so:
>
> 	if (n <= 0)
> 		return -EINVAL;
> 	sgt->nents = n;
> 	return 0;
>
Indeed this version looks much better. I will resend it in a few minutes.
> Otherwise this looks good to me:
>
> Reviewed-by: Christoph Hellwig <hch@....de>
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland