Re: [PATCH 07/12] x86/compat: get_gate_vma should check for both 32-bit compat and x32

[Brian Gerst]

On Mon, Jun 22, 2015 at 12:19 PM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> On Mon, Jun 22, 2015 at 4:55 AM, Brian Gerst <brgerst@xxxxxxxxx> wrote:
>> Change this to CONFIG_COMPAT so both 32-bit compat and x32 will do the
>> check.
>>
>> Signed-off-by: Brian Gerst <brgerst@xxxxxxxxx>
>> ---
>>  arch/x86/entry/vsyscall/vsyscall_64.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c
>> index 2dcc6ff..26a46f4 100644
>> --- a/arch/x86/entry/vsyscall/vsyscall_64.c
>> +++ b/arch/x86/entry/vsyscall/vsyscall_64.c
>> @@ -290,7 +290,7 @@ static struct vm_area_struct gate_vma = {
>>
>>  struct vm_area_struct *get_gate_vma(struct mm_struct *mm)
>>  {
>> -#ifdef CONFIG_IA32_EMULATION
>> +#ifdef CONFIG_COMPAT
>>         if (!mm || mm->context.ia32_compat)
>>                 return NULL;
>>  #endif
>
>
> This makes little sense to me.
>
> First, why is the !mm check guarded by any ifdef at all?  If this said
> "if (mm && mm->...)", it would make no sense.
>
> Second, and more importantly, what does mm->context.ia32_compat mean
> in the new less-nonsensical regime?  The flag itself is defined in a
> way that makes no sense (it's either 0, TIF_X32, or TIF_IA32 --
> presumably it should be an enum).  There aren't a whole lot of things
> that care -- it's just this check and some uprobe thing.  At some
> point, mpx will start caring, too.  There's also TIF_ADDR32, which is
> similarly ridiculous (why isn't it part of mm->context?, and why does
> it exist at all).
>
> I think that the questions we want to be able to answer are:
>
> 1. Is this mm intended to be addressable using 32 bits?  If so, we
> should probably not show the vsyscall area in /proc/self/maps.
>
> 2. Is this mm's mpx context intended to be used by 32-bit userspace?
> (That's real 32 bit, not x32 -- x32 is certainly 64-bit as far as MPX
> is concerned.)
>
> 3. Is the current mmap call intended to return something in the low 32
> bits?  Presumably that should depend only on the mmap call's bitness
> (is_compat_task, etc, which we really need to rename to something like
> in_compat_syscall).
>
> I find myself wondering whether there is any legitimate reason that
> TASK_SIZE isn't the same thing as TASK_SIZE_MAX...
>
> Anyway, your patch is probably fine -- you're not actually making
> anything worse.
>
> --Andy

I agree there are more cleanups needed here.  But that's probably a
whole patch series in itself.  This patch is intended to be no code
change when X32 doesn't depend on 32-bit compat anymore.

--
Brian Gerst
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/