Re: [PATCH 3/5] exec: unify compat_do_execve() code

[Oleg Nesterov <oleg@redhat.com>]

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.