lkml.org 
[lkml]   [2011]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/5] exec: unify compat_do_execve() code
On 02/25, Linus Torvalds wrote:
>
> On Fri, Feb 25, 2011 at 9:53 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > Teach get_arg_ptr() to handle compat = T case correctly.
>
> Does it?

I think it does.

> > +#ifdef CONFIG_COMPAT
> > +int compat_do_execve(char *filename,
> > +       compat_uptr_t __user *argv,
> > +       compat_uptr_t __user *envp,
> > +       struct pt_regs *regs)
> > +{
> > +       return do_execve_common(filename,
> > +                               (void __user *)argv, (void __user*)envp,
> > +                               regs, true);
> > +}
> > +#endif
>
> I really suspect this should be something like
>
> 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.

> int compat_do_execve(char *filename,
> compat_uptr_t argv,
> compat_uptr_t envp,
> {
> return do_execve_common(filename,
> compat_ptr(argv), compat_ptr(envp), regs);

Indeed! But, again, this has nothing to do with this series. We can
do this later and change the callers in arch/.

> where that 'do_execve_common()' takes it's arguments as
>
> union conditional_user_ptr_t __user *argv,
> union conditional_user_ptr_t __user *envp
>
> and then in get_arg_ptr() we do the proper union member dereference
> depending on the "compat" flag.

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

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,
this looks like the unnecessary complication to me, but of course this
is subjective.

So, what do you think?

Oleg.

--
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 13:49    [W:0.124 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site