Pramfs: Write Protection

Quoted 29 lines

Quoted 16 lines

(quoted diff) +16 -0

+/* init_mm.page_table_lock must be held before calling! */
+static void pram_page_writeable(unsigned long addr, int rw)
+{
+ ? ? ? pgd_t *pgdp;
+ ? ? ? pud_t *pudp;
+ ? ? ? pmd_t *pmdp;
+ ? ? ? pte_t *ptep;
+
+ ? ? ? pgdp = pgd_offset_k(addr);
+ ? ? ? if (!pgd_none(*pgdp)) {
+ ? ? ? ? ? ? ? pudp = pud_offset(pgdp, addr);
+ ? ? ? ? ? ? ? if (!pud_none(*pudp)) {
+ ? ? ? ? ? ? ? ? ? ? ? pmdp = pmd_offset(pudp, addr);
+ ? ? ? ? ? ? ? ? ? ? ? if (!pmd_none(*pmdp)) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pte_t pte;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ptep = pte_offset_kernel(pmdp, addr);

Review

Quoted 10 lines

(quoted diff) +10 -0

+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pte = *ptep;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (pte_present(pte)) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pte = rw ? pte_mkwrite(pte) :
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? pte_wrprotect(pte);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_pte(ptep, pte);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? }
+ ? ? ? }
+}

Wow. Don't we want to do this pte walking in mm/ someplace?

Quoted 4 lines

Do you really intend to protect just the PTE in question rather than the entire physical page, regardless of which PTE is talking to it? Maybe I'm missing something.

follow_pfn() ought to be fine for this, optionally follow_pte() could be exported and used.

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+#if defined(CONFIG_ARM) || defined(CONFIG_M68K) || defined(CONFIG_H8300) || \
+ ? ? ? defined(CONFIG_BLACKFIN)
+ ? ? ? /*
+ ? ? ? ?* FIXME: so far only these archs have flush_tlb_kernel_page(),
+ ? ? ? ?* for the rest just use flush_tlb_kernel_range(). Not ideal
+ ? ? ? ?* to use _range() because many archs just flush the whole TLB.
+ ? ? ? ?*/
+ ? ? ? if (end <= start + PAGE_SIZE)
+ ? ? ? ? ? ? ? flush_tlb_kernel_page(start);
+ ? ? ? else
+#endif
+ ? ? ? ? ? ? ? flush_tlb_kernel_range(start, end);
+}

Why not just fix flush_tlb_range()?

Quoted 3 lines

If an arch has a flush_tlb_kernel_page() that works then it stands to reason that the flush_tlb_kernel_range() shouldn't work with minimal effort, no?

flush_tlb_kernel_page() is a new one to me, it doesn't have any mention in Documentation/cachetlb.txt anyways.

Many of the flush_tlb_kernel_range() implementations do ranged checks with tunables to determine whether it is more expensive to selectively flush vs just blowing the entire TLB away.

Likewise, there is no reason why those 4 architectures can not just shove that if (end <= start + PAGE_SIZE) check in the beginning of their flush_tlb_kernel_range() and fall back on flush_tlb_kernel_page() for those cases. Hiding this in generic code is definitely not the way to go.

Quoted 27 lines · Jared Hulbert

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+#if defined(CONFIG_ARM) || defined(CONFIG_M68K) || defined(CONFIG_H8300) || \
+ ? ? ? defined(CONFIG_BLACKFIN)
+ ? ? ? /*
+ ? ? ? ?* FIXME: so far only these archs have flush_tlb_kernel_page(),
+ ? ? ? ?* for the rest just use flush_tlb_kernel_range(). Not ideal
+ ? ? ? ?* to use _range() because many archs just flush the whole TLB.
+ ? ? ? ?*/
+ ? ? ? if (end <= start + PAGE_SIZE)
+ ? ? ? ? ? ? ? flush_tlb_kernel_page(start);
+ ? ? ? else
+#endif
+ ? ? ? ? ? ? ? flush_tlb_kernel_range(start, end);
+}

Why not just fix flush_tlb_range()?

Quoted 3 lines

If an arch has a flush_tlb_kernel_page() that works then it stands to reason that the flush_tlb_kernel_range() shouldn't work with minimal effort, no?

flush_tlb_kernel_page() is a new one to me, it doesn't have any mention in Documentation/cachetlb.txt anyways.

Many of the flush_tlb_kernel_range() implementations do ranged checks with tunables to determine whether it is more expensive to selectively flush vs just blowing the entire TLB away.

Quoted 4 lines

Likewise, there is no reason why those 4 architectures can not just shove that if (end <= start + PAGE_SIZE) check in the beginning of their flush_tlb_kernel_range() and fall back on flush_tlb_kernel_page() for those cases. Hiding this in generic code is definitely not the way to go.

Ok I'll change that function at arch level and I'll remove the ifdef, I'll call only flush_tlb_kernel_page(), but I'd like to know what is the opinion of the arch maintainers to do that.

Quoted 27 lines · Jared Hulbert

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+#if defined(CONFIG_ARM) || defined(CONFIG_M68K) || defined(CONFIG_H8300) || \
+ ? ? ? defined(CONFIG_BLACKFIN)
+ ? ? ? /*
+ ? ? ? ?* FIXME: so far only these archs have flush_tlb_kernel_page(),
+ ? ? ? ?* for the rest just use flush_tlb_kernel_range(). Not ideal
+ ? ? ? ?* to use _range() because many archs just flush the whole TLB.
+ ? ? ? ?*/
+ ? ? ? if (end <= start + PAGE_SIZE)
+ ? ? ? ? ? ? ? flush_tlb_kernel_page(start);
+ ? ? ? else
+#endif
+ ? ? ? ? ? ? ? flush_tlb_kernel_range(start, end);
+}

Why not just fix flush_tlb_range()?

Quoted 3 lines

If an arch has a flush_tlb_kernel_page() that works then it stands to reason that the flush_tlb_kernel_range() shouldn't work with minimal effort, no?

flush_tlb_kernel_page() is a new one to me, it doesn't have any mention in Documentation/cachetlb.txt anyways.

Many of the flush_tlb_kernel_range() implementations do ranged checks with tunables to determine whether it is more expensive to selectively flush vs just blowing the entire TLB away.

Quoted 4 lines

Likewise, there is no reason why those 4 architectures can not just shove that if (end <= start + PAGE_SIZE) check in the beginning of their flush_tlb_kernel_range() and fall back on flush_tlb_kernel_page() for those cases. Hiding this in generic code is definitely not the way to go.

Ok I'll change that function at arch level and I'll remove the ifdef, I'll call only flush_tlb_kernel_page(), but I'd like to know what is the opinion of the arch maintainers to do that.

Quoted 9 lines · Jared Hulbert

Quoted 5 lines

Why not just fix flush_tlb_range()?

If an arch has a flush_tlb_kernel_page() that works then it stands to reason that the flush_tlb_kernel_range() shouldn't work with minimal effort, no?

flush_tlb_kernel_page() is a new one to me, it doesn't have any mention in Documentation/cachetlb.txt anyways.

Quoted 8 lines

Many of the flush_tlb_kernel_range() implementations do ranged checks with tunables to determine whether it is more expensive to selectively flush vs just blowing the entire TLB away.

Likewise, there is no reason why those 4 architectures can not just shove that if (end <= start + PAGE_SIZE) check in the beginning of their flush_tlb_kernel_range() and fall back on flush_tlb_kernel_page() for those cases. Hiding this in generic code is definitely not the way to go.

Ok I'll change that function at arch level and I'll remove the ifdef, I'll call only flush_tlb_kernel_page(), but I'd like to know what is the opinion of the arch maintainers to do that. (Who is the maintainer of H8300 arch?)

Quoted 19 lines

Quoted 9 lines · Jared Hulbert

Quoted 5 lines

Why not just fix flush_tlb_range()?

If an arch has a flush_tlb_kernel_page() that works then it stands to reason that the flush_tlb_kernel_range() shouldn't work with minimal effort, no?

flush_tlb_kernel_page() is a new one to me, it doesn't have any mention in Documentation/cachetlb.txt anyways.

Quoted 8 lines

Many of the flush_tlb_kernel_range() implementations do ranged checks with tunables to determine whether it is more expensive to selectively flush vs just blowing the entire TLB away.

Likewise, there is no reason why those 4 architectures can not just shove that if (end <= start + PAGE_SIZE) check in the beginning of their flush_tlb_kernel_range() and fall back on flush_tlb_kernel_page() for those cases. Hiding this in generic code is definitely not the way to go.

Quoted 5 lines

Ok I'll change that function at arch level and I'll remove the ifdef, I'll call only flush_tlb_kernel_page(), but I'd like to know what is the opinion of the arch maintainers to do that.  (Who is the maintainer of H8300 arch?)

No, you should call flush_tlb_kernel_range() and just fix up the flush_tlb_kernel_range() calls to wrap in to flush_tlb_kernel_page(). As far as the kernel is concerned, flush_tlb_kernel_page() is not a standard interface, as it has no mention in Documentation/cachetlb.txt. flush_tlb_page() and flush_tlb_kernel_range() on the other hand are both standard interfaces.

H8300 is a nommu platform, so it has no TLB to flush. Yoshinori Sato is the maintainer. Consult the MAINTAINERS file, that's what it is there for.

2009/6/18 Paul Mundt <[email protected]>:

Quoted 2 lines

H8300 is a nommu platform, so it has no TLB to flush. Yoshinori Sato is the maintainer. Consult the MAINTAINERS file, that's what it is there for.

I know the MAINTAINERS file but for h8300 there isn't an exactly indication (/arch/h8300 as for the other archs).

2009/6/18 Paul Mundt <[email protected]>:

Quoted 2 lines

H8300 is a nommu platform, so it has no TLB to flush. Yoshinori Sato is the maintainer. Consult the MAINTAINERS file, that's what it is there for.

I know the MAINTAINERS file but for h8300 there isn't an exactly indication (/arch/h8300 as for the other archs).

Quoted 19 lines

Quoted 9 lines · Jared Hulbert

Quoted 5 lines

Why not just fix flush_tlb_range()?

If an arch has a flush_tlb_kernel_page() that works then it stands to reason that the flush_tlb_kernel_range() shouldn't work with minimal effort, no?

flush_tlb_kernel_page() is a new one to me, it doesn't have any mention in Documentation/cachetlb.txt anyways.

Quoted 8 lines

Many of the flush_tlb_kernel_range() implementations do ranged checks with tunables to determine whether it is more expensive to selectively flush vs just blowing the entire TLB away.

Likewise, there is no reason why those 4 architectures can not just shove that if (end <= start + PAGE_SIZE) check in the beginning of their flush_tlb_kernel_range() and fall back on flush_tlb_kernel_page() for those cases. Hiding this in generic code is definitely not the way to go.

Quoted 5 lines

Ok I'll change that function at arch level and I'll remove the ifdef, I'll call only flush_tlb_kernel_page(), but I'd like to know what is the opinion of the arch maintainers to do that.  (Who is the maintainer of H8300 arch?)

No, you should call flush_tlb_kernel_range() and just fix up the flush_tlb_kernel_range() calls to wrap in to flush_tlb_kernel_page(). As far as the kernel is concerned, flush_tlb_kernel_page() is not a standard interface, as it has no mention in Documentation/cachetlb.txt. flush_tlb_page() and flush_tlb_kernel_range() on the other hand are both standard interfaces.

H8300 is a nommu platform, so it has no TLB to flush. Yoshinori Sato is the maintainer. Consult the MAINTAINERS file, that's what it is there for.

Quoted 9 lines · Jared Hulbert

Quoted 5 lines

Why not just fix flush_tlb_range()?

If an arch has a flush_tlb_kernel_page() that works then it stands to reason that the flush_tlb_kernel_range() shouldn't work with minimal effort, no?

flush_tlb_kernel_page() is a new one to me, it doesn't have any mention in Documentation/cachetlb.txt anyways.

Quoted 8 lines

Many of the flush_tlb_kernel_range() implementations do ranged checks with tunables to determine whether it is more expensive to selectively flush vs just blowing the entire TLB away.

Likewise, there is no reason why those 4 architectures can not just shove that if (end <= start + PAGE_SIZE) check in the beginning of their flush_tlb_kernel_range() and fall back on flush_tlb_kernel_page() for those cases. Hiding this in generic code is definitely not the way to go.

Ok I'll change that function at arch level and I'll remove the ifdef, I'll call only flush_tlb_kernel_page(), but I'd like to know what is the opinion of the arch maintainers to do that. (Who is the maintainer of H8300 arch?)