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

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

[Marco]

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.


Ok I can create a new exported function follow_pte().

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

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?)

Marco

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

[Mike Frysinger]

On Wed, Jun 17, 2009 at 12:58, Marco wrote:
> Jared Hulbert wrote:
>> > > +#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.
>
> 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.

considering Blackfin defines flush_tlb_kernel_page() to BUG(), i dont
think we care what happens.  we dont have a MMU, so all tlb funcs ->
BUG().  presumably this code shouldnt have been compiled in the first
place for us.
-mike

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

[Paul Mundt]

On Wed, Jun 17, 2009 at 06:58:00PM +0200, Marco wrote:
> Jared Hulbert wrote:
> > > 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.
> 
> 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.

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

[Marco Stornelli]

2009/6/18 Paul Mundt <lethal@linux-sh.org>:
> On Wed, Jun 17, 2009 at 06:58:00PM +0200, Marco wrote:
>> Jared Hulbert wrote:
>> > > 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.
>>
>> 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.

Oops, my fault. I meant flush_tlb_kernel_range not the page version,
sorry. I agree with you.

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

Marco

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

[Paul Mundt]

On Thu, Jun 18, 2009 at 08:24:35AM +0200, Marco Stornelli wrote:
> 2009/6/18 Paul Mundt <lethal@linux-sh.org>:
> > 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).
> 
The file patterns are a new thing, I guess not all of the platforms were
updated. In any event:

UCLINUX FOR RENESAS H8/300 (H8300)
P:      Yoshinori Sato
M:      ysato@users.sourceforge.jp
W:      http://uclinux-h8.sourceforge.jp/
S:      Supported

Which is basically the first thing you find when looking for H8.

[PATCH 13/14] Pramfs: Write protection

[Marco]

From: Marco Stornelli <marco.stornelli@gmail.com>

Write protection.

Signed-off-by: Marco Stornelli <marco.stornelli@gmail.com>
---

diff -uprN linux-2.6.30-orig/fs/pramfs/wprotect.c linux-2.6.30/fs/pramfs/wprotect.c
--- linux-2.6.30-orig/fs/pramfs/wprotect.c	1970-01-01 01:00:00.000000000 +0100
+++ linux-2.6.30/fs/pramfs/wprotect.c	2009-06-13 12:54:16.000000000 +0200
@@ -0,0 +1,84 @@
+/*
+ * FILE NAME fs/pramfs/wprotect.c
+ *
+ * BRIEF DESCRIPTION
+ *
+ * Write protection for the filesystem pages.
+ *
+ * Copyright 2009 Marco Stornelli <marco.stornelli@gmail.com>
+ * Copyright 2003 Sony Corporation
+ * Copyright 2003 Matsushita Electric Industrial Co., Ltd.
+ * 2003-2004 (c) MontaVista Software, Inc. , Steve Longerbeam
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/mm.h>
+#include <asm/pgtable.h>
+#include <asm/pgalloc.h>
+#include <asm/tlbflush.h>
+#include "pram_fs.h"
+
+/* 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);
+				}
+			}
+		}
+	}
+}
+
+/* init_mm.page_table_lock must be held before calling! */
+void pram_writeable(void *vaddr, unsigned long size, int rw)
+{
+	unsigned long addr = (unsigned long)vaddr & PAGE_MASK;
+	unsigned long end = (unsigned long)vaddr + size;
+	unsigned long start = addr;
+
+	do {
+		pram_page_writeable(addr, rw);
+		addr += PAGE_SIZE;
+	} while (addr && (addr < end));
+
+
+	/*
+	 * NOTE: we will always flush just one page (one TLB
+	 * entry) except possibly in one case: when a new
+	 * filesystem is initialized at mount time, when pram_read_super
+	 * calls pram_lock_range to make the super block, inode
+	 * table, and bitmap writeable.
+	 */
+#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);
+}

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

[Jared Hulbert]

> +/* 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.

> +/* init_mm.page_table_lock must be held before calling! */
> +void pram_writeable(void *vaddr, unsigned long size, int rw)
> +{
> +       unsigned long addr = (unsigned long)vaddr & PAGE_MASK;
> +       unsigned long end = (unsigned long)vaddr + size;
> +       unsigned long start = addr;
> +
> +       do {
> +               pram_page_writeable(addr, rw);
> +               addr += PAGE_SIZE;
> +       } while (addr && (addr < end));
> +
> +
> +       /*
> +        * NOTE: we will always flush just one page (one TLB
> +        * entry) except possibly in one case: when a new
> +        * filesystem is initialized at mount time, when pram_read_super
> +        * calls pram_lock_range to make the super block, inode
> +        * table, and bitmap writeable.
> +        */
> +#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?

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

[Paul Mundt]

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.