lkml.org 
[lkml]   [2011]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 3/5] exec: unify compat_do_execve() code
    On Sat, Feb 26, 2011 at 4:37 AM, Oleg Nesterov <oleg@redhat.com> wrote:
    >>
    >>   typedef union {
    >>      compat_uptr_t compat;
    >>      const char __user *native;
    >>    } conditional_user_ptr_t;
    >
    > Personally I don't really like this union, to me "void __user*" looks
    > better, but I won't insist.

    Umm. "void __user *" may look simpler/better, but it's WRONG.

    Using "const char __user *const __user *" is correct - but only for
    the non-compat case.

    And using "void __user *" may result in compiling code, but it will
    have lost all actual information about the type. We don't do that in
    the kernel if we can avoid it, because "void *" basically does no type
    checking. Sure, sometimes it's the only thing we can do, but _if_ we
    have a type, we should use it.

    And that "union" really is the true type. You are passing a user
    pointer down that can be either of those members.

    So if you think it looks ugly, then you shouldn't do that "conditional
    compat argument at run-time at all". Because the real ugliness of the
    type comes not from the type, but from the fact that you pass a
    pointer that can contain two different things.


    > Once again, to me "void __user*" looks better (just simpler). In this
    > case get_arg_ptr() becomes (without const/__user for the clarity)

    No.

    I simply won't apply that. It's WRONG. It's wrong because you've
    dropped all the type information.

    With the right union,

    >        void *get_arg_ptr(void **argv, int argc, bool compat)
    >        {
    >                char *ptr;
    >
    >        #ifdef CONFIG_COMPAT
    >                if (unlikely(compat)) {
    >                        compat_uptr_t *a = argv;
    >                        compat_uptr_t p;
    >
    >                        if (get_user(p, a + argc))
    >                                return ERR_PTR(-EFAULT);
    >
    >                        return compat_ptr(p);
    >                }
    >        #endif
    >
    >                if (get_user(ptr, &argv. + argc))
    >                        return ERR_PTR(-EFAULT);
    >
    >                return ptr;
    >        }
    >
    > Otherwise, get_arg_ptr() should return conditional_user_ptr_t as well,

    No it shouldn't. The get_arg_ptr() should always just return the
    actual pointer. It will have _resolved_ the ambiguity! That's what the
    "compat_ptr()" thing does in the return case inside teh CONFIG_COMPAT.

    So the correct way to do this is something like the following (yeah,
    maybe I got the syntax wrong, I didn't test this, I just wrote it in
    my MUA):

    void *get_arg_ptr(const union compat_ptr_union __user *argv,
    int argc, bool compat)
    {
    char *ptr;

    #ifdef CONFIG_COMPAT
    if (unlikely(compat)) {
    compat_uptr_t p;

    if (get_user(p, &argv->compat + argc))
    return ERR_PTR(-EFAULT);

    return compat_ptr(p);
    }
    #endif

    if (get_user(ptr, &argv->noncompat +argc))
    return ERR_PTR(-EFAULT);

    return ptr;
    }

    and notice how it gets the types right, and it even has one line LESS
    than your version, exactly because it gets the types right and doesn't
    need that implied cast in your

    compat_uptr_t *a = argv;

    (in fact, I think your version needs an _explicit_ cast in order to
    not get a warning: you can't just cast "void **" to something else).

    See? The advantage of the union is that the types are correct, which
    means that the casts are unnecessary.

    The advantage of the union is also that you see what is going on, and
    it's clear from the function prototype that this doesn't just take a
    random user pointer, it takes a user pointer to something that can be
    two different types.

    See? Correct typing is important.

    Linus
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-02-26 16:59    [W:7.530 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site