[PATCH 1/11] oprofile: add check_user_page_readable()

[PATCH 1/11] oprofile: add check_user_page_readable()

[Greg Banks]

[-- Attachment #1: Type: text/plain, Size: 95 bytes --]


-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.


[-- Attachment #2: check-user-page --]
[-- Type: text/plain, Size: 2343 bytes --]

Add check_user_page_readable() for kernel modules which need
to follow user space addresses but can't use get_user().

Signed-off-by: John Levon <levon@movementarian.org>
Signed-off-by: Greg Banks <gnb@melbourne.sgi.com>
---

 include/linux/mm.h |    1 +
 mm/memory.c        |   20 ++++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

Index: linux/include/linux/mm.h
===================================================================
--- linux.orig/include/linux/mm.h	2004-11-06 01:12:07.%N +1100
+++ linux/include/linux/mm.h	2004-11-07 17:59:40.%N +1100
@@ -789,6 +789,7 @@ extern struct page * vmalloc_to_page(voi
 extern unsigned long vmalloc_to_pfn(void *addr);
 extern struct page * follow_page(struct mm_struct *mm, unsigned long address,
 		int write);
+extern int check_user_page_readable(struct mm_struct *mm, unsigned long address);
 int remap_pfn_range(struct vm_area_struct *, unsigned long,
 		unsigned long, unsigned long, pgprot_t);
 
Index: linux/mm/memory.c
===================================================================
--- linux.orig/mm/memory.c	2004-11-06 01:11:57.%N +1100
+++ linux/mm/memory.c	2004-11-07 17:59:40.%N +1100
@@ -746,8 +746,8 @@ void zap_page_range(struct vm_area_struc
  * Do a quick page-table lookup for a single page.
  * mm->page_table_lock must be held.
  */
-struct page *
-follow_page(struct mm_struct *mm, unsigned long address, int write) 
+static struct page *
+__follow_page(struct mm_struct *mm, unsigned long address, int read, int write)
 {
 	pml4_t *pml4;
 	pgd_t *pgd;
@@ -790,6 +790,8 @@ follow_page(struct mm_struct *mm, unsign
 	if (pte_present(pte)) {
 		if (write && !pte_write(pte))
 			goto out;
+		if (read && !pte_read(pte))
+			goto out;
 		pfn = pte_pfn(pte);
 		if (pfn_valid(pfn)) {
 			page = pfn_to_page(pfn);
@@ -804,6 +806,20 @@ out:
 	return NULL;
 }
 
+struct page *
+follow_page(struct mm_struct *mm, unsigned long address, int write) 
+{
+	return __follow_page(mm, address, /*read*/0, write);
+}
+
+int
+check_user_page_readable(struct mm_struct *mm, unsigned long address)
+{
+	return __follow_page(mm, address, /*read*/1, /*write*/0) != NULL;
+}
+
+EXPORT_SYMBOL(check_user_page_readable);
+
 /* 
  * Given a physical address, is there a useful struct page pointing to
  * it?  This may become more complex in the future if we start dealing

Re: [PATCH 1/11] oprofile: add check_user_page_readable()

[Andrew Morton]

Greg Banks <gnb@melbourne.sgi.com> wrote:
>
> Add check_user_page_readable() for kernel modules which need
>  to follow user space addresses but can't use get_user().

Strange.  What is the usage pattern for this?  And why is that usage
pattern not racy in the presence of paging activity?

Did you consider use_mm(), in conjunction with get_user()?

Re: [PATCH 1/11] oprofile: add check_user_page_readable()

[Greg Banks]

On Tue, 2004-11-09 at 22:04, Andrew Morton wrote:
> Greg Banks <gnb@melbourne.sgi.com> wrote:
> >
> > Add check_user_page_readable() for kernel modules which need
> >  to follow user space addresses but can't use get_user().
> 
> Strange.  What is the usage pattern for this?

The i386 callgraph code attempts to follow user stacks, from
an interrupt (perfmon, NMI, or timer) where get_user() is
explicitly disallowed by Documentation/DocBook/kernel-locking.tmpl.
AFAICS from the ia64 and i386 page fault handlers get_user should
"just work" and return -EFAULT if the page isn't resident or
readable, but the doc says...

Currently this is only an issue for i386.  The ia64 code doesn't
even try to look at user stacks (shudder).

>   And why is that usage
> pattern not racy in the presence of paging activity?

The i386 backtracer takes the &current->mm->page_table_lock, and
just drops out of the trace early if a page isn't resident.  It
doesn't expect or try to page in.  After all this is only statistical
sampling not write() data.

> 
> Did you consider use_mm(), in conjunction with get_user()?

No, but glancing at use_mm() the comment says

 *      (Note: this routine is intended to be called only
 *      from a kernel thread context)

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

Re: [PATCH 1/11] oprofile: add check_user_page_readable()

[Andrew Morton]

Greg Banks <gnb@melbourne.sgi.com> wrote:
>
> On Tue, 2004-11-09 at 22:04, Andrew Morton wrote:
> > Greg Banks <gnb@melbourne.sgi.com> wrote:
> > >
> > > Add check_user_page_readable() for kernel modules which need
> > >  to follow user space addresses but can't use get_user().
> > 
> > Strange.  What is the usage pattern for this?
> 
> The i386 callgraph code attempts to follow user stacks, from
> an interrupt (perfmon, NMI, or timer)

Yikes.

> where get_user() is
> explicitly disallowed by Documentation/DocBook/kernel-locking.tmpl.
> AFAICS from the ia64 and i386 page fault handlers get_user should
> "just work" and return -EFAULT if the page isn't resident or
> readable, but the doc says...
> 
> Currently this is only an issue for i386.  The ia64 code doesn't
> even try to look at user stacks (shudder).
> 
> >   And why is that usage
> > pattern not racy in the presence of paging activity?
> 
> The i386 backtracer takes the &current->mm->page_table_lock,

But that cannot be taken from interrupt context.  A trylock would be OK I
guess.

> and
> just drops out of the trace early if a page isn't resident.  It
> doesn't expect or try to page in.  After all this is only statistical
> sampling not write() data.
> 
> > 
> > Did you consider use_mm(), in conjunction with get_user()?
> 
> No, but glancing at use_mm() the comment says
> 
>  *      (Note: this routine is intended to be called only
>  *      from a kernel thread context)

It could probably be made to work from a normal process, but not from
interrupt context.

I guess I should apply the patches and take a closer look.

Re: [PATCH 1/11] oprofile: add check_user_page_readable()

[Greg Banks]

On Tue, 2004-11-09 at 22:26, Andrew Morton wrote:
> > The i386 callgraph code attempts to follow user stacks, from
> > an interrupt (perfmon, NMI, or timer)
> 
> Yikes.

There are a number of problems with this, for example modern libcs
built with -fomit-frame-pointer limit it's usefulness.  But when it
does get meaningful traces it's really quite useful.

> > >   And why is that usage
> > > pattern not racy in the presence of paging activity?
> > 
> > The i386 backtracer takes the &current->mm->page_table_lock,
> 
> But that cannot be taken from interrupt context.  A trylock would be OK I
> guess.

The code reads:

#ifdef CONFIG_SMP
        if (!spin_trylock(&current->mm->page_table_lock))
                return;
#endif

i.e. it tries to get the lock and abandons the trace if it can't.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.