netdev - Re: [PATCH 1/3] uio: introduce UIO_DMA

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]

Date: Sat, 30 Sep 2023 11:08:39 -0700
From: Chris Leech <cleech@...hat.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Christoph Hellwig <hch@....de>, Rasesh Mody <rmody@...vell.com>,
	Ariel Elior <aelior@...vell.com>,
	Sudarsana Kalluru <skalluru@...vell.com>,
	Manish Chopra <manishc@...vell.com>,
	Nilesh Javali <njavali@...vell.com>,
	Manish Rangankar <mrangankar@...vell.com>,
	Jerry Snitselaar <jsnitsel@...hat.com>,
	John Meneghini <jmeneghi@...hat.com>, Lee Duncan <lduncan@...e.com>,
	Mike Christie <michael.christie@...cle.com>,
	Hannes Reinecke <hare@...nel.org>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] uio: introduce UIO_DMA_COHERENT type

On Sat, Sep 30, 2023 at 09:10:10AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 29, 2023 at 10:00:21AM -0700, Chris Leech wrote:
> > Add a UIO memtype specificially for sharing dma_alloc_coherent
> > memory with userspace, backed by dma_mmap_coherent.
> 
> Are you sure that you can share this type of memory with userspace
> safely?  And you are saying what you are doing here, but not why you
> want to do it and who will use it.
> 
> What are the userspace implications for accessing this type of memory?

Thanks for taking the time to look at this Greg.
I'm trying to help Marvell fix a regression with these drivers, by
figuring out what the right way to handle this type of mmap is.

The dma_mmap_coherent API exists for exactly this, so I thought making
the uio interface aware of it made sense.  There are uio drivers sharing
dma_alloc_coherent memory (uio_dmem_genirq, uio_pruss) using
UIO_MEM_PHYS, but that falls apart in the face of an iommu.

> >  struct uio_mem {
> >  	const char		*name;
> > -	phys_addr_t		addr;
> > +	union {
> > +		phys_addr_t	addr;
> > +		dma_addr_t	dma_addr;
> > +	};
> >  	unsigned long		offs;
> >  	resource_size_t		size;
> >  	int			memtype;
> > -	void __iomem		*internal_addr;
> > +	union {
> > +		void __iomem	*internal_addr;
> > +		void 		*virtual_addr;
> > +	};
> > +	struct device		*dma_device;
> 
> Why are you adding a new struct device here?

dma_mmap_coherent wants it.
 
> And why the unions?  How are you going to verify that they are being
> used correctly?  What space savings are you attempting to do here and
> why?

I should have expected that would be questioned, I was being paranoid
about mixing different pointer and address types.  I can remove the
unions if putting a dma_addr_t in addr going to be OK.

- Chris