linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/page-writeback.c: avoid potential division by zero
@ 2020-01-01  9:32 Wen Yang
  2020-01-01 12:39 ` Qian Cai
  0 siblings, 1 reply; 4+ messages in thread
From: Wen Yang @ 2020-01-01  9:32 UTC (permalink / raw)
  To: Andrew Morton; +Cc: xlpang, Wen Yang, linux-mm, linux-kernel

The variables 'min', 'max' and 'bw' are unsigned long and
do_div truncates them to 32 bits, which means it can test
non-zero and be truncated to zero for division.
Fix this issue by using div64_ul() instead.

For the two variables 'numerator' and 'denominator',
though they are declared as long, they should actually be
unsigned long (according to the implementation of
the fprop_fraction_percpu() function).

Signed-off-by: Wen Yang <wenyang@linux.alibaba.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
 mm/page-writeback.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 50055d2..2caf780 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -201,11 +201,11 @@ static void wb_min_max_ratio(struct bdi_writeback *wb,
 	if (this_bw < tot_bw) {
 		if (min) {
 			min *= this_bw;
-			do_div(min, tot_bw);
+			min = div64_ul(min, tot_bw);
 		}
 		if (max < 100) {
 			max *= this_bw;
-			do_div(max, tot_bw);
+			max = div64_ul(max, tot_bw);
 		}
 	}
 
@@ -766,7 +766,7 @@ static unsigned long __wb_calc_thresh(struct dirty_throttle_control *dtc)
 	struct wb_domain *dom = dtc_dom(dtc);
 	unsigned long thresh = dtc->thresh;
 	u64 wb_thresh;
-	long numerator, denominator;
+	unsigned long numerator, denominator;
 	unsigned long wb_min_ratio, wb_max_ratio;
 
 	/*
@@ -777,7 +777,7 @@ static unsigned long __wb_calc_thresh(struct dirty_throttle_control *dtc)
 
 	wb_thresh = (thresh * (100 - bdi_min_ratio)) / 100;
 	wb_thresh *= numerator;
-	do_div(wb_thresh, denominator);
+	wb_thresh = div64_ul(wb_thresh, denominator);
 
 	wb_min_max_ratio(dtc->wb, &wb_min_ratio, &wb_max_ratio);
 
@@ -1102,7 +1102,7 @@ static void wb_update_write_bandwidth(struct bdi_writeback *wb,
 	bw = written - min(written, wb->written_stamp);
 	bw *= HZ;
 	if (unlikely(elapsed > period)) {
-		do_div(bw, elapsed);
+		bw = div64_ul(bw, elapsed);
 		avg = bw;
 		goto out;
 	}
-- 
1.8.3.1



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/page-writeback.c: avoid potential division by zero
  2020-01-01  9:32 [PATCH] mm/page-writeback.c: avoid potential division by zero Wen Yang
@ 2020-01-01 12:39 ` Qian Cai
  2020-01-02  3:57   ` Wen Yang
  0 siblings, 1 reply; 4+ messages in thread
From: Qian Cai @ 2020-01-01 12:39 UTC (permalink / raw)
  To: Wen Yang; +Cc: Andrew Morton, xlpang, linux-mm, linux-kernel



> On Jan 1, 2020, at 4:32 AM, Wen Yang <wenyang@linux.alibaba.com> wrote:
> 
> The variables 'min', 'max' and 'bw' are unsigned long and
> do_div truncates them to 32 bits, which means it can test
> non-zero and be truncated to zero for division.
> Fix this issue by using div64_ul() instead.

How did you find out the issue? If it is caught by compilers, can you paste the original warnings? Also, can you figure out which commit introduced the issue in the first place, so it could be backported to stable if needed?

> 
> For the two variables 'numerator' and 'denominator',
> though they are declared as long, they should actually be
> unsigned long (according to the implementation of
> the fprop_fraction_percpu() function).


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/page-writeback.c: avoid potential division by zero
  2020-01-01 12:39 ` Qian Cai
@ 2020-01-02  3:57   ` Wen Yang
  2020-01-02  4:06     ` Qian Cai
  0 siblings, 1 reply; 4+ messages in thread
From: Wen Yang @ 2020-01-02  3:57 UTC (permalink / raw)
  To: Qian Cai; +Cc: Andrew Morton, xlpang, linux-mm, linux-kernel, julia.lawall



On 2020/1/1 8:39 下午, Qian Cai wrote:
> 
> 
>> On Jan 1, 2020, at 4:32 AM, Wen Yang <wenyang@linux.alibaba.com> wrote:
>>
>> The variables 'min', 'max' and 'bw' are unsigned long and
>> do_div truncates them to 32 bits, which means it can test
>> non-zero and be truncated to zero for division.
>> Fix this issue by using div64_ul() instead.
> 
> How did you find out the issue? If it is caught by compilers, can you paste the original warnings? Also, can you figure out which commit introduced the issue in the first place, so it could be backported to stable if needed?
> 

Thanks for your comments.
There are no compilation warnings here.

We found this issue by following these steps:
We were first inspired by commit b0ab99e7736a ("sched: Fix possible 
divide by zero in avg_atom () calculation"), combined with our recently 
analyzed mm code, we found this suspicious place.

And we also disassembled and confirmed it:

  201                 if (min) {
  202                         min *= this_bw;
  203                         do_div(min, tot_bw);
  204                 }

/usr/src/debug/kernel-4.9.168-016.ali3000/linux-4.9.168-016.ali3000.alios7.x86_64/mm/page-writeback.c: 
201
0xffffffff811c37da <__wb_calc_thresh+234>:      xor    %r10d,%r10d
0xffffffff811c37dd <__wb_calc_thresh+237>:      test   %rax,%rax
0xffffffff811c37e0 <__wb_calc_thresh+240>:      je 
0xffffffff811c3800 <__wb_calc_thresh+272>
/usr/src/debug/kernel-4.9.168-016.ali3000/linux-4.9.168-016.ali3000.alios7.x86_64/mm/page-writeback.c: 
202
0xffffffff811c37e2 <__wb_calc_thresh+242>:      imul   %r8,%rax
/usr/src/debug/kernel-4.9.168-016.ali3000/linux-4.9.168-016.ali3000.alios7.x86_64/mm/page-writeback.c: 
203
0xffffffff811c37e6 <__wb_calc_thresh+246>:      mov    %r9d,%r10d 
    ---> truncates it to 32 bits here

0xffffffff811c37e9 <__wb_calc_thresh+249>:      xor    %edx,%edx
0xffffffff811c37eb <__wb_calc_thresh+251>:      div    %r10
0xffffffff811c37ee <__wb_calc_thresh+254>:      imul   %rbx,%rax
0xffffffff811c37f2 <__wb_calc_thresh+258>:      shr    $0x2,%rax
0xffffffff811c37f6 <__wb_calc_thresh+262>:      mul    %rcx
0xffffffff811c37f9 <__wb_calc_thresh+265>:      shr    $0x2,%rdx
0xffffffff811c37fd <__wb_calc_thresh+269>:      mov    %rdx,%r10

This issue was introduced by commit 693108a8a667 (“writeback: make 
bdi->min/max_ratio handling cgroup writeback aware”).

Finally, we will summarize the above cases and plan to write a general 
coccinelle rule to check for similar problems.


>>
>> For the two variables 'numerator' and 'denominator',
>> though they are declared as long, they should actually be
>> unsigned long (according to the implementation of
>> the fprop_fraction_percpu() function).


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] mm/page-writeback.c: avoid potential division by zero
  2020-01-02  3:57   ` Wen Yang
@ 2020-01-02  4:06     ` Qian Cai
  0 siblings, 0 replies; 4+ messages in thread
From: Qian Cai @ 2020-01-02  4:06 UTC (permalink / raw)
  To: Wen Yang; +Cc: Andrew Morton, xlpang, linux-mm, linux-kernel, julia.lawall



> On Jan 1, 2020, at 10:57 PM, Wen Yang <wenyang@linux.alibaba.com> wrote:
> 
> This issue was introduced by commit 693108a8a667 (“writeback: make bdi->min/max_ratio handling cgroup writeback aware”).

Okay, this needs a Fixes tag then, and Cc the relevant people, i.e., Tejun and Jens at least.

> 
> Finally, we will summarize the above cases and plan to write a general coccinelle rule to check for similar problems.


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-01-02  4:06 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-01  9:32 [PATCH] mm/page-writeback.c: avoid potential division by zero Wen Yang
2020-01-01 12:39 ` Qian Cai
2020-01-02  3:57   ` Wen Yang
2020-01-02  4:06     ` Qian Cai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).