2004-11-09 10:41:29

by Greg Banks

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


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


Attachments:
check-user-page (2.29 kB)

2004-11-09 11:07:11

by Andrew Morton

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

Greg Banks <[email protected]> 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()?

2004-11-09 11:33:21

by Andrew Morton

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

Greg Banks <[email protected]> wrote:
>
> On Tue, 2004-11-09 at 22:04, Andrew Morton wrote:
> > Greg Banks <[email protected]> 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.

2004-11-09 11:33:22

by Greg Banks

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

On Tue, 2004-11-09 at 22:04, Andrew Morton wrote:
> Greg Banks <[email protected]> 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.


2004-11-09 11:53:24

by Greg Banks

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

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.