[PATCH 0/2] mm: Explicitly check & doc fragsz limit

[PATCH 0/2] mm: Explicitly check & doc fragsz limit

[Haiyang Zhang]

The page frag allocator is not designed for fragsz > PAGE_SIZE.
Explicitly check it in the function & document the fragsz limit.

Haiyang Zhang (2):
  mm: page_frag: Check fragsz at the beginning of __page_frag_alloc_align()
  docs/mm: Specify page frag size is not bigger than PAGE_SIZE

 Documentation/mm/page_frags.rst |  2 +-
 mm/page_frag_cache.c            | 22 +++++++++-------------
 2 files changed, 10 insertions(+), 14 deletions(-)

-- 
2.34.1

[PATCH 1/2] mm: page_frag: Check fragsz at the beginning of __page_frag_alloc_align()

[Haiyang Zhang]

Frag allocator is not designed for fragsz > PAGE_SIZE. So, check and return
the error at the beginning of __page_frag_alloc_align(), instead of
succeed for a few times, then fail due to not refilling the cache.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 mm/page_frag_cache.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
index d2423f30577e..d6bf022087e7 100644
--- a/mm/page_frag_cache.c
+++ b/mm/page_frag_cache.c
@@ -98,6 +98,15 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
 	unsigned int size, offset;
 	struct page *page;
 
+	if (unlikely(fragsz > PAGE_SIZE)) {
+		/*
+		 * The caller is trying to allocate a fragment
+		 * with fragsz > PAGE_SIZE which is not supported
+		 * by design. So we simply return NULL here.
+		 */
+		return NULL;
+	}
+
 	if (unlikely(!encoded_page)) {
 refill:
 		page = __page_frag_cache_refill(nc, gfp_mask);
@@ -119,19 +128,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
 	size = PAGE_SIZE << encoded_page_decode_order(encoded_page);
 	offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
 	if (unlikely(offset + fragsz > size)) {
-		if (unlikely(fragsz > PAGE_SIZE)) {
-			/*
-			 * 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;
-		}
-
 		page = encoded_page_decode_page(encoded_page);
 
 		if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
-- 
2.34.1

[PATCH 2/2] docs/mm: Specify page frag size is not bigger than PAGE_SIZE

[Haiyang Zhang]

The page frag allocator is not designed for fragsz > PAGE_SIZE.
Specify this in the document.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 Documentation/mm/page_frags.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/mm/page_frags.rst b/Documentation/mm/page_frags.rst
index 503ca6cdb804..212ecc69dd74 100644
--- a/Documentation/mm/page_frags.rst
+++ b/Documentation/mm/page_frags.rst
@@ -2,7 +2,7 @@
 Page fragments
 ==============
 
-A page fragment is an arbitrary-length arbitrary-offset area of memory
+A page fragment is a len <= PAGE_SIZE, arbitrary-offset area of memory
 which resides within a 0 or higher order compound page.  Multiple
 fragments within that page are individually refcounted, in the page's
 reference counter.
-- 
2.34.1

RE: [PATCH 0/2] mm: Explicitly check & doc fragsz limit

[Haiyang Zhang]

> -----Original Message-----
> From: LKML haiyangz <lkmlhyz@microsoft.com> On Behalf Of Haiyang Zhang
> Sent: Thursday, April 3, 2025 5:22 PM
> To: linux-hyperv@vger.kernel.org; akpm@linux-foundation.org;
> corbet@lwn.net; linux-mm@kvack.org; linux-doc@vger.kernel.org
> Cc: Haiyang Zhang <haiyangz@microsoft.com>; Dexuan Cui
> <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>; Paul Rosswurm
> <paulros@microsoft.com>; olaf@aepfle.de; vkuznets <vkuznets@redhat.com>;
> davem@davemloft.net; wei.liu@kernel.org; Long Li <longli@microsoft.com>;
> linux-kernel@vger.kernel.org
> Subject: [PATCH 0/2] mm: Explicitly check & doc fragsz limit
> 
> The page frag allocator is not designed for fragsz > PAGE_SIZE.
> Explicitly check it in the function & document the fragsz limit.
> 
> Haiyang Zhang (2):
>   mm: page_frag: Check fragsz at the beginning of
> __page_frag_alloc_align()
>   docs/mm: Specify page frag size is not bigger than PAGE_SIZE
> 
>  Documentation/mm/page_frags.rst |  2 +-
>  mm/page_frag_cache.c            | 22 +++++++++-------------
>  2 files changed, 10 insertions(+), 14 deletions(-)
> 

+@Mike Rapoport (Microsoft) <rppt@kernel.org>
Hi Mike, since you have a lot experience in mm subsystem, could you please 
review my patch set?

Thanks,
- Haiyang

Re: [PATCH 1/2] mm: page_frag: Check fragsz at the beginning of __page_frag_alloc_align()

[Yunsheng Lin]

On 2025/4/4 5:21, Haiyang Zhang wrote:
> Frag allocator is not designed for fragsz > PAGE_SIZE. So, check and return
> the error at the beginning of __page_frag_alloc_align(), instead of
> succeed for a few times, then fail due to not refilling the cache.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  mm/page_frag_cache.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> index d2423f30577e..d6bf022087e7 100644
> --- a/mm/page_frag_cache.c
> +++ b/mm/page_frag_cache.c
> @@ -98,6 +98,15 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>  	unsigned int size, offset;
>  	struct page *page;
>  
> +	if (unlikely(fragsz > PAGE_SIZE)) {
> +		/*
> +		 * The caller is trying to allocate a fragment
> +		 * with fragsz > PAGE_SIZE which is not supported
> +		 * by design. So we simply return NULL here.
> +		 */
> +		return NULL;
> +	}

The checking is done at below to avoid doing the checking for the
likely case of cache being enough as the frag API is mostly used
to allocate small memory.

And it seems my recent refactoring to frag API have made two
frag API misuse more obvious if I recalled it correctly. If more
explicit about that for all the codepath is really helpful, perhaps
VM_BUG_ON() is an option to make it more explicit while avoiding
the checking as much as possible.

> +
>  	if (unlikely(!encoded_page)) {
>  refill:
>  		page = __page_frag_cache_refill(nc, gfp_mask);
> @@ -119,19 +128,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>  	size = PAGE_SIZE << encoded_page_decode_order(encoded_page);
>  	offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
>  	if (unlikely(offset + fragsz > size)) {
> -		if (unlikely(fragsz > PAGE_SIZE)) {
> -			/*
> -			 * 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;
> -		}
> -
>  		page = encoded_page_decode_page(encoded_page);
>  
>  		if (!page_ref_sub_and_test(page, nc->pagecnt_bias))

RE: [EXTERNAL] Re: [PATCH 1/2] mm: page_frag: Check fragsz at the beginning of __page_frag_alloc_align()

[Haiyang Zhang]

> -----Original Message-----
> From: Yunsheng Lin <linyunsheng@huawei.com>
> Sent: Wednesday, April 16, 2025 3:38 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>; linux-hyperv@vger.kernel.org;
> akpm@linux-foundation.org; corbet@lwn.net; linux-mm@kvack.org; linux-
> doc@vger.kernel.org
> Cc: Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>;
> Paul Rosswurm <paulros@microsoft.com>; olaf@aepfle.de;
> vkuznets@redhat.com; davem@davemloft.net; wei.liu@kernel.org; Long Li
> <longli@microsoft.com>; linux-kernel@vger.kernel.org
> Subject: [EXTERNAL] Re: [PATCH 1/2] mm: page_frag: Check fragsz at the
> beginning of __page_frag_alloc_align()
> 
> On 2025/4/4 5:21, Haiyang Zhang wrote:
> > Frag allocator is not designed for fragsz > PAGE_SIZE. So, check and
> return
> > the error at the beginning of __page_frag_alloc_align(), instead of
> > succeed for a few times, then fail due to not refilling the cache.
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > ---
> >  mm/page_frag_cache.c | 22 +++++++++-------------
> >  1 file changed, 9 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/page_frag_cache.c b/mm/page_frag_cache.c
> > index d2423f30577e..d6bf022087e7 100644
> > --- a/mm/page_frag_cache.c
> > +++ b/mm/page_frag_cache.c
> > @@ -98,6 +98,15 @@ void *__page_frag_alloc_align(struct page_frag_cache
> *nc,
> >  	unsigned int size, offset;
> >  	struct page *page;
> >
> > +	if (unlikely(fragsz > PAGE_SIZE)) {
> > +		/*
> > +		 * The caller is trying to allocate a fragment
> > +		 * with fragsz > PAGE_SIZE which is not supported
> > +		 * by design. So we simply return NULL here.
> > +		 */
> > +		return NULL;
> > +	}
> 
> The checking is done at below to avoid doing the checking for the
> likely case of cache being enough as the frag API is mostly used
> to allocate small memory.
> 
> And it seems my recent refactoring to frag API have made two
> frag API misuse more obvious if I recalled it correctly. If more
> explicit about that for all the codepath is really helpful, perhaps
> VM_BUG_ON() is an option to make it more explicit while avoiding
> the checking as much as possible.

Thanks, I will use VM_BUG_ON() instead of a checking here. Also, will
keep the checking below unchanged.

> > +
> >  	if (unlikely(!encoded_page)) {
> >  refill:
> >  		page = __page_frag_cache_refill(nc, gfp_mask);
> > @@ -119,19 +128,6 @@ void *__page_frag_alloc_align(struct
> page_frag_cache *nc,
> >  	size = PAGE_SIZE << encoded_page_decode_order(encoded_page);
> >  	offset = __ALIGN_KERNEL_MASK(nc->offset, ~align_mask);
> >  	if (unlikely(offset + fragsz > size)) {
> > -		if (unlikely(fragsz > PAGE_SIZE)) {
> > -			/*
> > -			 * 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;
> > -		}
> > -
> >  		page = encoded_page_decode_page(encoded_page);
> >
> >  		if (!page_ref_sub_and_test(page, nc->pagecnt_bias))