Re:Re: Re: Re: [PATCH V3] mm: prevent page_frag_alloc() from corrupting the memory

["Chen Lin" <chen45464546@163.com>]

At 2022-07-20 22:54:18, "Alexander Duyck" <alexander.duyck@gmail.com> wrote:
>On Tue, Jul 19, 2022 at 3:27 PM Chen Lin <chen45464546@163.com> wrote:
>>
>> At 2022-07-18 23:33:42, "Alexander Duyck" <alexander.duyck@gmail.com> wrote:
>> >On Mon, Jul 18, 2022 at 8:25 AM Maurizio Lombardi <mlombard@redhat.com> wrote:
>> >>
>> >> po 18. 7. 2022 v 16:40 odesílatel Chen Lin <chen45464546@163.com> napsal:
>> >> >
>> >> > But the original intention of page frag interface is indeed to allocate memory
>> >> > less than one page. It's not a good idea to  complicate the definition of
>> >> > "page fragment".
>> >>
>> >> I see your point, I just don't think it makes much sense to break
>> >> drivers here and there
>> >> when a practically identical 2-lines patch can fix the memory corruption bug
>> >> without changing a single line of code in the drivers.
>> >>
>> >> By the way, I will wait for the maintainers to decide on the matter.
>> >>
>> >> Maurizio
>> >
>> >I'm good with this smaller approach. If it fails only under memory
>> >pressure I am good with that. The issue with the stricter checking is
>> >that it will add additional overhead that doesn't add much value to
>> >the code.
>> >
>> >Thanks,
>> >
>>
>> >- Alex
>>
>> One additional question：
>> I don't understand too much about  why point >Ａ<  have more overhead than point >B<.
>> It all looks the same, except for jumping to the refill process, and the refill is a very long process.
>> Could you please give more explain？
>>
>>         if (unlikely(offset < 0)) {
>>                  -------------->Ａ<------------
>
>In order to verify if the fragsz is larger than a page we would have
>to add a comparison between two values that aren't necessarily related
>to anything else in this block of code.
>
>Adding a comparison at this point should end up adding instructions
>along the lines of
>        cmp $0x1000,%[some register]
>        jg <return NULL block>
>
>These two lines would end up applying to everything that takes this
>path so every time we hit the end of a page we would encounter it, and
>in almost all cases it shouldn't apply so it is extra instructions. In
>addition they would be serialized with the earlier subtraction since
>we can't process it until after the first comparison which is actually
>using the flags to perform the check since it is checking if offset is
>signed.
>
>>                 page = virt_to_page(nc->va);
>>
>>                 if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
>>                         goto refill;
>>
>>                 if (unlikely(nc->pfmemalloc)) {
>>                         free_the_page(page, compound_order(page));
>>                         goto refill;
>>                 }
>>
>> #if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
>>                 /* if size can vary use size else just use PAGE_SIZE */
>>                 size = nc->size;
>> #endif
>>                 /* OK, page count is 0, we can safely set it */
>>                 set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1);
>>
>>                 /* reset page count bias and offset to start of new frag */
>>                 nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
>>                 offset = size - fragsz;
>>                  -------------->B<------------
>
>At this point we have already excluded the shared and pfmemalloc
>pages. Here we don't have to add a comparison. The comparison is
>already handled via the size - fragsz, we just need to make use of the
>flags following the operation by checking to see if offset is signed.
>
>So the added assembler would be something to the effect of:
>        js <return NULL block>
>
>In addition the assignment operations there should have no effect on
>the flags so the js can be added to the end of the block without
>having to do much in terms of forcing any reordering of the
>instructions.

Thank you for your detailed explanation, I get it.
I objdump the vmlinux on different modified versions and verified that 
the generated instructions are the same as what you said. 

Under the strict performance requirements, there seems no better way
than the patch Maurizio proposed.