[PATCH] aio: fix D-cache aliasing issues

[Helge Deller]

When a user page mapping is released via kunmap*() functions, the D-cache needs
to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.

This patch fixes aio on the parisc platform (and probably others).

Signed-off-by: Helge Deller <del...@gmx.de>
To: Benjamin LaHaise <bc...@kvack.org>
To: linu...@kvack.org
Cc: sta...@vger.kernel.org # 3.12+

diff --git a/fs/aio.c b/fs/aio.c
index 823efcb..2181821 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -514,6 +514,7 @@ static void free_ioctx(struct work_struct *work)
atomic_add(avail, &ctx->reqs_available);
ring->head = ring->tail;
kunmap_atomic(ring);
+ flush_dcache_page(ctx->ring_pages[0]);

if (atomic_read(&ctx->reqs_available) >= ctx->nr_events - 1)
break;
@@ -568,6 +569,7 @@ static int ioctx_add_table(struct kioctx *ctx, struct mm_struct *mm)
ring = kmap_atomic(ctx->ring_pages[0]);
ring->id = ctx->id;
kunmap_atomic(ring);
+ flush_dcache_page(ctx->ring_pages[0]);
return 0;
}

@@ -1032,6 +1034,7 @@ static long aio_read_events_ring(struct kioctx *ctx,
head = ring->head;
tail = ring->tail;
kunmap_atomic(ring);
+ flush_dcache_page(ctx->ring_pages[0]);

pr_debug("h%u t%u m%u\n", head, tail, ctx->nr_events);


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

[James Bottomley]

On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote:
> When a user page mapping is released via kunmap*() functions, the D-cache needs
> to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
>
> This patch fixes aio on the parisc platform (and probably others).

This should be flush_kernel_dcache_page(). flush_dcache_page() is for
full coherency but for unmap, we know the page was coherent going in and
may have been modified by the kernel, so only the kernel view needs to
be sync'd. Technically, by the kernel API, the flush should be done
*before* unmapping. This would have mattered on parisc until we did
flush via tmpalias which means we no-longer care if the mapping for the
flush exists or not because we always recreate it via the tmpalias
pages.

James

> To unsubscribe from this list: send the line "unsubscribe linux-parisc" in

[Simon Baatz]

On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote:
> On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote:
> > When a user page mapping is released via kunmap*() functions, the D-cache needs
> > to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
> >
> > This patch fixes aio on the parisc platform (and probably others).
>
> This should be flush_kernel_dcache_page(). flush_dcache_page() is for
> full coherency but for unmap, we know the page was coherent going in and
> may have been modified by the kernel, so only the kernel view needs to
> be sync'd. Technically, by the kernel API, the flush should be done
> *before* unmapping. This would have mattered on parisc until we did
> flush via tmpalias which means we no-longer care if the mapping for the
> flush exists or not because we always recreate it via the tmpalias
> pages.

On ARM, flush_kernel_dcache_page() actually assumes that the page is
mapped. It avoids double flushing of highmem pages by not flushing
in those cases where kunmap_atomic() already takes care of flushing.


- Simon

[Benjamin LaHaise]

On Sat, Nov 16, 2013 at 09:07:18PM +0100, Simon Baatz wrote:
> On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote:
> > On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote:
> > > When a user page mapping is released via kunmap*() functions, the D-cache needs
> > > to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
> > >
> > > This patch fixes aio on the parisc platform (and probably others).
> >
> > This should be flush_kernel_dcache_page(). flush_dcache_page() is for
> > full coherency but for unmap, we know the page was coherent going in and
> > may have been modified by the kernel, so only the kernel view needs to
> > be sync'd. Technically, by the kernel API, the flush should be done
> > *before* unmapping. This would have mattered on parisc until we did
> > flush via tmpalias which means we no-longer care if the mapping for the
> > flush exists or not because we always recreate it via the tmpalias
> > pages.
>
> On ARM, flush_kernel_dcache_page() actually assumes that the page is
> mapped. It avoids double flushing of highmem pages by not flushing
> in those cases where kunmap_atomic() already takes care of flushing.

Helge -- are you going to resubmit a version of this patch that makes the
recommended change?

-ben

>
>
> - Simon

--
"Thought is the essence of where you are now."

[James Bottomley]

On Sat, 2013-11-16 at 21:07 +0100, Simon Baatz wrote:
> On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote:
> > On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote:
> > > When a user page mapping is released via kunmap*() functions, the D-cache needs
> > > to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
> > >
> > > This patch fixes aio on the parisc platform (and probably others).
> >
> > This should be flush_kernel_dcache_page(). flush_dcache_page() is for
> > full coherency but for unmap, we know the page was coherent going in and
> > may have been modified by the kernel, so only the kernel view needs to
> > be sync'd. Technically, by the kernel API, the flush should be done
> > *before* unmapping. This would have mattered on parisc until we did
> > flush via tmpalias which means we no-longer care if the mapping for the
> > flush exists or not because we always recreate it via the tmpalias
> > pages.
>
> On ARM, flush_kernel_dcache_page() actually assumes that the page is
> mapped. It avoids double flushing of highmem pages by not flushing
> in those cases where kunmap_atomic() already takes care of flushing.

On Parisc, kmap/kunmap is currently a nop. However, if we ever
implemented highmem, we would also need to flush before we unmap, which
is why the flush needs to go before the kunmap.

I've got to say on all of this, we've implemented the most inane set of
primitives. It would be much easier if kmap/kunmap just took care of
all of this. kmap should bring userspace into coherence (because why
else would we be kmapping) and kunmap should force coherence on the
kernel address ... and then no-one would need to worry when to and when
not to flush.

James

[James Bottomley]

On Sat, 2013-11-16 at 17:32 -0500, John David Anglin wrote:

> Not quite. On PA8800/PA8900, we currently do a flush in kunmap.
>
> I'm fairly certain from discussion with Helge that he saw this bug on
> a C8000
> with PA8800 processor. In that case, adding a call to
> flush_kernel_dcache_page()
> just duplicates the call in kunmap().

Yes, if that's true it won't be the solution.

> Could be wrong though.
>
> The problem is the coherence with userspace. That's why
> flush_dcache_page()
> is called in several places in this file. I agree with you that this
> should be done
> before.

If we need coherence with userspace, we can't do it as we unmap because
by then we've used the data in the kernel. We have to do it as we kmap,
because that's when we're preparing to use the data. So the solution,
if this is the problem, would be flush_dcache_page() just after kmap.

[John David Anglin]

On 16-Nov-13, at 5:06 PM, James Bottomley wrote:

Not quite. On PA8800/PA8900, we currently do a flush in kunmap.

I'm fairly certain from discussion with Helge that he saw this bug on
a C8000
with PA8800 processor. In that case, adding a call to
flush_kernel_dcache_page()

just duplicates the call in kunmap(). Could be wrong though.

The problem is the coherence with userspace. That's why
flush_dcache_page()
is called in several places in this file. I agree with you that this
should be done
before.

>

> I've got to say on all of this, we've implemented the most inane set
> of
> primitives. It would be much easier if kmap/kunmap just took care of
> all of this. kmap should bring userspace into coherence (because why
> else would we be kmapping) and kunmap should force coherence on the
> kernel address ... and then no-one would need to worry when to and
> when
> not to flush.

I'm working on adding a flush to our kmap code to bring userspace into
coherence on PA8800/PA8900. It's ugly because it requires looping over
vma's as we do in in flush_dcache_page().

Dave
--
John David Anglin dave....@bell.net

[John David Anglin]

The calls may be needed for the two cases where the page is changed,
but I don't
know how to avoid double flush on PA8800/PA8900 without changing the
primatives.

Dave
--
John David Anglin dave....@bell.net

[Helge Deller]

On 11/16/2013 09:09 PM, Benjamin LaHaise wrote:
> On Sat, Nov 16, 2013 at 09:07:18PM +0100, Simon Baatz wrote:
>> On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote:
>>> On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote:
>>>> When a user page mapping is released via kunmap*() functions, the D-cache needs
>>>> to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
>>>>
>>>> This patch fixes aio on the parisc platform (and probably others).
>>>
>>> This should be flush_kernel_dcache_page(). flush_dcache_page() is for
>>> full coherency but for unmap, we know the page was coherent going in and
>>> may have been modified by the kernel, so only the kernel view needs to
>>> be sync'd. Technically, by the kernel API, the flush should be done
>>> *before* unmapping. This would have mattered on parisc until we did
>>> flush via tmpalias which means we no-longer care if the mapping for the
>>> flush exists or not because we always recreate it via the tmpalias
>>> pages.
>>
>> On ARM, flush_kernel_dcache_page() actually assumes that the page is
>> mapped. It avoids double flushing of highmem pages by not flushing
>> in those cases where kunmap_atomic() already takes care of flushing.
>
> Helge -- are you going to resubmit a version of this patch that makes the
> recommended change?

Sure, I'll do. May need some time for testing the various machine types though.
Maybe in the end you can drop my patch since we might be able to fix it in the
parisc arch code.

@James, Dave: The patch I sent here was not used on the C8000. The C8000 (PA8800)
works out of the box (and it uses the flush_kernel_dcache_page() function which
is provided by kunmap()). It seems we require the flush_kernel_dcache_page() on
the other machines too?

Helge

[James Bottomley]

Well, this is why I think our in-kernel API (not the parisc one) for
kmap/kunmap is faulty. I can't see any valid use of kmap/kunmap where
you don't need to flush. Either you kmapped for reading, in which case
you need to flush user space before the kernel access, or you kmapped
for writing, in which case you need to flush user space just before
mapping (eliminate any dirty user line) and the kernel cache before
unmap (so the modification becomes visible in main memory). I could see
an argument that flushing all the user mappings is expensive so only do
it if necessary, but I think it looks to be always necessary: even if
you're writing only, which means purging the lines above the user
mappings, most hardware will panic (HPMC) if it sees inequivalent
aliases with clashing dirty lines.

I think the reason for the complexity is issues on ARM. The user space
cache lines have to remain empty until the kunmap. On parisc we can
ensure this by purging the tlb entries to prevent speculative move in
(if the user deliberately accesses while we're modifying, it's caveat
emptor), but I don't believe ARM can control this speculation, so they
always need to re-invalidate all of user space before unmap just to
eliminate speculated lines ... in any case, that could still be done as
part of the kunmap API.

James

[Helge Deller]

* James Bottomley <James.B...@HansenPartnership.com>:

Maybe adding a flag to kmap(), e.g. READ_ONLY, WRITE_ONLY or similiar?

> Either you kmapped for reading, in which case
> you need to flush user space before the kernel access, or you kmapped
> for writing, in which case you need to flush user space just before
> mapping (eliminate any dirty user line) and the kernel cache before
> unmap (so the modification becomes visible in main memory).

That's exactly what Dave told me in a private mail earlier.

Below is a patch to the parisc arch code which is originally from Dave.
I only changed kunmap_parisc() to always call
flush_kernel_dcache_page_addr(addr) - independend if we are on a PA8800/8900
CPU or not - which is what you proposed in your original mail.
And yes, this flush_kernel_dcache_page() fixes the aio problems
on all my machines (32- and 64bit kernels, PA7300LC, PA8700, ...).

> I could see
> an argument that flushing all the user mappings is expensive so only do
> it if necessary, but I think it looks to be always necessary: even if
> you're writing only, which means purging the lines above the user
> mappings, most hardware will panic (HPMC) if it sees inequivalent
> aliases with clashing dirty lines.

diff --git a/arch/parisc/include/asm/cacheflush.h b/arch/parisc/include/asm/cacheflush.h
index f0e2784..ce6d605 100644
--- a/arch/parisc/include/asm/cacheflush.h
+++ b/arch/parisc/include/asm/cacheflush.h
@@ -43,6 +43,8 @@ static inline void flush_kernel_dcache_page(struct page *page)
flush_kernel_dcache_page_addr(page_address(page));
}

+void flush_user_dcache_page(struct page *page);
+
#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);
+}
+
+static inline void kunmap_parisc(void *addr)
+{
+ /* always flush kernel dcache, independed of
+ * parisc_requires_coherency(). Needed on CPUs < PA8800/PA8900 */
+ flush_kernel_dcache_page_addr(addr);
+}

static inline void *kmap(struct page *page)
{
might_sleep();
+ kmap_parisc(page);
return page_address(page);
}

@@ -148,6 +159,7 @@ static inline void kunmap(struct page *page)
static inline void *kmap_atomic(struct page *page)
{
pagefault_disable();
+ kmap_parisc(page);
return page_address(page);
}

@@ -160,7 +172,6 @@ static inline void __kunmap_atomic(void *addr)
#define kmap_atomic_prot(page, prot) kmap_atomic(page)
#define kmap_atomic_pfn(pfn) kmap_atomic(pfn_to_page(pfn))
#define kmap_atomic_to_page(ptr) virt_to_page(ptr)
-#endif

#endif /* _PARISC_CACHEFLUSH_H */

diff --git a/arch/parisc/kernel/cache.c b/arch/parisc/kernel/cache.c
index c035673..a7ee7b7 100644
--- a/arch/parisc/kernel/cache.c
+++ b/arch/parisc/kernel/cache.c
@@ -283,7 +283,7 @@ __flush_cache_page(struct vm_area_struct *vma, unsigned long vmaddr,
preempt_enable();
}

-void flush_dcache_page(struct page *page)
+void flush_user_dcache_page(struct page *page)
{
struct address_space *mapping = page_mapping(page);
struct vm_area_struct *mpnt;
@@ -291,13 +291,6 @@ void flush_dcache_page(struct page *page)
unsigned long addr, old_addr = 0;
pgoff_t pgoff;

- if (mapping && !mapping_mapped(mapping)) {
- set_bit(PG_dcache_dirty, &page->flags);
- return;
- }
-
- flush_kernel_dcache_page(page);
-
if (!mapping)
return;

@@ -332,6 +325,20 @@ void flush_dcache_page(struct page *page)
}
flush_dcache_mmap_unlock(mapping);
}
+EXPORT_SYMBOL(flush_user_dcache_page);
+
+void flush_dcache_page(struct page *page)
+{
+ struct address_space *mapping = page_mapping(page);
+
+ if (mapping && !mapping_mapped(mapping)) {
+ set_bit(PG_dcache_dirty, &page->flags);
+ return;
+ }
+
+ flush_kernel_dcache_page(page);
+ flush_user_dcache_page(page);
+}
EXPORT_SYMBOL(flush_dcache_page);

/* Defined in arch/parisc/kernel/pacache.S */
@@ -413,16 +420,6 @@ void copy_user_page(void *vto, void *vfrom, unsigned long vaddr,
}
EXPORT_SYMBOL(copy_user_page);

-#ifdef CONFIG_PA8X00
-
-void kunmap_parisc(void *addr)
-{
- if (parisc_requires_coherency())
- flush_kernel_dcache_page_addr(addr);
-}
-EXPORT_SYMBOL(kunmap_parisc);
-#endif
-
void purge_tlb_entries(struct mm_struct *mm, unsigned long addr)
{
unsigned long flags;

[James Bottomley]

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.

> #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.

James

[John David Anglin]

On 17-Nov-13, at 6:47 PM, Helge Deller wrote:

> I only changed kunmap_parisc() to always call
> flush_kernel_dcache_page_addr(addr) - independend if we are on a
> PA8800/8900
> CPU or not - which is what you proposed in your original mail.

In which case the flushes in clear_user_page() and copy_user_page()
aren't needed.

Dave
--
John David Anglin dave....@bell.net

[John David Anglin]

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.
2) It seems to unnecessarily call flush_kernel_dcache_page().

Dave
--
John David Anglin dave....@bell.net

[James Bottomley]

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

[Simon Baatz]

Hi Helge,

On Sun, Nov 17, 2013 at 10:23:08PM +0100, Helge Deller wrote:
> On 11/16/2013 09:09 PM, Benjamin LaHaise wrote:
> > On Sat, Nov 16, 2013 at 09:07:18PM +0100, Simon Baatz wrote:
> >> On Fri, Nov 15, 2013 at 02:42:05PM -0800, James Bottomley wrote:
> >>> On Fri, 2013-11-15 at 23:05 +0100, Helge Deller wrote:
> >>>> When a user page mapping is released via kunmap*() functions, the D-cache needs
> >>>> to be flushed via flush_dcache_page() to avoid D-cache aliasing issues.
> >>>>
> >>>> This patch fixes aio on the parisc platform (and probably others).
> >>>
> >>> This should be flush_kernel_dcache_page(). flush_dcache_page() is for
> >>> full coherency but for unmap, we know the page was coherent going in and
> >>> may have been modified by the kernel, so only the kernel view needs to
> >>> be sync'd. Technically, by the kernel API, the flush should be done
> >>> *before* unmapping. This would have mattered on parisc until we did
> >>> flush via tmpalias which means we no-longer care if the mapping for the
> >>> flush exists or not because we always recreate it via the tmpalias
> >>> pages.
> >>
> >> On ARM, flush_kernel_dcache_page() actually assumes that the page is
> >> mapped. It avoids double flushing of highmem pages by not flushing
> >> in those cases where kunmap_atomic() already takes care of flushing.
> >
> > Helge -- are you going to resubmit a version of this patch that makes the
> > recommended change?
>
> Sure, I'll do. May need some time for testing the various machine types though.
> Maybe in the end you can drop my patch since we might be able to fix it in the
> parisc arch code.

Could you provide me with the test case(s) you are running to
reproduce the problem? I could test this on aliasing D-cache on ARM
as well. Since kmap/kunmap do not flush in general on ARM, we
might need the explicit flushes here.


- Simon

[Helge Deller]

> Hi Helge,

> Could you provide me with the test case(s) you are running to
> reproduce the problem? I could test this on aliasing D-cache on ARM
> as well. Since kmap/kunmap do not flush in general on ARM, we
> might need the explicit flushes here.

Just google for aio-stress.c (e.g. https://github.com/repoforge/rpms/blob/master/specs/aio-stress/aio-stress.c).
And the aio01 / aio02 testcases from the Linux Test Project (https://github.com/linux-test-project/ltp).

Helge