[PATCH] lib/genalloc: Set chunk size to real size which gen_pool managed.

[PATCH] lib/genalloc: Set chunk size to real size which gen_pool managed.

[wuchi]

The demand size (chunk->avail > size > round_down(chunk->avail)) will
lead to meaningless algo calls in gen_pool_alloc_algo_owner without the
patch, alse move the follow code:
	size = nbits << order
out of read-side critical section.

Signed-off-by: wuchi <wuchi.zero@gmail.com>
---
 lib/genalloc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/genalloc.c b/lib/genalloc.c
index 00fc50d0a640..1b1843613fb8 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -193,6 +193,7 @@ int gen_pool_add_owner(struct gen_pool *pool, unsigned long virt, phys_addr_t ph
 	if (unlikely(chunk == NULL))
 		return -ENOMEM;
 
+	size = nbits << pool->min_alloc_order;
 	chunk->phys_addr = phys;
 	chunk->start_addr = virt;
 	chunk->end_addr = virt + size - 1;
@@ -293,6 +294,7 @@ unsigned long gen_pool_alloc_algo_owner(struct gen_pool *pool, size_t size,
 		return 0;
 
 	nbits = (size + (1UL << order) - 1) >> order;
+	size = nbits << order;
 	rcu_read_lock();
 	list_for_each_entry_rcu(chunk, &pool->chunks, next_chunk) {
 		if (size > atomic_long_read(&chunk->avail))
@@ -314,7 +316,6 @@ unsigned long gen_pool_alloc_algo_owner(struct gen_pool *pool, size_t size,
 		}
 
 		addr = chunk->start_addr + ((unsigned long)start_bit << order);
-		size = nbits << order;
 		atomic_long_sub(size, &chunk->avail);
 		if (owner)
 			*owner = chunk->owner;
@@ -499,6 +500,7 @@ void gen_pool_free_owner(struct gen_pool *pool, unsigned long addr, size_t size,
 		*owner = NULL;
 
 	nbits = (size + (1UL << order) - 1) >> order;
+	size = nbits << order;
 	rcu_read_lock();
 	list_for_each_entry_rcu(chunk, &pool->chunks, next_chunk) {
 		if (addr >= chunk->start_addr && addr <= chunk->end_addr) {
@@ -506,7 +508,6 @@ void gen_pool_free_owner(struct gen_pool *pool, unsigned long addr, size_t size,
 			start_bit = (addr - chunk->start_addr) >> order;
 			remain = bitmap_clear_ll(chunk->bits, start_bit, nbits);
 			BUG_ON(remain);
-			size = nbits << order;
 			atomic_long_add(size, &chunk->avail);
 			if (owner)
 				*owner = chunk->owner;
-- 
2.20.1

Re: [PATCH] lib/genalloc: Set chunk size to real size which gen_pool managed.

[Andrew Morton]

On Sun, 12 Jun 2022 18:59:37 +0800 wuchi <wuchi.zero@gmail.com> wrote:

> The demand size (chunk->avail > size > round_down(chunk->avail)) will
> lead to meaningless algo calls in gen_pool_alloc_algo_owner without the
> patch, alse move the follow code:
> 	size = nbits << order
> out of read-side critical section.
> 

Nobody has seriously worked on this code in a long time :(

Please expand more on the flaw.  What are "algo calls"?  Why are they
meaningless, etc?  What are the runtime effects of this error?

> --- a/lib/genalloc.c
> +++ b/lib/genalloc.c
> @@ -193,6 +193,7 @@ int gen_pool_add_owner(struct gen_pool *pool, unsigned long virt, phys_addr_t ph
>  	if (unlikely(chunk == NULL))
>  		return -ENOMEM;
>  
> +	size = nbits << pool->min_alloc_order;

If we're going to do this then gen_pool_add_owner() no longer needs its
`size' argument.

Re: [PATCH] lib/genalloc: Set chunk size to real size which gen_pool managed.

[chi wu]

Andrew Morton <akpm@linux-foundation.org> 于2022年6月14日周二 02:14写道：
>
> On Sun, 12 Jun 2022 18:59:37 +0800 wuchi <wuchi.zero@gmail.com> wrote:
>
> > The demand size (chunk->avail > size > round_down(chunk->avail)) will
> > lead to meaningless algo calls in gen_pool_alloc_algo_owner without the
> > patch, alse move the follow code:
> >       size = nbits << order
> > out of read-side critical section.
> >
>
> Nobody has seriously worked on this code in a long time :(
>
> Please expand more on the flaw.  What are "algo calls"?  Why are they
> meaningless, etc?  What are the runtime effects of this error?
>
The following case may be far away from the real use scenario:
1. In function gen_pool_create, we get:
    pool->min_alloc_order = 6;       //(64)
    pool->algo = gen_pool_first_fit;

2. In function gen_pool_add_owner, add chunk (size = 4k + 32   ?) to
pool:                              [a]
    (
     nbits = size >> pool->min_alloc_order = (4k + 32) >> 6 = 4k >> 6;
     for the nbits bitmap, we managed the 4k size.
    )
    chunk->avail = 4k + 32;

3. after some alloc actions, the chunk state is:
    chunk->avail = 32;
    (the remain 32 Byte is out of managed)

4. In function gen_pool_alloc_algo_owner, we want to alloc size = 32
Byte, we may do the follow step:
    list_for_each_entry_rcu(.......)
        if (32 > chunk->avail)      ===> if (false)
            continue;
        .........
        start_bit = algo(chunk->bits, end_bit ....)      ===>
gen_pool_first_fit(chunk->bits, end_bit ...)           [b]

for [b], In step 4, the real avail size chunk managed is 0, but we
still call the algo to alloc... which is meaningless.
                           the final result is ok, just there are some
redundant steps.

for [a], the root reason is the memory size  %  pool->min_alloc_order
!= 0. (though I also doubt if this exists).
> > --- a/lib/genalloc.c
> > +++ b/lib/genalloc.c
> > @@ -193,6 +193,7 @@ int gen_pool_add_owner(struct gen_pool *pool, unsigned long virt, phys_addr_t ph
> >       if (unlikely(chunk == NULL))
> >               return -ENOMEM;
> >
> > +     size = nbits << pool->min_alloc_order;
>
> If we're going to do this then gen_pool_add_owner() no longer needs its
> `size' argument.
>
>
Sorry, What point did I misunderstand？
The chunk owns a bitmap itself,  and nbits is calculate from size in
gen_pool_add_owner:

unsigned long nbits = size >> pool->min_alloc_order;
......
size = nbits << pool->min_alloc_order;

Re: [PATCH] lib/genalloc: Set chunk size to real size which gen_pool managed.

[Andrew Morton]

On Tue, 14 Jun 2022 13:20:08 +0800 chi wu <wuchi.zero@gmail.com> wrote:

> > If we're going to do this then gen_pool_add_owner() no longer needs its
> > `size' argument.
> >
> >
> Sorry, What point did I misunderstand？

I mean we can now do this:

--- a/lib/genalloc.c~lib-genalloc-set-chunk-size-to-real-size-which-gen_pool-managed-fix
+++ a/lib/genalloc.c
@@ -181,13 +181,14 @@ EXPORT_SYMBOL(gen_pool_create);
  *
  * Returns 0 on success or a -ve errno on failure.
  */
-int gen_pool_add_owner(struct gen_pool *pool, unsigned long virt, phys_addr_t phys,
-		 size_t size, int nid, void *owner)
+int gen_pool_add_owner(struct gen_pool *pool, unsigned long virt,
+		       phys_addr_t phys, int nid, void *owner)
 {
 	struct gen_pool_chunk *chunk;
 	unsigned long nbits = size >> pool->min_alloc_order;
 	unsigned long nbytes = sizeof(struct gen_pool_chunk) +
 				BITS_TO_LONGS(nbits) * sizeof(long);
+	size_t size;
 
 	chunk = vzalloc_node(nbytes, nid);
 	if (unlikely(chunk == NULL))
_

and fix up all the callers, prototype, kerneldoc, etc.  Probably as a
second followup patch.


Please take a look at that, add the additional details to the changelog
and resend?

Re: [PATCH] lib/genalloc: Set chunk size to real size which gen_pool managed.

[chi wu]

Andrew Morton <akpm@linux-foundation.org> 于2022年6月15日周三 01:53写道：
>
> On Tue, 14 Jun 2022 13:20:08 +0800 chi wu <wuchi.zero@gmail.com> wrote:
>
> > > If we're going to do this then gen_pool_add_owner() no longer needs its
> > > `size' argument.
> > >
> > >
> > Sorry, What point did I misunderstand？
>
> I mean we can now do this:
>
> --- a/lib/genalloc.c~lib-genalloc-set-chunk-size-to-real-size-which-gen_pool-managed-fix
> +++ a/lib/genalloc.c
> @@ -181,13 +181,14 @@ EXPORT_SYMBOL(gen_pool_create);
>   *
>   * Returns 0 on success or a -ve errno on failure.
>   */
> -int gen_pool_add_owner(struct gen_pool *pool, unsigned long virt, phys_addr_t phys,
> -                size_t size, int nid, void *owner)
> +int gen_pool_add_owner(struct gen_pool *pool, unsigned long virt,
> +                      phys_addr_t phys, int nid, void *owner)
>  {
>         struct gen_pool_chunk *chunk;
>         unsigned long nbits = size >> pool->min_alloc_order;    [1]

The arg @size is in bytes of the memory chunk to add to pool,  we
really need that, [1] here we
get nbits by size. [2]  later we will alloc bitmap by nbits...   If we
remove the arg @size, we will
have no way to get the size of memory which we will manage.

>         unsigned long nbytes = sizeof(struct gen_pool_chunk) +
>                                 BITS_TO_LONGS(nbits) * sizeof(long);      [1]
> +       size_t size;
>
>         chunk = vzalloc_node(nbytes, nid);
>         if (unlikely(chunk == NULL))
> _
>

Thanks for Andrew's time