Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 10 Aug 2017 10:35:58 -0600
From: Tycho Andersen <tycho@...ker.com>
To: Konrad Rzeszutek Wilk <konrad@...nok.org>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	kernel-hardening@...ts.openwall.com,
	Marco Benatto <marco.antonio.780@...il.com>,
	Juerg Haefliger <juerg.haefliger@...onical.com>,
	Juerg Haefliger <juerg.haefliger@....com>
Subject: Re: [PATCH v5 08/10] arm64/mm: Add support for XPFO to swiotlb

Hi Konrad,

Thanks for taking a look!

On Thu, Aug 10, 2017 at 09:11:12AM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 09, 2017 at 02:07:53PM -0600, Tycho Andersen wrote:
> > +
> > +inline void xpfo_dma_map_unmap_area(bool map, const void *addr, size_t size,
> 
> And inline? You sure about that? It is quite a lot of code to duplicate
> in all of those call-sites.
>
> > +				    int dir)
> 
> Not enum dma_data_direction ?

I'll fix both of these, thanks.

> > +{
> > +	unsigned long flags;
> > +	struct page *page = virt_to_page(addr);
> > +
> > +	/*
> > +	 * +2 here because we really want
> > +	 * ceil(size / PAGE_SIZE), not floor(), and one extra in case things are
> > +	 * not page aligned
> > +	 */
> > +	int i, possible_pages = size / PAGE_SIZE + 2;
> 
> Could you use the PAGE_SHIFT macro instead? Or PFN_UP ?
> 
> And there is also the PAGE_ALIGN macro...
> 
> > +	void *buf[possible_pages];
> 
> What if you just did 'void *buf[possible_pages] = { };'
> 
> Wouldn't that eliminate the need for the memset?

gcc doesn't seem to like that:

arch/arm64//mm/xpfo.c: In function ‘xpfo_dma_map_unmap_area’:
arch/arm64//mm/xpfo.c:80:2: error: variable-sized object may not be initialized
  void *buf[possible_pages] = {};
  ^~~~

I thought about putting this on the heap, but there's no real way to return
errors here if e.g. the kmalloc fails. I'm open to suggestions though, because
this is ugly.

> > +
> > +	memset(buf, 0, sizeof(void *) * possible_pages);
> > +
> > +	local_irq_save(flags);
> 
> ?? Why?

I'm afraid I don't really know. I'll drop it for the next version, thanks!

Tycho

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.