On Tue, Apr 14, 2020 at 03:53:45PM +0100, Vincenzo Frascino wrote:
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.
This is true, but irrelevant. The C standard does not define free_page(), which is a Linnux kernel internal function, and does not have the same semantics as free().
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.
- 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.
Regardless, this is all academic unless you disagree with the first two bullets above.
You don't randomly sprinkle a program with free(NULL) for the fun of it. Similarly, and regardless of how obfuscated, one should not do the same here.
Thanks, Mark.