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

[Alexander Duyck]

On Tue, Jul 19, 2022 at 3:27 PM Chen Lin <chen45464546@xxxxxxx> wrote:
>
> At 2022-07-18 23:33:42, "Alexander Duyck" <alexander.duyck@xxxxxxxxx> wrote:
> >On Mon, Jul 18, 2022 at 8:25 AM Maurizio Lombardi <mlombard@xxxxxxxxxx> wrote:
> >>
> >> po 18. 7. 2022 v 16:40 odesílatel Chen Lin <chen45464546@xxxxxxx> 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.