On 4/14/20 2:27 PM, Mark Rutland wrote:
> On Tue, Apr 14, 2020 at 01:50:38PM +0100, Vincenzo Frascino wrote:
>> Hi Mark,
>>
>> On 4/14/20 11:42 AM, Mark Rutland wrote:
>>> The aarch32_vdso_pages[] array never has entries allocated in the C_VVAR
>>> or C_VDSO slots, and as the array is zero initialized these contain
>>> NULL.
>>>
>>> However in __aarch32_alloc_vdso_pages() when
>>> aarch32_alloc_kuser_vdso_page() fails we attempt to free the page whose
>>> struct page is at NULL, which is obviously nonsensical.
>>
>> Could you please explain why do you think that free(NULL) is "nonsensical"?
>
> Regardless of the below, can you please explain why it is sensical? I'm
> struggling to follow your argument here.
free(NULL) is a no-operation ("no action occurs") according to the C standard
(ISO-IEC 9899 paragraph 7.20.3.2). Hence this should not cause any bug if the
allocator is correctly implemented. From what I can see the implementation of
the page allocator honors this assumption.
Since you say it is a bug (providing evidence), we might have to investigate
because probably there is an issue somewhere else.
>
> * It serves no legitimate purpose. One cannot free a page without a
> corresponding struct page.
>
> * It is redundant. Removing the code does not detract from the utility
> of the remainging code, or make that remaing code more complex.
>
> * It hinders comprehension of the code. When a developer sees the
> free_page() they will assume that the page was allocated somewhere,
> but there is no corresponding allocation as the pointers are never
> assigned to. Even if the code in question is not harmful to the
> functional correctness of the code, it is an unnecessary burden to
> developers.
>
> * page_to_virt(NULL) does not have a well-defined result, and
> page_to_virt() should only be called for a valid struct page pointer.
> The result of page_to_virt(NULL) may not be a pointer into the linear
> map as would be expected.
>
Do you know why this is the case? To be compliant with what the page allocator
expects page_to_virt(NULL) should be equal to NULL.
> * free_page(x) calls free_pages(x, 0), which checks virt_addr_valid(x).
> As page_to_virt(NULL) is not a valid linear map address, this can
> trigger a VM_BUG_ON()
>
free_pages(x, 0) checks virt_addr_valid(x) only if "addr != 0" (as per C
standard) which makes me infer what I stated above. But maybe I am missing
something.
[...]
--
Regards,
Vincenzo
