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

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

[Maurizio Lombardi]

A number of drivers call page_frag_alloc() with a
fragment's size > PAGE_SIZE.
In low memory conditions, __page_frag_cache_refill() may fail the order 3
cache allocation and fall back to order 0;
In this case, the cache will be smaller than the fragment, causing
memory corruptions.

Prevent this from happening by checking if the newly allocated cache
is large enough for the fragment; if not, the allocation will fail
and page_frag_alloc() will return NULL.

V2: do not free the cache page because this could make memory pressure
even worse, just return NULL.

V3: add a comment to explain why we return NULL.

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 mm/page_alloc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index e008a3df0485..59c4dddf379f 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5617,6 +5617,18 @@ void *page_frag_alloc_align(struct page_frag_cache *nc,
 		/* reset page count bias and offset to start of new frag */
 		nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
 		offset = size - fragsz;
+		if (unlikely(offset < 0)) {
+			/*
+			 * The caller is trying to allocate a fragment
+			 * with fragsz > PAGE_SIZE but the cache isn't big
+			 * enough to satisfy the request, this may
+			 * happen in low memory conditions.
+			 * We don't release the cache page because
+			 * it could make memory pressure worse
+			 * so we simply return NULL here.
+			 */
+			return NULL;
+		}
 	}
 
 	nc->pagecnt_bias--;
-- 
2.31.1

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

[Chen Lin]

At 2022-07-15 20:50:13, "Maurizio Lombardi" <mlombard@redhat.com> wrote:
>A number of drivers call page_frag_alloc() with a
>fragment's size > PAGE_SIZE.
>In low memory conditions, __page_frag_cache_refill() may fail the order 3
>cache allocation and fall back to order 0;
>In this case, the cache will be smaller than the fragment, causing
>memory corruptions.
>
>Prevent this from happening by checking if the newly allocated cache
>is large enough for the fragment; if not, the allocation will fail
>and page_frag_alloc() will return NULL.
>
>V2: do not free the cache page because this could make memory pressure
>even worse, just return NULL.
>
>V3: add a comment to explain why we return NULL.
>
>Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
>---
> mm/page_alloc.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
>diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>index e008a3df0485..59c4dddf379f 100644
>--- a/mm/page_alloc.c
>+++ b/mm/page_alloc.c
>@@ -5617,6 +5617,18 @@ void *page_frag_alloc_align(struct page_frag_cache *nc,
> 		/* reset page count bias and offset to start of new frag */
> 		nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
> 		offset = size - fragsz;
>+		if (unlikely(offset < 0)) {
>+			/*
>+			 * The caller is trying to allocate a fragment
>+			 * with fragsz > PAGE_SIZE but the cache isn't big
>+			 * enough to satisfy the request, this may
>+			 * happen in low memory conditions.
>+			 * We don't release the cache page because
>+			 * it could make memory pressure worse
>+			 * so we simply return NULL here.
>+			 */
>+			return NULL;
>+		}
> 	}
> 
> 	nc->pagecnt_bias--;
>-- 
>2.31.1

Will  this lead to memory leak when device driver miss use this interface muti-times？

----------------------------------------
If we can accept adding a branch to this process, why not add it at the beginning like below?
The below changes are also more in line with the definition of "page fragment", 
which i mean the above changes may make the allocation of more than one page successful.

index 7a28f7d..9d09ea5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5551,6 +5551,8 @@ void *page_frag_alloc_align(struct page_frag_cache *nc,

        offset = nc->offset - fragsz;
        if (unlikely(offset < 0)) {
+               if (unlikely(fragsz > PAGE_SIZE))
+                       return NULL;
                page = virt_to_page(nc->va);

                if (!page_ref_sub_and_test(page, nc->pagecnt_bias))

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

[Maurizio Lombardi]

po 18. 7. 2022 v 15:14 odesílatel Chen Lin <chen45464546@163.com> napsal:
> ----------------------------------------
> If we can accept adding a branch to this process, why not add it at the beginning like below?
> The below changes are also more in line with the definition of "page fragment",
> which i mean the above changes may make the allocation of more than one page successful.
>
> index 7a28f7d..9d09ea5 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5551,6 +5551,8 @@ void *page_frag_alloc_align(struct page_frag_cache *nc,
>
>         offset = nc->offset - fragsz;
>         if (unlikely(offset < 0)) {
> +               if (unlikely(fragsz > PAGE_SIZE))
> +                       return NULL;
>                 page = virt_to_page(nc->va);
>
>                 if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
>

This will make *all* page_frag_alloc() calls with fragsz > PAGE_SIZE
fail, so it will
basically break all those drivers that are relying on the current behaviour.
With my patch it will return NULL only if the cache isn't big enough.

Maurizio.

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

[Chen Lin]

At 2022-07-18 21:50:41, "Maurizio Lombardi" <mlombard@redhat.com> wrote:
>po 18. 7. 2022 v 15:14 odesílatel Chen Lin <chen45464546@163.com> napsal:
>> ----------------------------------------
>> If we can accept adding a branch to this process, why not add it at the beginning like below?
>> The below changes are also more in line with the definition of "page fragment",
>> which i mean the above changes may make the allocation of more than one page successful.
>>
>> index 7a28f7d..9d09ea5 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -5551,6 +5551,8 @@ void *page_frag_alloc_align(struct page_frag_cache *nc,
>>
>>         offset = nc->offset - fragsz;
>>         if (unlikely(offset < 0)) {
>> +               if (unlikely(fragsz > PAGE_SIZE))
>> +                       return NULL;
>>                 page = virt_to_page(nc->va);
>>
>>                 if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
>>
>
>This will make *all* page_frag_alloc() calls with fragsz > PAGE_SIZE
>fail, so it will
>basically break all those drivers that are relying on the current behaviour.
>With my patch it will return NULL only if the cache isn't big enough.
>
>Maurizio.

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".

Furthermore, the patch above is easier to synchronize to lower versions.

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

[Maurizio Lombardi]

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

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

[Alexander Duyck]

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

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

[Chen Lin]

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)) {
                 -------------->Ａ<------------
		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<------------
	}

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@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.

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

[Chen Lin]

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.

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

[Maurizio Lombardi]

po 18. 7. 2022 v 13:17 odesílatel Chen Lin <chen45464546@163.com> napsal:
>
> Will  this lead to memory leak when device driver miss use this interface muti-times？

No, I tested it and pagecnt_bias and the page refcnt are always consistent.

Maurizio

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

[Andrew Morton]

On Fri, 15 Jul 2022 14:50:13 +0200 Maurizio Lombardi <mlombard@redhat.com> wrote:

> A number of drivers call page_frag_alloc() with a
> fragment's size > PAGE_SIZE.
> In low memory conditions, __page_frag_cache_refill() may fail the order 3
> cache allocation and fall back to order 0;
> In this case, the cache will be smaller than the fragment, causing
> memory corruptions.
> 
> Prevent this from happening by checking if the newly allocated cache
> is large enough for the fragment; if not, the allocation will fail
> and page_frag_alloc() will return NULL.

Can we come up with a Fixes: for this?

Should this fix be backported into -stable kernels?

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

[Maurizio Lombardi]

út 9. 8. 2022 v 2:14 odesílatel Andrew Morton
<akpm@linux-foundation.org> napsal:
>
> On Fri, 15 Jul 2022 14:50:13 +0200 Maurizio Lombardi <mlombard@redhat.com> wrote:
>
> > A number of drivers call page_frag_alloc() with a
> > fragment's size > PAGE_SIZE.
> > In low memory conditions, __page_frag_cache_refill() may fail the order 3
> > cache allocation and fall back to order 0;
> > In this case, the cache will be smaller than the fragment, causing
> > memory corruptions.
> >
> > Prevent this from happening by checking if the newly allocated cache
> > is large enough for the fragment; if not, the allocation will fail
> > and page_frag_alloc() will return NULL.
>
> Can we come up with a Fixes: for this?

I think the bug has been introduced in kernel 3.19-rc1
Fixes: ffde7328a36d16e626bae8468571858d71cd010b

>
> Should this fix be backported into -stable kernels?

Yes, IMO this should be backported to -stable

Thanks,
Maurizio

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

[Alexander Duyck]

On Tue, Aug 9, 2022 at 4:45 AM Maurizio Lombardi <mlombard@redhat.com> wrote:
>
> út 9. 8. 2022 v 2:14 odesílatel Andrew Morton
> <akpm@linux-foundation.org> napsal:
> >
> > On Fri, 15 Jul 2022 14:50:13 +0200 Maurizio Lombardi <mlombard@redhat.com> wrote:
> >
> > > A number of drivers call page_frag_alloc() with a
> > > fragment's size > PAGE_SIZE.
> > > In low memory conditions, __page_frag_cache_refill() may fail the order 3
> > > cache allocation and fall back to order 0;
> > > In this case, the cache will be smaller than the fragment, causing
> > > memory corruptions.
> > >
> > > Prevent this from happening by checking if the newly allocated cache
> > > is large enough for the fragment; if not, the allocation will fail
> > > and page_frag_alloc() will return NULL.
> >
> > Can we come up with a Fixes: for this?
>
> I think the bug has been introduced in kernel 3.19-rc1
> Fixes: ffde7328a36d16e626bae8468571858d71cd010b

The problem is this patch won't cleanly apply to that since we moved
the function. In addition this issue is a bit more complex since it
isn't necessarily a problem in the code, but the assumption on how it
is can be used by a select few drivers that were using it to allocate
to higher order pages.

It would probably be best to just go with:
Fixes: b63ae8ca096d ("mm/net: Rename and move page fragment handling
from net/ to mm/")

> >
> > Should this fix be backported into -stable kernels?
>
> Yes, IMO this should be backported to -stable

This should be fine for -stable. Basically it just needs to be there
to block the drivers that abused the API to allocate high order pages
instead of fragments of an order 0 page. Ultimately the correct fix
for this is to fix those drivers, but this at least is enough so that
they will fail allocations now instead of corrupting memory by
overflowing an order 0 page.