Re: [RESEND PATCH 1/1] mm/percpu.c: correct max_distance calculation for pcpu_embed_first_chunk()

[zijun_hu]

On 2016/9/29 18:35, Tejun Heo wrote:
> Hello,
> 
> On Sat, Sep 24, 2016 at 07:20:49AM +0800, zijun_hu wrote:
>> it is error to represent the max range max_distance spanned by all the
>> group areas as the offset of the highest group area plus unit size in
>> pcpu_embed_first_chunk(), it should equal to the offset plus the size
>> of the highest group area
>>
>> in order to fix this issue,let us find the highest group area who has the
>> biggest base address among all the ones, then max_distance is formed by
>> add it's offset and size value
> 
>  [PATCH] percpu: fix max_distance calculation in pcpu_embed_first_chunk()
> 
>  pcpu_embed_first_chunk() calculates the range a percpu chunk spans
>  into max_distance and uses it to ensure that a chunk is not too big
>  compared to the total vmalloc area.  However, during calculation, it
>  used incorrect top address by adding a unit size to the higest
>  group's base address.
> 
>  This can make the calculated max_distance slightly smaller than the
>  actual distance although given the scale of values involved the error
>  is very unlikely to have an actual impact.
> 
>  Fix this issue by adding the group's size instead of a unit size.
> 
>> the type of variant max_distance is changed from size_t to unsigned long
>> to prevent potential overflow
> 
> This doesn't make any sense.  All the values involved are valid
> addresses (or +1 of it), they can't overflow and size_t is the same
> size as ulong.
> 
>> @@ -2025,17 +2026,18 @@ int __init pcpu_embed_first_chunk(size_t 
>> reserved_size, size_t dyn_size,
>>  }
>>  
>>  /* base address is now known, determine group base offsets */
>> -max_distance = 0;
>> +i = 0;
>>  for (group = 0; group < ai->nr_groups; group++) {
>>  ai->groups[group].base_offset = areas[group] - base;
>> -max_distance = max_t(size_t, max_distance,
>> - ai->groups[group].base_offset);
>> +if (areas[group] > areas[i])
>> +i = group;
>>  }
>> -max_distance += ai->unit_size;
>> +max_distance = ai->groups[i].base_offset +
>> +(unsigned long)ai->unit_size * ai->groups[i].nr_units;
> 
> I don't think you need ulong cast here.
> 
> Thanks.
> 
okay, thanks for your reply
i will correct this in another patch

Re: [RESEND PATCH 1/1] mm/percpu.c: correct max_distance calculation for pcpu_embed_first_chunk()

[zijun_hu]

On 2016/9/29 18:35, Tejun Heo wrote:
> Hello,
> 
> On Sat, Sep 24, 2016 at 07:20:49AM +0800, zijun_hu wrote:
>> it is error to represent the max range max_distance spanned by all the
>> group areas as the offset of the highest group area plus unit size in
>> pcpu_embed_first_chunk(), it should equal to the offset plus the size
>> of the highest group area
>>
>> in order to fix this issue,let us find the highest group area who has the
>> biggest base address among all the ones, then max_distance is formed by
>> add it's offset and size value
> 
>  [PATCH] percpu: fix max_distance calculation in pcpu_embed_first_chunk()
> 
>  pcpu_embed_first_chunk() calculates the range a percpu chunk spans
>  into max_distance and uses it to ensure that a chunk is not too big
>  compared to the total vmalloc area.  However, during calculation, it
>  used incorrect top address by adding a unit size to the higest
>  group's base address.
> 
>  This can make the calculated max_distance slightly smaller than the
>  actual distance although given the scale of values involved the error
>  is very unlikely to have an actual impact.
> 
>  Fix this issue by adding the group's size instead of a unit size.
> 
>> the type of variant max_distance is changed from size_t to unsigned long
>> to prevent potential overflow
> 
> This doesn't make any sense.  All the values involved are valid
> addresses (or +1 of it), they can't overflow and size_t is the same
> size as ulong.
> 
>> @@ -2025,17 +2026,18 @@ int __init pcpu_embed_first_chunk(size_t 
>> reserved_size, size_t dyn_size,
>>  }
>>  
>>  /* base address is now known, determine group base offsets */
>> -max_distance = 0;
>> +i = 0;
>>  for (group = 0; group < ai->nr_groups; group++) {
>>  ai->groups[group].base_offset = areas[group] - base;
>> -max_distance = max_t(size_t, max_distance,
>> - ai->groups[group].base_offset);
>> +if (areas[group] > areas[i])
>> +i = group;
>>  }
>> -max_distance += ai->unit_size;
>> +max_distance = ai->groups[i].base_offset +
>> +(unsigned long)ai->unit_size * ai->groups[i].nr_units;
> 
> I don't think you need ulong cast here.
> 
> Thanks.
> 
okay, thanks for your reply
i will correct this in another patch

Re: [RESEND PATCH 1/1] mm/percpu.c: correct max_distance calculation for pcpu_embed_first_chunk()

[Tejun Heo]

Hello,

On Sat, Sep 24, 2016 at 07:20:49AM +0800, zijun_hu wrote:
> it is error to represent the max range max_distance spanned by all the
> group areas as the offset of the highest group area plus unit size in
> pcpu_embed_first_chunk(), it should equal to the offset plus the size
> of the highest group area
> 
> in order to fix this issue,let us find the highest group area who has the
> biggest base address among all the ones, then max_distance is formed by
> add it's offset and size value

 [PATCH] percpu: fix max_distance calculation in pcpu_embed_first_chunk()

 pcpu_embed_first_chunk() calculates the range a percpu chunk spans
 into max_distance and uses it to ensure that a chunk is not too big
 compared to the total vmalloc area.  However, during calculation, it
 used incorrect top address by adding a unit size to the higest
 group's base address.

 This can make the calculated max_distance slightly smaller than the
 actual distance although given the scale of values involved the error
 is very unlikely to have an actual impact.

 Fix this issue by adding the group's size instead of a unit size.

> the type of variant max_distance is changed from size_t to unsigned long
> to prevent potential overflow

This doesn't make any sense.  All the values involved are valid
addresses (or +1 of it), they can't overflow and size_t is the same
size as ulong.

> @@ -2025,17 +2026,18 @@ int __init pcpu_embed_first_chunk(size_t 
> reserved_size, size_t dyn_size,
>   }
>  
>   /* base address is now known, determine group base offsets */
> - max_distance = 0;
> + i = 0;
>   for (group = 0; group < ai->nr_groups; group++) {
>   ai->groups[group].base_offset = areas[group] - base;
> - max_distance = max_t(size_t, max_distance,
> -  ai->groups[group].base_offset);
> + if (areas[group] > areas[i])
> + i = group;
>   }
> - max_distance += ai->unit_size;
> + max_distance = ai->groups[i].base_offset +
> + (unsigned long)ai->unit_size * ai->groups[i].nr_units;

I don't think you need ulong cast here.

Thanks.

-- 
tejun

Re: [RESEND PATCH 1/1] mm/percpu.c: correct max_distance calculation for pcpu_embed_first_chunk()

[Tejun Heo]

Hello,

On Sat, Sep 24, 2016 at 07:20:49AM +0800, zijun_hu wrote:
> it is error to represent the max range max_distance spanned by all the
> group areas as the offset of the highest group area plus unit size in
> pcpu_embed_first_chunk(), it should equal to the offset plus the size
> of the highest group area
> 
> in order to fix this issue,let us find the highest group area who has the
> biggest base address among all the ones, then max_distance is formed by
> add it's offset and size value

 [PATCH] percpu: fix max_distance calculation in pcpu_embed_first_chunk()

 pcpu_embed_first_chunk() calculates the range a percpu chunk spans
 into max_distance and uses it to ensure that a chunk is not too big
 compared to the total vmalloc area.  However, during calculation, it
 used incorrect top address by adding a unit size to the higest
 group's base address.

 This can make the calculated max_distance slightly smaller than the
 actual distance although given the scale of values involved the error
 is very unlikely to have an actual impact.

 Fix this issue by adding the group's size instead of a unit size.

> the type of variant max_distance is changed from size_t to unsigned long
> to prevent potential overflow

This doesn't make any sense.  All the values involved are valid
addresses (or +1 of it), they can't overflow and size_t is the same
size as ulong.

> @@ -2025,17 +2026,18 @@ int __init pcpu_embed_first_chunk(size_t 
> reserved_size, size_t dyn_size,
>   }
>  
>   /* base address is now known, determine group base offsets */
> - max_distance = 0;
> + i = 0;
>   for (group = 0; group < ai->nr_groups; group++) {
>   ai->groups[group].base_offset = areas[group] - base;
> - max_distance = max_t(size_t, max_distance,
> -  ai->groups[group].base_offset);
> + if (areas[group] > areas[i])
> + i = group;
>   }
> - max_distance += ai->unit_size;
> + max_distance = ai->groups[i].base_offset +
> + (unsigned long)ai->unit_size * ai->groups[i].nr_units;

I don't think you need ulong cast here.

Thanks.

-- 
tejun

Re: [RESEND PATCH 1/1] mm/percpu.c: correct max_distance calculation for pcpu_embed_first_chunk()

[zijun_hu]

From: zijun_hu 

it is error to represent the max range max_distance spanned by all the
group areas as the offset of the highest group area plus unit size in
pcpu_embed_first_chunk(), it should equal to the offset plus the size
of the highest group area

in order to fix this issue,let us find the highest group area who has the
biggest base address among all the ones, then max_distance is formed by
add it's offset and size value

the type of variant max_distance is changed from size_t to unsigned long
to prevent potential overflow

Signed-off-by: zijun_hu 
---
 more detailed commit messages is provided against the previous one as
 advised by t...@kernel.org

 mm/percpu.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index fcaaac977954..ee0d1c93f070 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1963,7 +1963,8 @@ int __init pcpu_embed_first_chunk(size_t reserved_size, 
size_t dyn_size,
void *base = (void *)ULONG_MAX;
void **areas = NULL;
struct pcpu_alloc_info *ai;
-   size_t size_sum, areas_size, max_distance;
+   size_t size_sum, areas_size;
+   unsigned long max_distance;
int group, i, rc;
 
ai = pcpu_build_alloc_info(reserved_size, dyn_size, atom_size,
@@ -2025,17 +2026,18 @@ int __init pcpu_embed_first_chunk(size_t reserved_size, 
size_t dyn_size,
}
 
/* base address is now known, determine group base offsets */
-   max_distance = 0;
+   i = 0;
for (group = 0; group < ai->nr_groups; group++) {
ai->groups[group].base_offset = areas[group] - base;
-   max_distance = max_t(size_t, max_distance,
-ai->groups[group].base_offset);
+   if (areas[group] > areas[i])
+   i = group;
}
-   max_distance += ai->unit_size;
+   max_distance = ai->groups[i].base_offset +
+   (unsigned long)ai->unit_size * ai->groups[i].nr_units;
 
/* warn if maximum distance is further than 75% of vmalloc space */
if (max_distance > VMALLOC_TOTAL * 3 / 4) {
-   pr_warn("max_distance=0x%zx too large for vmalloc space 
0x%lx\n",
+   pr_warn("max_distance=0x%lx too large for vmalloc space 
0x%lx\n",
max_distance, VMALLOC_TOTAL);
 #ifdef CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK
/* and fail if we have fallback */
-- 
1.9.1

Re: [RESEND PATCH 1/1] mm/percpu.c: correct max_distance calculation for pcpu_embed_first_chunk()

[zijun_hu]

From: zijun_hu 

it is error to represent the max range max_distance spanned by all the
group areas as the offset of the highest group area plus unit size in
pcpu_embed_first_chunk(), it should equal to the offset plus the size
of the highest group area

in order to fix this issue,let us find the highest group area who has the
biggest base address among all the ones, then max_distance is formed by
add it's offset and size value

the type of variant max_distance is changed from size_t to unsigned long
to prevent potential overflow

Signed-off-by: zijun_hu 
---
 more detailed commit messages is provided against the previous one as
 advised by t...@kernel.org

 mm/percpu.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index fcaaac977954..ee0d1c93f070 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1963,7 +1963,8 @@ int __init pcpu_embed_first_chunk(size_t reserved_size, 
size_t dyn_size,
void *base = (void *)ULONG_MAX;
void **areas = NULL;
struct pcpu_alloc_info *ai;
-   size_t size_sum, areas_size, max_distance;
+   size_t size_sum, areas_size;
+   unsigned long max_distance;
int group, i, rc;
 
ai = pcpu_build_alloc_info(reserved_size, dyn_size, atom_size,
@@ -2025,17 +2026,18 @@ int __init pcpu_embed_first_chunk(size_t reserved_size, 
size_t dyn_size,
}
 
/* base address is now known, determine group base offsets */
-   max_distance = 0;
+   i = 0;
for (group = 0; group < ai->nr_groups; group++) {
ai->groups[group].base_offset = areas[group] - base;
-   max_distance = max_t(size_t, max_distance,
-ai->groups[group].base_offset);
+   if (areas[group] > areas[i])
+   i = group;
}
-   max_distance += ai->unit_size;
+   max_distance = ai->groups[i].base_offset +
+   (unsigned long)ai->unit_size * ai->groups[i].nr_units;
 
/* warn if maximum distance is further than 75% of vmalloc space */
if (max_distance > VMALLOC_TOTAL * 3 / 4) {
-   pr_warn("max_distance=0x%zx too large for vmalloc space 
0x%lx\n",
+   pr_warn("max_distance=0x%lx too large for vmalloc space 
0x%lx\n",
max_distance, VMALLOC_TOTAL);
 #ifdef CONFIG_NEED_PER_CPU_PAGE_FIRST_CHUNK
/* and fail if we have fallback */
-- 
1.9.1