Re: [PATCH 13/14] Pramfs: Write protection

[Paul Mundt <lethal@linux-sh.org>]

On Tue, Jun 16, 2009 at 07:35:24PM -0700, Jared Hulbert wrote:
> > +/* 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);
> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? 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?
> 
> 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.

> > +#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()?
> 
> 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.