We are above to use page_frag_alloc_*() API to not just
allocate memory for skb->data, but also use them to do
the memory allocation for skb frag too. Currently the
implementation of page_frag in mm subsystem is running
the offset as a countdown rather than count-up value,
there may have several advantages to that as mentioned
in [1], but it may have some disadvantages, for example,
it may disable skb frag coaleasing and more correct cache
prefetching
We have a trade-off to make in order to have a unified
implementation and API for page_frag, so use a initial zero
offset in this patch, and the following patch will try to
make some optimization to aovid the disadvantages as much
as possible.
As offsets is added due to alignment requirement before
actually checking if the cache is enough, which might make
it exploitable if caller passes a align value bigger than
32K mistakenly. As we are allowing order 3 page allocation
to fail easily under low memory condition, align value bigger
than PAGE_SIZE is not really allowed, so add a 'align >
PAGE_SIZE' checking in page_frag_alloc_va_align() to catch
that.
1. https://lore.kernel.org/all/f4abe71b3439b39d17a6fb2d410180f367cadf5c.camel@gmail.com/
CC: Alexander Duyck <alexander.duyck@gmail.com>
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
include/linux/page_frag_cache.h | 2 +-
include/linux/skbuff.h | 4 ++--
mm/page_frag_cache.c | 26 +++++++++++---------------
3 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h
index 3a44bfc99750..b9411f0db25a 100644
--- a/include/linux/page_frag_cache.h
+++ b/include/linux/page_frag_cache.h
@@ -32,7 +32,7 @@ static inline void *page_frag_alloc_align(struct page_frag_cache *nc,
unsigned int fragsz, gfp_t gfp_mask,
unsigned int align)
{
- WARN_ON_ONCE(!is_power_of_2(align));
+ WARN_ON_ONCE(!is_power_of_2(align) || align > PAGE_SIZE);
return __page_frag_alloc_align(nc, fragsz, gfp_mask, -align);
}
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index eb8ae8292c48..d1fea23ec386 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3320,7 +3320,7 @@ static inline void *netdev_alloc_frag(unsigned int fragsz)
static inline void *netdev_alloc_frag_align(unsigned int fragsz,
unsigned int align)
{
- WARN_ON_ONCE(!is_power_of_2(align));
+ WARN_ON_ONCE(!is_power_of_2(align) || align > PAGE_SIZE);
return __netdev_alloc_frag_align(fragsz, -align);
}
@@ -3391,7 +3391,7 @@ static inline void *napi_alloc_frag(unsigned int fragsz)
static inline void *napi_alloc_frag_align(unsigned int fragsz,
unsigned int align)
{
- WARN_ON_ONCE(!is_power_of_2(align));
+ WARN_ON_ONCE(!is_power_of_2(align) || align > PAGE_SIZE);
return __napi_alloc_frag_align(fragsz, -align);
}
diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
index 88f567ef0e29..da244851b8a4 100644
--- a/mm/page_frag_cache.c
+++ b/mm/page_frag_cache.c
@@ -72,10 +72,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
if (!page)
return NULL;
-#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
- /* if size can vary use size else just use PAGE_SIZE */
- size = nc->size;
-#endif
/* Even if we own the page, we do not use atomic_set().
* This would break get_page_unless_zero() users.
*/
@@ -84,11 +80,16 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
/* reset page count bias and offset to start of new frag */
nc->pfmemalloc = page_is_pfmemalloc(page);
nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
- nc->offset = size;
+ nc->offset = 0;
}
- offset = nc->offset - fragsz;
- if (unlikely(offset < 0)) {
+#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
+ /* if size can vary use size else just use PAGE_SIZE */
+ size = nc->size;
+#endif
+
+ offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
+ if (unlikely(offset + fragsz > size)) {
page = virt_to_page(nc->va);
if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
@@ -99,17 +100,13 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
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;
- if (unlikely(offset < 0)) {
+ offset = 0;
+ if (unlikely(fragsz > PAGE_SIZE)) {
/*
* The caller is trying to allocate a fragment
* with fragsz > PAGE_SIZE but the cache isn't big
@@ -124,8 +121,7 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
}
nc->pagecnt_bias--;
- offset &= align_mask;
- nc->offset = offset;
+ nc->offset = offset + fragsz;
return nc->va + offset;
}
--
2.33.0On Tue, 2024-06-25 at 21:52 +0800, Yunsheng Lin wrote:
> We are above to use page_frag_alloc_*() API to not just
"about to use", not "above to use"
> allocate memory for skb->data, but also use them to do
> the memory allocation for skb frag too. Currently the
> implementation of page_frag in mm subsystem is running
> the offset as a countdown rather than count-up value,
> there may have several advantages to that as mentioned
> in [1], but it may have some disadvantages, for example,
> it may disable skb frag coaleasing and more correct cache
> prefetching
>
> We have a trade-off to make in order to have a unified
> implementation and API for page_frag, so use a initial zero
> offset in this patch, and the following patch will try to
> make some optimization to aovid the disadvantages as much
> as possible.
>
> As offsets is added due to alignment requirement before
> actually checking if the cache is enough, which might make
> it exploitable if caller passes a align value bigger than
> 32K mistakenly. As we are allowing order 3 page allocation
> to fail easily under low memory condition, align value bigger
> than PAGE_SIZE is not really allowed, so add a 'align >
> PAGE_SIZE' checking in page_frag_alloc_va_align() to catch
> that.
>
> 1. https://lore.kernel.org/all/f4abe71b3439b39d17a6fb2d410180f367cadf5c.camel@gmail.com/
>
> CC: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> ---
> include/linux/page_frag_cache.h | 2 +-
> include/linux/skbuff.h | 4 ++--
> mm/page_frag_cache.c | 26 +++++++++++---------------
> 3 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/page_frag_cache.h b/include/linux/page_frag_cache.h
> index 3a44bfc99750..b9411f0db25a 100644
> --- a/include/linux/page_frag_cache.h
> +++ b/include/linux/page_frag_cache.h
> @@ -32,7 +32,7 @@ static inline void *page_frag_alloc_align(struct page_frag_cache *nc,
> unsigned int fragsz, gfp_t gfp_mask,
> unsigned int align)
> {
> - WARN_ON_ONCE(!is_power_of_2(align));
> + WARN_ON_ONCE(!is_power_of_2(align) || align > PAGE_SIZE);
> return __page_frag_alloc_align(nc, fragsz, gfp_mask, -align);
> }
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index eb8ae8292c48..d1fea23ec386 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3320,7 +3320,7 @@ static inline void *netdev_alloc_frag(unsigned int fragsz)
> static inline void *netdev_alloc_frag_align(unsigned int fragsz,
> unsigned int align)
> {
> - WARN_ON_ONCE(!is_power_of_2(align));
> + WARN_ON_ONCE(!is_power_of_2(align) || align > PAGE_SIZE);
> return __netdev_alloc_frag_align(fragsz, -align);
> }
>
> @@ -3391,7 +3391,7 @@ static inline void *napi_alloc_frag(unsigned int fragsz)
> static inline void *napi_alloc_frag_align(unsigned int fragsz,
> unsigned int align)
> {
> - WARN_ON_ONCE(!is_power_of_2(align));
> + WARN_ON_ONCE(!is_power_of_2(align) || align > PAGE_SIZE);
> return __napi_alloc_frag_align(fragsz, -align);
> }
>
> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> index 88f567ef0e29..da244851b8a4 100644
> --- a/mm/page_frag_cache.c
> +++ b/mm/page_frag_cache.c
> @@ -72,10 +72,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> if (!page)
> return NULL;
>
> -#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
> - /* if size can vary use size else just use PAGE_SIZE */
> - size = nc->size;
> -#endif
> /* Even if we own the page, we do not use atomic_set().
> * This would break get_page_unless_zero() users.
> */
> @@ -84,11 +80,16 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> /* reset page count bias and offset to start of new frag */
> nc->pfmemalloc = page_is_pfmemalloc(page);
> nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
> - nc->offset = size;
> + nc->offset = 0;
> }
>
> - offset = nc->offset - fragsz;
> - if (unlikely(offset < 0)) {
> +#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
> + /* if size can vary use size else just use PAGE_SIZE */
> + size = nc->size;
> +#endif
> +
> + offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
> + if (unlikely(offset + fragsz > size)) {
The fragsz check below could be moved to here.
> page = virt_to_page(nc->va);
>
> if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
> @@ -99,17 +100,13 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> 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;
> - if (unlikely(offset < 0)) {
> + offset = 0;
> + if (unlikely(fragsz > PAGE_SIZE)) {
Since we aren't taking advantage of the flag that is left after the
subtraction we might just want to look at moving this piece up to just
after the offset + fragsz check. That should prevent us from trying to
refill if we have a request that is larger than a single page. In
addition we could probably just drop the 3 PAGE_SIZE checks above as
they would be redundant.
> /*
> * The caller is trying to allocate a fragment
> * with fragsz > PAGE_SIZE but the cache isn't big
> @@ -124,8 +121,7 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> }
>
> nc->pagecnt_bias--;
> - offset &= align_mask;
> - nc->offset = offset;
> + nc->offset = offset + fragsz;
>
> return nc->va + offset;
> }
On 2024/7/2 7:27, Alexander H Duyck wrote:
> On Tue, 2024-06-25 at 21:52 +0800, Yunsheng Lin wrote:
>> We are above to use page_frag_alloc_*() API to not just
> "about to use", not "above to use"
Ack.
>
>> allocate memory for skb->data, but also use them to do
>> the memory allocation for skb frag too. Currently the
>> implementation of page_frag in mm subsystem is running
>> the offset as a countdown rather than count-up value,
>> there may have several advantages to that as mentioned
>> in [1], but it may have some disadvantages, for example,
>> it may disable skb frag coaleasing and more correct cache
>> prefetching
>>
...
>> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
>> index 88f567ef0e29..da244851b8a4 100644
>> --- a/mm/page_frag_cache.c
>> +++ b/mm/page_frag_cache.c
>> @@ -72,10 +72,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>> if (!page)
>> return NULL;
>>
>> -#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
>> - /* if size can vary use size else just use PAGE_SIZE */
>> - size = nc->size;
>> -#endif
>> /* Even if we own the page, we do not use atomic_set().
>> * This would break get_page_unless_zero() users.
>> */
>> @@ -84,11 +80,16 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>> /* reset page count bias and offset to start of new frag */
>> nc->pfmemalloc = page_is_pfmemalloc(page);
>> nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
>> - nc->offset = size;
>> + nc->offset = 0;
>> }
>>
>> - offset = nc->offset - fragsz;
>> - if (unlikely(offset < 0)) {
>> +#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
>> + /* if size can vary use size else just use PAGE_SIZE */
>> + size = nc->size;
>> +#endif
>> +
>> + offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
>> + if (unlikely(offset + fragsz > size)) {
>
> The fragsz check below could be moved to here.
>
>> page = virt_to_page(nc->va);
>>
>> if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
>> @@ -99,17 +100,13 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>> 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;
>> - if (unlikely(offset < 0)) {
>> + offset = 0;
>> + if (unlikely(fragsz > PAGE_SIZE)) {
>
> Since we aren't taking advantage of the flag that is left after the
> subtraction we might just want to look at moving this piece up to just
> after the offset + fragsz check. That should prevent us from trying to
> refill if we have a request that is larger than a single page. In
> addition we could probably just drop the 3 PAGE_SIZE checks above as
> they would be redundant.
I am not sure I understand the 'drop the 3 PAGE_SIZE checks' part and
the 'redundant' part, where is the '3 PAGE_SIZE checks'? And why they
are redundant?
>
>> /*
>> * The caller is trying to allocate a fragment
>> * with fragsz > PAGE_SIZE but the cache isn't big
>> @@ -124,8 +121,7 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>> }
>>
>> nc->pagecnt_bias--;
>> - offset &= align_mask;
>> - nc->offset = offset;
>> + nc->offset = offset + fragsz;
>>
>> return nc->va + offset;
>> }
>
>
> .
>
On Tue, Jul 2, 2024 at 5:28 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2024/7/2 7:27, Alexander H Duyck wrote:
> > On Tue, 2024-06-25 at 21:52 +0800, Yunsheng Lin wrote:
> >> We are above to use page_frag_alloc_*() API to not just
> > "about to use", not "above to use"
>
> Ack.
>
> >
> >> allocate memory for skb->data, but also use them to do
> >> the memory allocation for skb frag too. Currently the
> >> implementation of page_frag in mm subsystem is running
> >> the offset as a countdown rather than count-up value,
> >> there may have several advantages to that as mentioned
> >> in [1], but it may have some disadvantages, for example,
> >> it may disable skb frag coaleasing and more correct cache
> >> prefetching
> >>
>
> ...
>
> >> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> >> index 88f567ef0e29..da244851b8a4 100644
> >> --- a/mm/page_frag_cache.c
> >> +++ b/mm/page_frag_cache.c
> >> @@ -72,10 +72,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> >> if (!page)
> >> return NULL;
> >>
> >> -#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
> >> - /* if size can vary use size else just use PAGE_SIZE */
> >> - size = nc->size;
> >> -#endif
> >> /* Even if we own the page, we do not use atomic_set().
> >> * This would break get_page_unless_zero() users.
> >> */
> >> @@ -84,11 +80,16 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> >> /* reset page count bias and offset to start of new frag */
> >> nc->pfmemalloc = page_is_pfmemalloc(page);
> >> nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
> >> - nc->offset = size;
> >> + nc->offset = 0;
> >> }
> >>
> >> - offset = nc->offset - fragsz;
> >> - if (unlikely(offset < 0)) {
> >> +#if (PAGE_SIZE < PAGE_FRAG_CACHE_MAX_SIZE)
> >> + /* if size can vary use size else just use PAGE_SIZE */
> >> + size = nc->size;
> >> +#endif
> >> +
> >> + offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
> >> + if (unlikely(offset + fragsz > size)) {
> >
> > The fragsz check below could be moved to here.
> >
> >> page = virt_to_page(nc->va);
> >>
> >> if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
> >> @@ -99,17 +100,13 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
> >> 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;
> >> - if (unlikely(offset < 0)) {
> >> + offset = 0;
> >> + if (unlikely(fragsz > PAGE_SIZE)) {
> >
> > Since we aren't taking advantage of the flag that is left after the
> > subtraction we might just want to look at moving this piece up to just
> > after the offset + fragsz check. That should prevent us from trying to
> > refill if we have a request that is larger than a single page. In
> > addition we could probably just drop the 3 PAGE_SIZE checks above as
> > they would be redundant.
>
> I am not sure I understand the 'drop the 3 PAGE_SIZE checks' part and
> the 'redundant' part, where is the '3 PAGE_SIZE checks'? And why they
> are redundant?
I was referring to the addition of the checks for align > PAGE_SIZE in
the alloc functions at the start of this diff. I guess I had dropped
them from the first half of it with the "...". Also looking back
through the patch you misspelled "avoid" as "aovid".
The issue is there is a ton of pulling things forward that don't
necessarily make sense into these diffs. Now that I have finished
looking through the set I have a better idea of why those are there
and they might make sense. It is just difficult to review since code
is being added for things that aren't applicable to the patch being
reviewed.
On 2024/7/3 0:00, Alexander Duyck wrote:
...
>>>> +
>>>> + offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
>>>> + if (unlikely(offset + fragsz > size)) {
>>>
>>> The fragsz check below could be moved to here.
>>>
>>>> page = virt_to_page(nc->va);
>>>>
>>>> if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
>>>> @@ -99,17 +100,13 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>>>> 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;
>>>> - if (unlikely(offset < 0)) {
>>>> + offset = 0;
>>>> + if (unlikely(fragsz > PAGE_SIZE)) {
>>>
>>> Since we aren't taking advantage of the flag that is left after the
>>> subtraction we might just want to look at moving this piece up to just
>>> after the offset + fragsz check. That should prevent us from trying to
>>> refill if we have a request that is larger than a single page. In
>>> addition we could probably just drop the 3 PAGE_SIZE checks above as
>>> they would be redundant.
>>
>> I am not sure I understand the 'drop the 3 PAGE_SIZE checks' part and
>> the 'redundant' part, where is the '3 PAGE_SIZE checks'? And why they
>> are redundant?
>
> I was referring to the addition of the checks for align > PAGE_SIZE in
> the alloc functions at the start of this diff. I guess I had dropped
> them from the first half of it with the "...". Also looking back
> through the patch you misspelled "avoid" as "aovid".
>
> The issue is there is a ton of pulling things forward that don't
> necessarily make sense into these diffs. Now that I have finished
> looking through the set I have a better idea of why those are there
> and they might make sense. It is just difficult to review since code
> is being added for things that aren't applicable to the patch being
> reviewed.
As you mentioned in other thread, perhaps the 'remaining' changing does
need to be incorporated into this patch.
> .
>
© 2016 - 2024 Red Hat, Inc.