Re: [Qemu-devel] [PATCH v1 for 3.0 1/2] linux-user/mmap.c: handle len = 0 maps correctly

[Laurent Vivier]

Le 26/07/2018 à 19:58, Alex Bennée a écrit :
> 
> Laurent Vivier <address@hidden> writes:
> 
>> Le 26/07/2018 à 15:29, Alex Bennée a écrit:
>>> I've slightly re-organised the check to more closely match the
>>> sequence that the kernel uses in do_mmap().
>>>
>>> Signed-off-by: Alex Bennée <address@hidden>
>>> Cc: umarcor <address@hidden>
>>> ---
>>>  linux-user/mmap.c | 14 +++++++++++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
>>> index d0c50e4888..3ef69fa2d0 100644
>>> --- a/linux-user/mmap.c
>>> +++ b/linux-user/mmap.c
>>> @@ -391,14 +391,22 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, 
>>> int prot,
>>>      }
>>>  #endif
>>>
>>> -    if (offset & ~TARGET_PAGE_MASK) {
>>> +    if (!len) {
>>>          errno = EINVAL;
>>>          goto fail;
>>>      }
>>>
>>>      len = TARGET_PAGE_ALIGN(len);
>>> -    if (len == 0)
>>> -        goto the_end;
>>> +    if (!len) {
>>> +        errno = EINVAL;
>>> +        goto fail;
>>> +    }
>>
>> Why do you check twice len?
>> TARGET_PAGE_ALIGN() rounds up the value, so if it was not 0, it cannot
>> be now.
> 
> I was trying to follow the kernel style but I realise TARGET_PAGE_ALIGN
> might be a different test compared to the kernel's PAGE_ALIGN(len) for
> overflows:
...
>       /* Careful about overflows.. */
>       len = PAGE_ALIGN(len);
>       if (!len)
>               return -ENOMEM;
> 


OK, so keep it but you should use ENOMEM, not EINVAL (and add a comment :) )

Thanks,
Laurent