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: <20240712054725.GD1815706@unreal>
Date: Fri, 12 Jul 2024 08:47:25 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Robin Murphy <robin.murphy@....com>
Cc: Christoph Hellwig <hch@....de>, Joerg Roedel <joro@...tes.org>,
	Will Deacon <will@...nel.org>,
	Marek Szyprowski <m.szyprowski@...sung.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 Thu, Jul 11, 2024 at 07:02:39PM +0100, Robin Murphy wrote:
> 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.

I'll add, however the plan is to get rid of dummy ops in the near future.

Thanks

> 
> 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);