linux-kernel - Re: [PATCH] aio: fix D-cache aliasing issues

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: <1384739457.2050.28.camel@dabdike.int.hansenpartnership.com>
Date:	Sun, 17 Nov 2013 17:50:57 -0800
From:	James Bottomley <James.Bottomley@...senPartnership.com>
To:	John David Anglin <dave.anglin@...l.net>
Cc:	Helge Deller <deller@....de>, linux-parisc@...r.kernel.org,
	Benjamin LaHaise <bcrl@...ck.org>,
	Simon Baatz <gmbnomis@...il.com>, linux-aio@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] aio: fix D-cache aliasing issues

On Sun, 2013-11-17 at 20:15 -0500, John David Anglin wrote:
> On 17-Nov-13, at 7:52 PM, James Bottomley wrote:
> 
> >> +void flush_user_dcache_page(struct page *page);
> >> +
> >
> > The split into flush_kernel.. and flush_user.. is pointless.  We  
> > have no
> > use for a flush_user_.. API because it's not part of the standard  
> > linux
> > one.  Plus it's going to confuse those who come after us no end  
> > because
> > now we're different from every other architecture.
> 
> I introduced it because it seemed to be what was needed on parisc when
> we map a kernel page on PA8800/PA8900.  We need this to ensure what the
> kernel sees is up to date and also to ensure that we don't create  
> inequivalent
> aliases between the user and kernel mappings.
> 
> >
> >> #define flush_kernel_dcache_range(start,size) \
> >> 	flush_kernel_dcache_range_asm((start), (start)+(size));
> >> /* vmap range flushes and invalidates.  Architecturally, we don't  
> >> need
> >> @@ -125,18 +127,27 @@ flush_anon_page(struct vm_area_struct *vma,  
> >> struct page *page, unsigned long vma
> >> void mark_rodata_ro(void);
> >> #endif
> >>
> >> -#ifdef CONFIG_PA8X00
> >> -/* Only pa8800, pa8900 needs this */
> >> -
> >> #include <asm/kmap_types.h>
> >>
> >> #define ARCH_HAS_KMAP
> >>
> >> -void kunmap_parisc(void *addr);
> >> +static inline void kmap_parisc(struct page *page)
> >> +{
> >> +	if (parisc_requires_coherency())
> >> +		flush_user_dcache_page(page);
> >> +}
> >
> > No ... if we're genuinely moving coherency into kmap/kunmap, this  
> > has to
> > become
> >
> > 	flush_dcache_page(page);
> >
> > unconditionally.
> 
> There are two things about flush_dcache_page(page):
> 
> 1) The flush could be deferred.  I don't think we want that.

Our deferred flush logic seems a bit bogus to me.  We set a flag that
causes a kernel page flush in invalidate_kernel_vmap_range().  However,
when that function is called we checked this on a page by page bases and
if set we flush the page then we then go and do a dcache flush on the
entire range ... that's got to be wrong.  I also can't really see what
the check for mapping && !mapping_mapped() is about.  Mapping != NULL
means shared mappings with user space !mapping_mapped() means no file
backing (so they must be some form of anonymous vma).  Even if we got
the deferred logic right, I don't think we can avoid flushing in the
anon_vma case

> 2) It seems to unnecessarily call flush_kernel_dcache_page().

Remember flush on parisc is flush and invalidate.  We do need to
invalidate the kernel page in flush_dcache_page() just in case there are
any read only speculated cache lines above it.  I agree there shouldn't
be any dirty cache lines above it in kernel space.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/