Messages in this thread Patch in this message | | | Date | Sat, 25 Sep 2004 17:54:04 +0200 | From | Andrea Arcangeli <> | Subject | ptep_establish/establish_pte needs set_pte_atomic and all set_pte must be written in asm |
| |
I was discussing an obscure bug with Martin in the COW handling.
After some thoughts on the issue the only thing that I found potentially buggy in that path is that we're not safe not writing the 8bytes atomically before flushing the tlb. I'm afraid without set_pte_atomic the other cpu in another userspace thread could touch the memory with an empty tlb entry, and load the half-written pte, that may contain the new high bits for the high part of the pfn number, but the old low bits for the low part of the pfn number plus the old ro protection. Potentially giving access to a random physical page to a task (in readonly mode, so no kernel crash or userspace malfunction except the security compromise). I don't expect anybody to be able to exploit it though.
Worthy to note is that we're buggy in all set_pte implementations since, all archs would need to also implement the set_pte in assembler to make sure the C language doesn't write it byte-by-byte which would break the SMP in the other thread. On ppc64 where a problem triggered (possibily unrelated to this) the pte is an unsigned long and it's being updated by set_pte with this:
*ptep = __pte(pte_val(pte)) & ~_PAGE_HPTEFLAGS
(note pte_clear would be fine to be still in C, pte clear is guaranteed to run on not present ptes, so we don't race with other threads, it's only set_pte that should always be written in assembler in the last opcode that writes in the pte)
We don't need an SMP lock, we only need to write 4 or 8 bytes at once (a plain movl in x86 would do the trick). That's all we need. (and in theory only for SMP, but UP will not get any slowdown since no lock on the bus will be necessary, we're the only writer thanks to the page_table_lock, but there are other readers running in userspace in SMP, hence the need of atomicity)
pte_clear would not be safe to call inside ptep_establish for the same reason (pte_clear is not atomic and it doesn't need to be atomic unlike set_pte), while something like this should be fine:
ptep_get_and_clear set_pte flush_tlb
but it doesn't worht it since all other archs supporting SMP in their architecture will have to change set_pte to an assembly version, so for them set_pte_atomic will be defined to set_pte.
The x86 set_pte itself should be changed to:
static inline void set_pte(pte_t *ptep, pte_t pte) { ptep->pte_high = pte.pte_high; smp_wmb(); do this in a single not locked movl -> (ptep->pte_low = pte.pte_low); }
and for x86 the set_pte_atomic will remain the same as today, so the below patch seems the right long term fix (even if it breaks all archs but x86).
Comments?
Index: linux-2.5/include/asm-generic/pgtable.h =================================================================== RCS file: /home/andrea/crypto/cvs/linux-2.5/include/asm-generic/pgtable.h,v retrieving revision 1.8 diff -u -p -r1.8 pgtable.h --- linux-2.5/include/asm-generic/pgtable.h 29 Jul 2004 06:01:30 -0000 1.8 +++ linux-2.5/include/asm-generic/pgtable.h 25 Sep 2004 15:16:50 -0000 @@ -15,7 +15,7 @@ */ #define ptep_establish(__vma, __address, __ptep, __entry) \ do { \ - set_pte(__ptep, __entry); \ + set_pte_atomic(__ptep, __entry); \ flush_tlb_page(__vma, __address); \ } while (0) #endif - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |