Messages in this thread | | | Date | Sat, 26 Feb 2011 13:37:31 +0100 | From | Oleg Nesterov <> | Subject | Re: [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/
| |