linux-kernel - Re: [PATCH 1/2] dma: call unconditionally to unmap_page and unmap

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: <7ef6cd1e-3dc6-452e-ac1c-128ee98acdb0@arm.com>
Date: Thu, 11 Jul 2024 19:02:39 +0100
From: Robin Murphy <robin.murphy@....com>
To: Leon Romanovsky <leon@...nel.org>, Christoph Hellwig <hch@....de>,
 Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
 Marek Szyprowski <m.szyprowski@...sung.com>
Cc: Leon Romanovsky <leonro@...dia.com>, linux-kernel@...r.kernel.org,
 iommu@...ts.linux.dev, Jason Gunthorpe <jgg@...dia.com>
Subject: Re: [PATCH 1/2] dma: call unconditionally to unmap_page and unmap_sg
 callbacks

On 11/07/2024 11:38 am, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@...dia.com>
> 
> Almost all users of ->map_page()/map_sg() callbacks implement
> ->unmap_page()/unmap_sg() callbacks too. One user which doesn't do it,
> is dummy DMA ops interface, and the use of this interface is to fail
> the operation and in such case, there won't be any call to
> ->unmap_page()/unmap_sg().
> 
> This patch removes the existence checks of ->unmap_page()/unmap_sg()
> and calls to it directly to create symmetrical interface to
> ->map_page()/map_sg().

Now that all the common cases have been mopped up by dma-direct, I'm 
inclined to agree that this seems reasonable. For sure there is code out 
there still abusing random DMA API calls as a cache maintenance 
interface because it thinks it knows better, but even that's going to be 
running on platforms where it expects unmap to have the desired effect 
anyway, so the chance of somehow ending up with the dummy ops and 
crashing seems sufficiently unlikely.

However, I'm a little wary of the static checker brigade noticing that 
and trying to "fix" it by reinstating these tests, so perhaps it's worth 
just adding unmaps to the dummy ops (with a WARN() in them) as well for 
the sake of cleanliness and avoidance of any doubt.

Thanks,
Robin.

> Signed-off-by: Leon Romanovsky <leonro@...dia.com>
> ---
>   kernel/dma/mapping.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index 81de84318ccc..6832fd6f0796 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -177,7 +177,7 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size,
>   	if (dma_map_direct(dev, ops) ||
>   	    arch_dma_unmap_page_direct(dev, addr + size))
>   		dma_direct_unmap_page(dev, addr, size, dir, attrs);
> -	else if (ops->unmap_page)
> +	else
>   		ops->unmap_page(dev, addr, size, dir, attrs);
>   	debug_dma_unmap_page(dev, addr, size, dir);
>   }
> @@ -291,7 +291,7 @@ void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg,
>   	if (dma_map_direct(dev, ops) ||
>   	    arch_dma_unmap_sg_direct(dev, sg, nents))
>   		dma_direct_unmap_sg(dev, sg, nents, dir, attrs);
> -	else if (ops->unmap_sg)
> +	else
>   		ops->unmap_sg(dev, sg, nents, dir, attrs);
>   }
>   EXPORT_SYMBOL(dma_unmap_sg_attrs);