Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490374 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/1] mm/percpu.c: correct max_distance calculation for  pcpu_embed_first_chunk() zijun_hu <zijun_hu@zoho.com> - 2016-09-23 20:30 +0200
    Re: [PATCH 1/1] mm/percpu.c: correct max_distance calculation for  pcpu_embed_first_chunk() Tejun Heo <tj@kernel.org> - 2016-09-23 21:30 +0200
      Re: [PATCH 1/1] mm/percpu.c: correct max_distance calculation for  pcpu_embed_first_chunk() zijun_hu <zijun_hu@zoho.com> - 2016-09-23 23:20 +0200
        Re: [PATCH 1/1] mm/percpu.c: correct max_distance calculation for  pcpu_embed_first_chunk() Tejun Heo <tj@kernel.org> - 2016-09-23 23:40 +0200
    Re: [RESEND PATCH 1/1] mm/percpu.c: correct max_distance calculation  for pcpu_embed_first_chunk() zijun_hu <zijun_hu@zoho.com> - 2016-09-24 01:30 +0200
      Re: [RESEND PATCH 1/1] mm/percpu.c: correct max_distance calculation  for pcpu_embed_first_chunk() Tejun Heo <tj@kernel.org> - 2016-09-29 12:40 +0200
        Re: [RESEND PATCH 1/1] mm/percpu.c: correct max_distance calculation  for pcpu_embed_first_chunk() zijun_hu <zijun_hu@zoho.com> - 2016-09-29 13:30 +0200

#1490374 — [PATCH 1/1] mm/percpu.c: correct max_distance calculation for pcpu_embed_first_chunk()

From: zijun_hu <zijun_hu@htc.com>

correct max_distance from (base of the highest group + ai->unit_size)
to (base of the highest group + the group size)

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
 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

[toc] | [next] | [standalone]

#1490401

On Sat, Sep 24, 2016 at 02:20:24AM +0800, zijun_hu wrote:
> From: zijun_hu <zijun_hu@htc.com>
> 
> correct max_distance from (base of the highest group + ai->unit_size)
> to (base of the highest group + the group size)
> 
> Signed-off-by: zijun_hu <zijun_hu@htc.com>

Nacked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1490464

On 2016/9/24 3:23, Tejun Heo wrote:
> On Sat, Sep 24, 2016 at 02:20:24AM +0800, zijun_hu wrote:
>> From: zijun_hu <zijun_hu@htc.com>
>>
>> correct max_distance from (base of the highest group + ai->unit_size)
>> to (base of the highest group + the group size)
>>
>> Signed-off-by: zijun_hu <zijun_hu@htc.com>
> 
> Nacked-by: Tejun Heo <tj@kernel.org>
> 
> Thanks.
>
frankly, the current max_distance is error, doesn't represents the ranges spanned by
areas owned by the groups

[toc] | [prev] | [next] | [standalone]

#1490470

Hello,

On Sat, Sep 24, 2016 at 05:16:56AM +0800, zijun_hu wrote:
> On 2016/9/24 3:23, Tejun Heo wrote:
> > On Sat, Sep 24, 2016 at 02:20:24AM +0800, zijun_hu wrote:
> >> From: zijun_hu <zijun_hu@htc.com>
> >>
> >> correct max_distance from (base of the highest group + ai->unit_size)
> >> to (base of the highest group + the group size)
> >>
> >> Signed-off-by: zijun_hu <zijun_hu@htc.com>
> > 
> > Nacked-by: Tejun Heo <tj@kernel.org>
> > 
> > Thanks.
> >
> frankly, the current max_distance is error, doesn't represents the ranges spanned by
> areas owned by the groups

I think you're right but can you please update the patch description
so that it explains what the bug and the implications are and how the
patch fixes the bug.  Also, please make sure that all changes made by
the patch are explained in the description - e.g. why the type of
@max_distance is changed from size_t to ulong.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

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

From: zijun_hu <zijun_hu@htc.com>

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 <zijun_hu@htc.com>
---
 more detailed commit messages is provided against the previous one as
 advised by tj@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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web