mm/percpu.c: fix panic triggered by BUG_ON() falsely

RFC PATCH Applied zijun_hu 3 participants 9 messages Oct 11, 2016 – Oct 14, 2016

Subsystems (2)

Memory management Per-CPU memory allocator

lore

Share & tools

lore

zijun_hu

author

2016-10-11 14:01 UTC #0 lore raw

RFC PATCH v2 1/1 mm/percpu.c: fix panic triggered by BUG_ON() falsely

From: zijun_hu <[email protected]>

as shown by pcpu_build_alloc_info(), the number of units within a percpu group is educed by rounding up the number of CPUs within the group to @upa boundary, therefore, the number of CPUs isn't equal to the units's if it isn't aligned to @upa normally. however, pcpu_page_first_chunk() uses BUG_ON() to assert one number is equal the other roughly, so a panic is maybe triggered by the BUG_ON() falsely.

in order to fix this issue, the number of CPUs is rounded up then compared with units's, the BUG_ON() is replaced by warning and returning error code as well to keep system alive as much as possible.

Signed-off-by zijun_hu <[email protected]>

---
Changes in v2:
- fix build error

 mm/percpu.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

mm/percpu.c +12 -4

index 32e2d8d128c1..ab1186c68ab6 100644
@@ -2095,6 +2095,8 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
 	size_t pages_size;
 	struct page **pages;
 	int unit, i, j, rc;
+	int upa;
+	int nr_g0_units;
 
 	snprintf(psize_str, sizeof(psize_str), "%luK", PAGE_SIZE >> 10);

@@ -2102,7 +2104,12 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
 	if (IS_ERR(ai))
 		return PTR_ERR(ai);
 	BUG_ON(ai->nr_groups != 1);
-	BUG_ON(ai->groups[0].nr_units != num_possible_cpus());
+	upa = ai->alloc_size/ai->unit_size;
+	nr_g0_units = roundup(num_possible_cpus(), upa);
+	if (unlikely(WARN_ON(ai->groups[0].nr_units != nr_g0_units))) {
+		pcpu_free_alloc_info(ai);
+		return -EINVAL;
+	}
 
 	unit_pages = ai->unit_size >> PAGE_SHIFT;

@@ -2113,21 +2120,22 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
 
 	/* allocate pages */
 	j = 0;
-	for (unit = 0; unit < num_possible_cpus(); unit++)
+	for (unit = 0; unit < num_possible_cpus(); unit++) {
+		unsigned int cpu = ai->groups[0].cpu_map[unit];
 		for (i = 0; i < unit_pages; i++) {
-			unsigned int cpu = ai->groups[0].cpu_map[unit];
 			void *ptr;
 
 			ptr = alloc_fn(cpu, PAGE_SIZE, PAGE_SIZE);
 			if (!ptr) {
 				pr_warn("failed to allocate %s page for cpu%u\n",
-					psize_str, cpu);
+						psize_str, cpu);
 				goto enomem;
 			}
 			/* kmemleak tracks the percpu allocations separately */
 			kmemleak_free(ptr);
 			pages[j++] = virt_to_page(ptr);
 		}
+	}
 
 	/* allocate vm area, map the pages and copy static data */
 	vm.flags = VM_ALLOC;

Signature · 1 line

1.9.1

Andrew Morton

maintainer

2016-10-12 21:41 UTC #1 lore raw

Quoted 10 lines · On Tue, 11 Oct 2016 22:00:28 +0800 zijun_hu <[email protected]> wrote:

as shown by pcpu_build_alloc_info(), the number of units within a percpu group is educed by rounding up the number of CPUs within the group to @upa boundary, therefore, the number of CPUs isn't equal to the units's if it isn't aligned to @upa normally. however, pcpu_page_first_chunk() uses BUG_ON() to assert one number is equal the other roughly, so a panic is maybe triggered by the BUG_ON() falsely.
in order to fix this issue, the number of CPUs is rounded up then compared with units's, the BUG_ON() is replaced by warning and returning error code as well to keep system alive as much as possible.

Under what circumstances is the triggered? In other words, what are the end-user visible effects of the fix?

I mean, this is pretty old code (isn't it?) so what are you doing that triggers this?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

zijun_hu

author

2016-10-13 00:07 UTC #2 lore raw

Quoted 9 lines · Andrew Morton

Quoted 7 lines · On Tue, 11 Oct 2016 22:00:28 +0800 zijun_hu <[email protected]> wrote:
as shown by pcpu_build_alloc_info(), the number of units within a percpu group is educed by rounding up the number of CPUs within the group to @upa boundary, therefore, the number of CPUs isn't equal to the units's if it isn't aligned to @upa normally. however, pcpu_page_first_chunk() uses BUG_ON() to assert one number is equal the other roughly, so a panic is maybe triggered by the BUG_ON() falsely.

Quoted 7 lines

Quoted 3 lines
in order to fix this issue, the number of CPUs is rounded up then compared with units's, the BUG_ON() is replaced by warning and returning error code as well to keep system alive as much as possible.
Under what circumstances is the triggered? In other words, what are the end-user visible effects of the fix?

the BUG_ON() takes effect when the number isn't aligned @upa, the BUG_ON() should not be triggered under this normal circumstances.
the aim of this fixing is prevent the BUG_ON() which is triggered under the case.

see below original code segments for reason.
pcpu_build_alloc_info(){
...

	for_each_possible_cpu(cpu)
		if (group_map[cpu] == group)
			gi->cpu_map[gi->nr_units++] = cpu;
	gi->nr_units = roundup(gi->nr_units, upa);

calculate the number of CPUs belonging to a group into relevant @gi->nr_units then roundup @gi->nr_units up to @upa for itself

unit += gi->nr_units;
...
}

pcpu_page_first_chunk() {
...

	ai = pcpu_build_alloc_info(reserved_size, 0, PAGE_SIZE, NULL);
	if (IS_ERR(ai))
		return PTR_ERR(ai);
	BUG_ON(ai->nr_groups != 1);
	BUG_ON(ai->groups[0].nr_units != num_possible_cpus());

it seems there is only one group and all CPUs belong to the group but compare the number of CPUs with the number of units directly.

as shown by comments in above function. ai->groups[0].nr_units should equal to roundup(num_possible_cpus(), @upa) other than num_possible_cpus() directly.
...
}

Quoted 4 lines

I mean, this is pretty old code (isn't it?) so what are you doing that triggers this?

i am learning memory source and find the inconsistency and think the BUG_ON() maybe be triggered under this special normal but possible case

zijun_hu

author

2016-10-13 00:10 UTC #3 lore raw

In reply to Andrew Morton

Quoted 9 lines · Andrew Morton

Quoted 7 lines · On Tue, 11 Oct 2016 22:00:28 +0800 zijun_hu <[email protected]> wrote:
as shown by pcpu_build_alloc_info(), the number of units within a percpu group is educed by rounding up the number of CPUs within the group to @upa boundary, therefore, the number of CPUs isn't equal to the units's if it isn't aligned to @upa normally. however, pcpu_page_first_chunk() uses BUG_ON() to assert one number is equal the other roughly, so a panic is maybe triggered by the BUG_ON() falsely.

Quoted 7 lines

Quoted 3 lines
in order to fix this issue, the number of CPUs is rounded up then compared with units's, the BUG_ON() is replaced by warning and returning error code as well to keep system alive as much as possible.
Under what circumstances is the triggered? In other words, what are the end-user visible effects of the fix?

the BUG_ON() takes effect when the number of CPUs isn't aligned @upa, the BUG_ON() should not be triggered under this normal circumstances. the aim of this fixing is prevent the BUG_ON() which is triggered under the case.

see below original code segments for reason.
pcpu_build_alloc_info(){
...

	for_each_possible_cpu(cpu)
		if (group_map[cpu] == group)
			gi->cpu_map[gi->nr_units++] = cpu;
	gi->nr_units = roundup(gi->nr_units, upa);

calculate the number of CPUs belonging to a group into relevant @gi->nr_units then roundup @gi->nr_units up to @upa for itself

	unit += gi->nr_units;

...
}

pcpu_page_first_chunk() {
...

	ai = pcpu_build_alloc_info(reserved_size, 0, PAGE_SIZE, NULL);
	if (IS_ERR(ai))
		return PTR_ERR(ai);
	BUG_ON(ai->nr_groups != 1);
	BUG_ON(ai->groups[0].nr_units != num_possible_cpus());

it seems there is only one group and all CPUs belong to the group but compare the number of CPUs with the number of units directly.
...
}

as shown by comments in above function. ai->groups[0].nr_units should equal to roundup(num_possible_cpus(), @upa) other than num_possible_cpus() directly.

Quoted 4 lines

I mean, this is pretty old code (isn't it?) so what are you doing that triggers this?

i am learning memory management source and find the inconsistency and think the BUG_ON() maybe be triggered under this special normal but possible case it maybe a logic error

Tejun Heo

maintainer

2016-10-13 23:29 UTC #4 lore raw

In reply to zijun_hu

Quoted 9 lines · zijun_hu

From: zijun_hu <[email protected]>
as shown by pcpu_build_alloc_info(), the number of units within a percpu group is educed by rounding up the number of CPUs within the group to @upa boundary, therefore, the number of CPUs isn't equal to the units's if it isn't aligned to @upa normally. however, pcpu_page_first_chunk() uses BUG_ON() to assert one number is equal the other roughly, so a panic is maybe triggered by the BUG_ON() falsely.

Quoted 3 lines

in order to fix this issue, the number of CPUs is rounded up then compared with units's, the BUG_ON() is replaced by warning and returning error code as well to keep system alive as much as possible.

I really can't decode what the actual issue is here. Can you please give an example of a concrete case?

Quoted 6 lines

mm/percpu.c +1 -1

@@ -2113,21 +2120,22 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
 
 	/* allocate pages */
 	j = 0;
-	for (unit = 0; unit < num_possible_cpus(); unit++)
+	for (unit = 0; unit < num_possible_cpus(); unit++) {

mm/percpu.c int __init pcpu_page_first_chunk(size_t reserved_size, · line 2131

Quoted 10 lines

mm/percpu.c +2 -2

+		unsigned int cpu = ai->groups[0].cpu_map[unit];
 		for (i = 0; i < unit_pages; i++) {
-			unsigned int cpu = ai->groups[0].cpu_map[unit];
 			void *ptr;
 
 			ptr = alloc_fn(cpu, PAGE_SIZE, PAGE_SIZE);
 			if (!ptr) {
 				pr_warn("failed to allocate %s page for cpu%u\n",
-					psize_str, cpu);
+						psize_str, cpu);

And stop making gratuitous changes?

Thanks.

Signature · 7 lines

tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

zijun_hu

author

2016-10-14 00:08 UTC #5 lore raw

Quoted 10 lines · Tejun Heo

Quoted 9 lines · zijun_hu
From: zijun_hu <[email protected]>
as shown by pcpu_build_alloc_info(), the number of units within a percpu group is educed by rounding up the number of CPUs within the group to @upa boundary, therefore, the number of CPUs isn't equal to the units's if it isn't aligned to @upa normally. however, pcpu_page_first_chunk() uses BUG_ON() to assert one number is equal the other roughly, so a panic is maybe triggered by the BUG_ON() falsely.

Quoted 7 lines

Quoted 3 lines
in order to fix this issue, the number of CPUs is rounded up then compared with units's, the BUG_ON() is replaced by warning and returning error code as well to keep system alive as much as possible.
I really can't decode what the actual issue is here. Can you please give an example of a concrete case?

the right relationship between the number of CPUs @nr_cpus within a percpu group and the number of unites @nr_units within the same group is that @nr_units == roundup(@nr_cpus, @upa);

the process of consideration is shown as follows:

1i 1/4 ? current code segments:

BUG_ON(ai->nr_groups != 1); BUG_ON(ai->groups[0].nr_units != num_possible_cpus());

2i 1/4 ? changes for considering the right relationship between the number of CPUs and units

BUG_ON(ai->nr_groups != 1);
BUG_ON(ai->groups[0].nr_units != roundup(num_possible_cpus(), @upa));

3) replace BUG_ON() by warning and returning error code since it seems BUG_ON() isn't nice as shown by linus recent LKML mail

BUG_ON(ai->nr_groups != 1);
if (ai->groups[0].nr_units != roundup(num_possible_cpus(), @upa))
return -EINVAL;

so 3) is my finial changes;

for the relationship of both numbers : see the reply for andrew thanks

;

Quoted 19 lines

Quoted 6 lines

mm/percpu.c +1 -1

@@ -2113,21 +2120,22 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
 
 	/* allocate pages */
 	j = 0;
-	for (unit = 0; unit < num_possible_cpus(); unit++)
+	for (unit = 0; unit < num_possible_cpus(); unit++) {

mm/percpu.c int __init pcpu_page_first_chunk(size_t reserved_size, · line 2131

Quoted 10 lines

mm/percpu.c +2 -2

+		unsigned int cpu = ai->groups[0].cpu_map[unit];
 		for (i = 0; i < unit_pages; i++) {
-			unsigned int cpu = ai->groups[0].cpu_map[unit];
 			void *ptr;
 
 			ptr = alloc_fn(cpu, PAGE_SIZE, PAGE_SIZE);
 			if (!ptr) {
 				pr_warn("failed to allocate %s page for cpu%u\n",
-					psize_str, cpu);
+						psize_str, cpu);

And stop making gratuitous changes?

Quoted 2 lines

Thanks.

Tejun Heo

maintainer

2016-10-14 00:24 UTC #6 lore raw

Hello,

Quoted 6 lines · zijun_hu

Quoted 3 lines
I really can't decode what the actual issue is here. Can you please give an example of a concrete case?
the right relationship between the number of CPUs @nr_cpus within a percpu group and the number of unites @nr_units within the same group is that @nr_units == roundup(@nr_cpus, @upa);

My question was whether there can be actual hardware configurations where this code can fail and if so what they look like and how they would fail.

Thanks.

Signature · 7 lines

tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

zijun_hu

author

2016-10-14 00:54 UTC #7 lore raw

Quoted 10 lines · Tejun Heo

Hello,
Quoted 6 lines · zijun_hu
Quoted 3 lines
I really can't decode what the actual issue is here. Can you please give an example of a concrete case?
the right relationship between the number of CPUs @nr_cpus within a percpu group and the number of unites @nr_units within the same group is that @nr_units == roundup(@nr_cpus, @upa);

Quoted 6 lines

My question was whether there can be actual hardware configurations where this code can fail and if so what they look like and how they would fail.
Thanks.

this answer is difficult to answer since there are so many hardware configurations moreover, besides hardware configuration, reserved size can contribute to this issue as we known, this interface is developed for various ARCHs to setups percpu areas, so we should not assume more detailed aspects about ARCH. neither hardware config nor reserved size.

i am learning memory management code and find the inconsistency between here and there. the log is similar with a panic triggered by BUG_ON() if the numbers of CPUs isn't aligned to @upa

are you agree the relationship of between CPU and units? what bad effects do this changes results in?

are you sure all hardware configurations and reserved size always make number of CPUs are equal to units? if turei 1/4 ? is it redundant for the consideration in there place.

zijun_hu

author

2016-10-14 00:16 UTC #8 lore raw

In reply to Tejun Heo

Quoted 10 lines · Tejun Heo

Quoted 9 lines · zijun_hu
From: zijun_hu <[email protected]>
as shown by pcpu_build_alloc_info(), the number of units within a percpu group is educed by rounding up the number of CPUs within the group to @upa boundary, therefore, the number of CPUs isn't equal to the units's if it isn't aligned to @upa normally. however, pcpu_page_first_chunk() uses BUG_ON() to assert one number is equal the other roughly, so a panic is maybe triggered by the BUG_ON() falsely.

Quoted 7 lines

Quoted 3 lines
in order to fix this issue, the number of CPUs is rounded up then compared with units's, the BUG_ON() is replaced by warning and returning error code as well to keep system alive as much as possible.
I really can't decode what the actual issue is here. Can you please give an example of a concrete case?

the right relationship between the number of CPUs @nr_cpus within a percpu group and the number of unites @nr_units within the same group is that @nr_units == roundup(@nr_cpus, @upa);

the process of consideration is shown as follows:

1i 1/4 ? current code segments:

BUG_ON(ai->nr_groups != 1); BUG_ON(ai->groups[0].nr_units != num_possible_cpus());

2i 1/4 ? changes for considering the right relationship between the number of CPUs and units

BUG_ON(ai->nr_groups != 1);
BUG_ON(ai->groups[0].nr_units != roundup(num_possible_cpus(), @upa));

3) replace BUG_ON() by warning and returning error code since it seems BUG_ON() isn't nice as shown by linus recent LKML mail

BUG_ON(ai->nr_groups != 1);
if (ai->groups[0].nr_units != roundup(num_possible_cpus(), @upa))
return -EINVAL;

so 3) is my finial changes;
for the relationship of both numbers : see the reply for andrew

Quoted 11 lines

mm/percpu.c +2 -2

@@ -2113,21 +2120,22 @@ int __init pcpu_page_first_chunk(size_t reserved_size,
 
 	/* allocate pages */
 	j = 0;
-	for (unit = 0; unit < num_possible_cpus(); unit++)
+	for (unit = 0; unit < num_possible_cpus(); unit++) {
+		unsigned int cpu = ai->groups[0].cpu_map[unit];
 		for (i = 0; i < unit_pages; i++) {
-			unsigned int cpu = ai->groups[0].cpu_map[unit];
 			void *ptr;

Quoted 8 lines

mm/percpu.c int __init pcpu_page_first_chunk(size_t reserved_size, · line 2131
Quoted 5 lines
mm/percpu.c +1 -1
 			ptr = alloc_fn(cpu, PAGE_SIZE, PAGE_SIZE);
 			if (!ptr) {
 				pr_warn("failed to allocate %s page for cpu%u\n",
-					psize_str, cpu);
+						psize_str, cpu);
And stop making gratuitous changes?

this changes is just for looking nicer instinctively @cpu can be determined in the first outer loop.

Quoted 2 lines

Thanks.