why "probe_kernel_address()", not "probe_user_address()"?

why "probe_kernel_address()", not "probe_user_address()"?

[Robert P. J. Day]

  it seems odd that the purpose of the "probe_kernel_address()" macro
is, in fact, to probe a *user* address (from linux/uaccess.h):

#define probe_kernel_address(addr, retval)              \
        ({                                              \
                long ret;                               \
                                                        \
                inc_preempt_count();                    \
                ret = __get_user(retval, addr);         \
                dec_preempt_count();                    \
                ret;                                    \
        })

  given that that routine is referenced only 5 places in the entire
source tree, wouldn't it be more meaningful to use a more appropriate
name?

pedantically yours,
rday

Re: why "probe_kernel_address()", not "probe_user_address()"?

[Andrew Morton]

On Sat, 28 Oct 2006 11:56:24 -0400 (EDT)
"Robert P. J. Day" <rpjday@mindspring.com> wrote:

> 
>   it seems odd that the purpose of the "probe_kernel_address()" macro
> is, in fact, to probe a *user* address (from linux/uaccess.h):
> 
> #define probe_kernel_address(addr, retval)              \
>         ({                                              \
>                 long ret;                               \
>                                                         \
>                 inc_preempt_count();                    \
>                 ret = __get_user(retval, addr);         \
>                 dec_preempt_count();                    \
>                 ret;                                    \
>         })
> 
>   given that that routine is referenced only 5 places in the entire
> source tree, wouldn't it be more meaningful to use a more appropriate
> name?
> 

You'll notice that all callers are indeed probing kernel addresses.  The
function _could_ be used for user addresses and could perhaps be called
probe_address().

One of the reasons this wrapper exists is to communicate that the
__get_user() it is in fact not being used to access user memory.

Re: why "probe_kernel_address()", not "probe_user_address()"?

[Robert P. J. Day]

On Sat, 28 Oct 2006, Andrew Morton wrote:

> On Sat, 28 Oct 2006 11:56:24 -0400 (EDT)
> "Robert P. J. Day" <rpjday@mindspring.com> wrote:
>
> >
> >   it seems odd that the purpose of the "probe_kernel_address()" macro
> > is, in fact, to probe a *user* address (from linux/uaccess.h):
> >
> > #define probe_kernel_address(addr, retval)              \
> >         ({                                              \
> >                 long ret;                               \
> >                                                         \
> >                 inc_preempt_count();                    \
> >                 ret = __get_user(retval, addr);         \
> >                 dec_preempt_count();                    \
> >                 ret;                                    \
> >         })
> >
> >   given that that routine is referenced only 5 places in the
> > entire source tree, wouldn't it be more meaningful to use a more
> > appropriate name?
>
> You'll notice that all callers are indeed probing kernel addresses.
> The function _could_ be used for user addresses and could perhaps be
> called probe_address().
>
> One of the reasons this wrapper exists is to communicate that the
> __get_user() it is in fact not being used to access user memory.

um ... ok.  i think.  i agree that "probe_address()" would be a more
appropriate name, but i'm still a bit confused as to why
"__get_user()" would be used to access something *not* in user memory,
given this seemingly unambiguous explanation in
include/asm-i386/uaccess.h:

=====
 * get_user: - Get a simple variable from user space.
 * @x:   Variable to store result.
 * @ptr: Source address, in user space.
 *
 * Context: User context only.  This function may sleep.
 *
 * This macro copies a single simple variable from user space to kernel
 * space.  It supports simple types like char and int, but not larger
 * data types like structures or arrays.
  ...
=====

  so having probe_kernel_address() invoke __get_user() does seem to be
just a wee bit confusing for us newbies.  in any event, i'll leave the
clarification for someone much higher up the food chain.

rday

Re: why "probe_kernel_address()", not "probe_user_address()"?

[Robert P. J. Day]

On Sat, 28 Oct 2006, Andrew Morton wrote:

> On Sat, 28 Oct 2006 11:56:24 -0400 (EDT)
> "Robert P. J. Day" <rpjday@mindspring.com> wrote:
>
> >
> >   it seems odd that the purpose of the "probe_kernel_address()" macro
> > is, in fact, to probe a *user* address (from linux/uaccess.h):
> >
> > #define probe_kernel_address(addr, retval)              \
> >         ({                                              \
> >                 long ret;                               \
> >                                                         \
> >                 inc_preempt_count();                    \
> >                 ret = __get_user(retval, addr);         \
> >                 dec_preempt_count();                    \
> >                 ret;                                    \
> >         })
> >
> >   given that that routine is referenced only 5 places in the entire
> > source tree, wouldn't it be more meaningful to use a more appropriate
> > name?
> >
>
> You'll notice that all callers are indeed probing kernel addresses.
> The function _could_ be used for user addresses and could perhaps be
> called probe_address().
>
> One of the reasons this wrapper exists is to communicate that the
> __get_user() it is in fact not being used to access user memory.

one quick addition to this.  in arch/i386/kernel/traps.c, we have the
code snippet:

        if (eip < PAGE_OFFSET)
                return;
        if (probe_kernel_address((unsigned short __user *)eip, ud2))
                return;
        if (ud2 != 0x0b0f)
                return;

        printk(KERN_EMERG "------------[ cut here ]------------\n");

#ifdef CONFIG_DEBUG_BUGVERBOSE
        do {
                unsigned short line;
                char *file;
                char c;

                if (probe_kernel_address((unsigned short __user *)(eip + 2),
                                        line))
... etc etc ...

  if those pointers qualified by "__user" *aren't* actually addresses
into user space, that would seem to violate what i read in the book
"linux device drivers (3rd ed)", p. 50:

"This [__user] annotation is a form of documentation, noting that a
pointer is a user-space address that cannot be directly dereferenced.
For normal compilation, __user has no effect, but it can be used by
external checking software to find misuse of user-space addresses."

  under the circumstances, wouldn't this show up as one of those
misuses?

rday

Re: why "probe_kernel_address()", not "probe_user_address()"?

[Luca Tettamanti]

Robert P. J. Day <rpjday@mindspring.com> ha scritto:
> On Sat, 28 Oct 2006, Andrew Morton wrote:
> 
>> On Sat, 28 Oct 2006 11:56:24 -0400 (EDT)
>> "Robert P. J. Day" <rpjday@mindspring.com> wrote:
>>
>> >
>> >   it seems odd that the purpose of the "probe_kernel_address()" macro
>> > is, in fact, to probe a *user* address (from linux/uaccess.h):
>> >
>> > #define probe_kernel_address(addr, retval)              \
>> >         ({                                              \
>> >                 long ret;                               \
>> >                                                         \
>> >                 inc_preempt_count();                    \
>> >                 ret = __get_user(retval, addr);         \
>> >                 dec_preempt_count();                    \
>> >                 ret;                                    \
>> >         })
>> >
>> >   given that that routine is referenced only 5 places in the entire
>> > source tree, wouldn't it be more meaningful to use a more appropriate
>> > name?
>> >
>>
>> You'll notice that all callers are indeed probing kernel addresses.
>> The function _could_ be used for user addresses and could perhaps be
>> called probe_address().
>>
>> One of the reasons this wrapper exists is to communicate that the
>> __get_user() it is in fact not being used to access user memory.
> 
> one quick addition to this.  in arch/i386/kernel/traps.c, we have the
> code snippet:
> 
>        if (eip < PAGE_OFFSET)
>                return;
>        if (probe_kernel_address((unsigned short __user *)eip, ud2))
>                return;
>        if (ud2 != 0x0b0f)
>                return;
> 
>        printk(KERN_EMERG "------------[ cut here ]------------\n");
> 
> #ifdef CONFIG_DEBUG_BUGVERBOSE
>        do {
>                unsigned short line;
>                char *file;
>                char c;
> 
>                if (probe_kernel_address((unsigned short __user *)(eip + 2),
>                                        line))
> ... etc etc ...
> 

I agree that it may be confusing. The whole point in using __get_user()
is that it ensures that the source address is valid.

When handle_BUG() is called the kernel is no more in a "sane" state,
blindly using the content of the registers may lead to a page fault in
kernel mode. So the code extract filename and line of the BUG checking
that the line number is indeed readable, the address of the string (file
name) is readable and it points to a readable memory location.

probe_kernel_address wrapper is used to "hide" the fact that we are
re-using the infrastructure provided by a function with a confusing name
;) The cast to __user is needed to keep sparse quiet.

>  if those pointers qualified by "__user" *aren't* actually addresses
> into user space, that would seem to violate what i read in the book
> "linux device drivers (3rd ed)", p. 50:
> 
> "This [__user] annotation is a form of documentation, noting that a
> pointer is a user-space address that cannot be directly dereferenced.
> For normal compilation, __user has no effect, but it can be used by
> external checking software to find misuse of user-space addresses."
> 
>  under the circumstances, wouldn't this show up as one of those
> misuses?

No, __user annotation ensure that sparse will notice things like:

int random_pointer_from_userspace __user *data;
...
my_data = *data; /* <-- sparse will complain */

In this scenario 'data' is a pointer provided by userspace application
and *cannot* be trusted; it shall be used only with functions that do
check the address before using it, like __get_user(). (get_user() also
checks that the address is a *userspace* address, it can't be used to
read kernel memory).

Luca
-- 
Se alla sera, dopo una strepitosa vittoria, guardandoti allo
specchio dovessi notare un secondo paio di palle, che il tuo 
cuore non si riempia d'orgoglio, perche` vuol dire che ti 
stanno inculando -- Saggio Cinese

Re: why "probe_kernel_address()", not "probe_user_address()"?

[Robert P. J. Day]

On Sat, 28 Oct 2006, Luca Tettamanti wrote:

> I agree that it may be confusing. The whole point in using
> __get_user() is that it ensures that the source address is valid.
>
> When handle_BUG() is called the kernel is no more in a "sane" state,
> blindly using the content of the registers may lead to a page fault
> in kernel mode. So the code extract filename and line of the BUG
> checking that the line number is indeed readable, the address of the
> string (file name) is readable and it points to a readable memory
> location.
>
> probe_kernel_address wrapper is used to "hide" the fact that we are
> re-using the infrastructure provided by a function with a confusing
> name ;) The cast to __user is needed to keep sparse quiet.

... more response snipped ...

ok, i'm going to defer to people who are clearly much smarter about
this than me, and i'll go back and read that entire response more
carefully until i understand it.  thanks for explaining it.

rday

Re: why "probe_kernel_address()", not "probe_user_address()"?

[Denis Vlasenko]

On Saturday 28 October 2006 22:30, Luca Tettamanti wrote:
> probe_kernel_address wrapper is used to "hide" the fact that we are
> re-using the infrastructure provided by a function with a confusing name
> ;) The cast to __user is needed to keep sparse quiet.

Shouldn't the above text be placed in a comment above probe_kernel_address()
definition? ;)
--
vda

Re: why "probe_kernel_address()", not "probe_user_address()"?

[Luca Tettamanti]

Il Sat, Oct 28, 2006 at 05:01:31PM -0400, Robert P. J. Day ha scritto: 
> ok, i'm going to defer to people who are clearly much smarter about
> this than me, and i'll go back and read that entire response more
> carefully until i understand it. 

Well, if you tell me what is not clear I can try to re-explain it.

> thanks for explaining it.

No problem, the best way to learn about the kernel is reading the source
and asking questions ;)

Luca
-- 
Mi piace avere amici rispettabili;
Mi piace essere il peggiore della compagnia.