mem: Fix bugs in the PageTable cache that allow accessing uninitialized data

Information
Submitter:	Mitch Hayenga
Repository:	gem5
Branch:
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Fixes two bugs relating to software caching of PageTable entries.

The existing implementation can read uninitialized data or stale information from the cached PageTable entries.

1) Add a valid bit for the cache entries.  Simply using zero for the virtual address to signify invalid entries is not sufficient.  Speculative, wrong-path accesses frequently access page zero.  The current implementation would return a uninitialized TLB entry when address zero was accessed and the PageTable cache entry was invalid.

2) When unmapping/mapping/remaping a page, invalidate the corresponding PageTable cache entry if one already exists.

Testing Done

Issue Summary

Description	From	Last Updated	Status
I would suggest calling this eraseEntry eraseCacheEntry as it does not erase the entire cache.	Andreas Hansson	April 19, 2013, 9:41 p.m.	Open

Change Summary:

Modified remap and map to also invalidate the page table cache if need be.

Description:

		Fixes two bugs relating to software caching of PageTable entries.

		The existing implementation can read uninitialized data or stale information from the cached PageTable entries.

		1) Add a valid bit for the cache entries. Simply using zero for the virtual address to signify invalid entries is not sufficient. Speculative, wrong-path accesses frequently access page zero. The current implementation would return a uninitialized TLB entry when address zero was accessed and the PageTable cache entry was invalid.

~		2) When unmapping a page, also invalidate the corresponding PageTable cache entry if one exists.
	~	2) When unmapping/mapping/remaping a page, invalidate the corresponding PageTable cache entry if one already exists.

Diff:

Revision 2 (+30 -7)

Show changes

	src/mem/page_table.hh
	src/mem/page_table.cc

LGTM. How does it affect the regressions?

src/mem/page_table.hh (Diff revision 2)

I would suggest calling this eraseEntry eraseCacheEntry as it does not erase the entire cache.

Show all issues

Cool! I wonder if this is what causes the long SE regressions to change a little bit every so often. I agree with Andreas' name suggestion, but otherwise thanks! How did you figure it out?

Mitch Hayenga April 20, 2013, 6:01 a.m. (April 20, 2013, 6:01 a.m.)

One of my checkpoints worked on the cluster but failed on my laptop.  The only difference was the build environment and it failed fairly quickly, so I did a quick run with valgrind.  I really hit the first bug, but noticed the second bug while fixing it.

Here's what happened:
1. Checkpoint is restored.  BTB holds zeros, page table cache holds zeros as well.
2. Program starts running, a branch redirects fetch to address zero (indirect unconditional)

4678654008000: system.cpu.fetch: [tid:0]: Instruction is:   uopReg_uop.w   pc, r35
4678654008000: system.cpu.fetch: [tid:0]: [sn:85]:  Branch predicted to be taken to (0x4=>0x8).(0=>1).


3. Zero hits in the pagetable cache, so no page fault is signaled

4678654008000: system.cpu.workload: Translating: 0->0
4678654008000: system.cpu.itb: Translation returning delay=0 fault=0

4. Depending on the value of the invalid TLB entry either the warning at src/cpu/o3/fetch_impl.hh:639 is issued and fetch stalls forever (this really should be a panic, not a warning) or the frontend continues accessing and creating invalid instructions (accessing uninitialized data) until the original branch properly resolves and squashes everything.

Change Summary:

Ok, fixed things up.  Changed function name to eraseCache to eraseCacheEntry.  The ARM/quick/se regressions still pass, but I haven't tried any others.

PS: Someone else will have to push this.

Diff:

Revision 3 (+30 -7)

Show changes

	src/mem/page_table.hh
	src/mem/page_table.cc

i'll push it shortly.

Mitch Hayenga April 20, 2013, 7:17 a.m. (April 20, 2013, 7:17 a.m.)

I just realized something else last minute.  I don't really know the use case for remap.  Should we also invalidate the new_vaddr in the cache?  I don't know if a program would ever map onto something else it had already mapped.

Ali Saidi April 20, 2013, 8:53 a.m. (April 20, 2013, 8:53 a.m.)

remap is used exclusively by the remap syscall which is going to allocate new memory in our implementation. So I don't think it's needed.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Change Summary:

Description:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)