[PATCH 0/5] avoid divide-by-zero due to max_nr_accesses overflow

[PATCH 0/5] avoid divide-by-zero due to max_nr_accesses overflow

[SeongJae Park]

The maximum nr_accesses of given DAMON context can be calculated by
dividing the aggregation interval by the sampling interval.  Some logics
in DAMON uses the maximum nr_accesses as a divisor.  Hence, the value
shouldn't be zero.  Such case is avoided since DAMON avoids setting the
agregation interval as samller than the sampling interval.  However,
since nr_accesses is unsigned int while the intervals are unsigned long,
the maximum nr_accesses could be zero while casting.

Avoid the divide-by-zero by implementing a function that handles the
corner case (first patch), and replaces the vulnerable direct max
nr_accesses calculations (remaining patches).

Note that the patches for the replacements are divided for broken
commits, to make backporting on required tres easier.  Especially, the
last patch is for a patch that not yet merged into the mainline but in
mm tree.

SeongJae Park (5):
  mm/damon: implement a function for max nr_accesses safe calculation
  mm/damon/core: avoid divide-by-zero during monitoring results update
  mm/damon/ops-common: avoid divide-by-zero during region hotness
    calculation
  mm/damon/lru_sort: avoid divide-by-zero in hot threshold calculation
  mm/damon/core: avoid divide-by-zero from pseudo-moving window length
    calculation

 include/linux/damon.h |  7 +++++++
 mm/damon/core.c       | 12 +++---------
 mm/damon/lru_sort.c   |  4 +---
 mm/damon/ops-common.c |  5 ++---
 4 files changed, 13 insertions(+), 15 deletions(-)


base-commit: e845524c56a529768a8793e96304db09134eafdf
-- 
2.34.1

[PATCH 1/5] mm/damon: implement a function for max nr_accesses safe calculation

[SeongJae Park]

The maximum nr_accesses of given DAMON context can be calculated by
dividing the aggregation interval by the sampling interval.  Some logics
in DAMON uses the maximum nr_accesses as a divisor.  Hence, the value
shouldn't be zero.  Such case is avoided since DAMON avoids setting the
agregation interval as samller than the sampling interval.  However,
since nr_accesses is unsigned int while the intervals are unsigned long,
the maximum nr_accesses could be zero while casting.  Implement a
function that handles the corner case.

Note that this commit is not fixing the real issue since this is only
introducing the safe function that will replaces the problematic
divisions.  The replacements will be made by followup commits, to make
backporting on stable series easier.

Fixes: 198f0f4c58b9 ("mm/damon/vaddr,paddr: support pageout prioritization")
Cc: <stable@vger.kernel.org> # 5.16.x
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 include/linux/damon.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 27b995c22497..ab2f17d9926b 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -681,6 +681,13 @@ static inline bool damon_target_has_pid(const struct damon_ctx *ctx)
 	return ctx->ops.id == DAMON_OPS_VADDR || ctx->ops.id == DAMON_OPS_FVADDR;
 }
 
+static inline unsigned int damon_max_nr_accesses(const struct damon_attrs *attrs)
+{
+	/* {aggr,sample}_interval are unsigned long, hence could overflow */
+	return min(attrs->aggr_interval / attrs->sample_interval,
+			(unsigned long)UINT_MAX);
+}
+
 
 int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive);
 int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
-- 
2.34.1

[PATCH 2/5] mm/damon/core: avoid divide-by-zero during monitoring results update

[SeongJae Park]

When monitoring attributes are changed, DAMON updates access rate of the
monitoring results accordingly.  For that, it divides some values by the
maximum nr_accesses.  However, due to the type of the related variables,
simple division-based calculation of the divisor can return zero.  As a
result, divide-by-zero is possible.  Fix it by using
damon_max_nr_accesses(), which handles the case.

Fixes: 2f5bef5a590b ("mm/damon/core: update monitoring results for new monitoring attributes")
Cc: <stable@vger.kernel.org> # 6.3.x
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/core.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 9f4f7c378cf3..e194c8075235 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -500,20 +500,14 @@ static unsigned int damon_age_for_new_attrs(unsigned int age,
 static unsigned int damon_accesses_bp_to_nr_accesses(
 		unsigned int accesses_bp, struct damon_attrs *attrs)
 {
-	unsigned int max_nr_accesses =
-		attrs->aggr_interval / attrs->sample_interval;
-
-	return accesses_bp * max_nr_accesses / 10000;
+	return accesses_bp * damon_max_nr_accesses(attrs) / 10000;
 }
 
 /* convert nr_accesses to access ratio in bp (per 10,000) */
 static unsigned int damon_nr_accesses_to_accesses_bp(
 		unsigned int nr_accesses, struct damon_attrs *attrs)
 {
-	unsigned int max_nr_accesses =
-		attrs->aggr_interval / attrs->sample_interval;
-
-	return nr_accesses * 10000 / max_nr_accesses;
+	return nr_accesses * 10000 / damon_max_nr_accesses(attrs);
 }
 
 static unsigned int damon_nr_accesses_for_new_attrs(unsigned int nr_accesses,
-- 
2.34.1

[PATCH 3/5] mm/damon/ops-common: avoid divide-by-zero during region hotness calculation

[SeongJae Park]

When calculating the hotness of each region for the under-quota regions
prioritization, DAMON divides some values by the maximum nr_accesses.
However, due to the type of the related variables, simple division-based
calculation of the divisor can return zero.  As a result, divide-by-zero
is possible.  Fix it by using damon_max_nr_accesses(), which handles the
case.

Fixes: 198f0f4c58b9 ("mm/damon/vaddr,paddr: support pageout prioritization")
Cc: <stable@vger.kernel.org> # 5.16.x
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/ops-common.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
index ac1c3fa80f98..d25d99cb5f2b 100644
--- a/mm/damon/ops-common.c
+++ b/mm/damon/ops-common.c
@@ -73,7 +73,6 @@ void damon_pmdp_mkold(pmd_t *pmd, struct vm_area_struct *vma, unsigned long addr
 int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
 			struct damos *s)
 {
-	unsigned int max_nr_accesses;
 	int freq_subscore;
 	unsigned int age_in_sec;
 	int age_in_log, age_subscore;
@@ -81,8 +80,8 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
 	unsigned int age_weight = s->quota.weight_age;
 	int hotness;
 
-	max_nr_accesses = c->attrs.aggr_interval / c->attrs.sample_interval;
-	freq_subscore = r->nr_accesses * DAMON_MAX_SUBSCORE / max_nr_accesses;
+	freq_subscore = r->nr_accesses * DAMON_MAX_SUBSCORE /
+		damon_max_nr_accesses(&c->attrs);
 
 	age_in_sec = (unsigned long)r->age * c->attrs.aggr_interval / 1000000;
 	for (age_in_log = 0; age_in_log < DAMON_MAX_AGE_IN_LOG && age_in_sec;
-- 
2.34.1

[PATCH 4/5] mm/damon/lru_sort: avoid divide-by-zero in hot threshold calculation

[SeongJae Park]

When calculating the hotness threshold for lru_prio scheme of
DAMON_LRU_SORT, the module divides some values by the maximum
nr_accesses.  However, due to the type of the related variables, simple
division-based calculation of the divisor can return zero.  As a result,
divide-by-zero is possible.  Fix it by using damon_max_nr_accesses(),
which handles the case.

Fixes: 40e983cca927 ("mm/damon: introduce DAMON-based LRU-lists Sorting")
Cc: <stable@vger.kernel.org> # 6.0.x
Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/lru_sort.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
index 3ecdcc029443..f2e5f9431892 100644
--- a/mm/damon/lru_sort.c
+++ b/mm/damon/lru_sort.c
@@ -195,9 +195,7 @@ static int damon_lru_sort_apply_parameters(void)
 	if (err)
 		return err;
 
-	/* aggr_interval / sample_interval is the maximum nr_accesses */
-	hot_thres = damon_lru_sort_mon_attrs.aggr_interval /
-		damon_lru_sort_mon_attrs.sample_interval *
+	hot_thres = damon_max_nr_accesses(&damon_lru_sort_mon_attrs) *
 		hot_thres_access_freq / 1000;
 	scheme = damon_lru_sort_new_hot_scheme(hot_thres);
 	if (!scheme)
-- 
2.34.1

[PATCH 5/5] mm/damon/core: avoid divide-by-zero from pseudo-moving window length calculation

[SeongJae Park]

When calculating the pseudo-moving access rate, DAMON divides some
values by the maximum nr_accesses.  However, due to the type of the
related variables, simple division-based calculation of the divisor can
return zero.  As a result, divide-by-zero is possible.  Fix it by using
damon_max_nr_accesses(), which handles the case.

Note that this is a fix for a commit that not in the mainline but mm
tree.

Fixes: ace30fb21af5 ("mm/damon/core: use pseudo-moving sum for nr_accesses_bp")
Signed-off-by: SeongJae Park <sj@kernel.org>
---

Note that this is for a patch in mm-stable that not yet merged into the
mainline.

 mm/damon/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index e194c8075235..aa2dc7087cd9 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1665,7 +1665,7 @@ void damon_update_region_access_rate(struct damon_region *r, bool accessed,
 	 * aggr_interval, owing to validation of damon_set_attrs().
 	 */
 	if (attrs->sample_interval)
-		len_window = attrs->aggr_interval / attrs->sample_interval;
+		len_window = damon_max_nr_accesses(attrs);
 	r->nr_accesses_bp = damon_moving_sum(r->nr_accesses_bp,
 			r->last_nr_accesses * 10000, len_window,
 			accessed ? 10000 : 0);
-- 
2.34.1

Re: [PATCH 0/5] avoid divide-by-zero due to max_nr_accesses overflow

[SeongJae Park]

On Thu, 19 Oct 2023 19:49:19 +0000 SeongJae Park <sj@kernel.org> wrote:

> The maximum nr_accesses of given DAMON context can be calculated by
> dividing the aggregation interval by the sampling interval.  Some logics
> in DAMON uses the maximum nr_accesses as a divisor.  Hence, the value
> shouldn't be zero.  Such case is avoided since DAMON avoids setting the
> agregation interval as samller than the sampling interval.  However,
> since nr_accesses is unsigned int while the intervals are unsigned long,
> the maximum nr_accesses could be zero while casting.

Actually, the issue was reported by Jakub, and I didn't add 'Reported-by:' tags
for him.  I sure Andrew could add that on his own, but I want to minimize
Andrew's load, so will send v2 of this patchset.  Andrew, please let me know if
that doesn't help but only increasing your load.


Thanks,
SJ

> 
> Avoid the divide-by-zero by implementing a function that handles the
> corner case (first patch), and replaces the vulnerable direct max
> nr_accesses calculations (remaining patches).
> 
> Note that the patches for the replacements are divided for broken
> commits, to make backporting on required tres easier.  Especially, the
> last patch is for a patch that not yet merged into the mainline but in
> mm tree.
> 
> SeongJae Park (5):
>   mm/damon: implement a function for max nr_accesses safe calculation
>   mm/damon/core: avoid divide-by-zero during monitoring results update
>   mm/damon/ops-common: avoid divide-by-zero during region hotness
>     calculation
>   mm/damon/lru_sort: avoid divide-by-zero in hot threshold calculation
>   mm/damon/core: avoid divide-by-zero from pseudo-moving window length
>     calculation
> 
>  include/linux/damon.h |  7 +++++++
>  mm/damon/core.c       | 12 +++---------
>  mm/damon/lru_sort.c   |  4 +---
>  mm/damon/ops-common.c |  5 ++---
>  4 files changed, 13 insertions(+), 15 deletions(-)
> 
> 
> base-commit: e845524c56a529768a8793e96304db09134eafdf
> -- 
> 2.34.1
> 
> 

Re: [PATCH 0/5] avoid divide-by-zero due to max_nr_accesses overflow

[Andrew Morton]

On Fri, 20 Oct 2023 17:19:01 +0000 SeongJae Park <sj@kernel.org> wrote:

> On Thu, 19 Oct 2023 19:49:19 +0000 SeongJae Park <sj@kernel.org> wrote:
> 
> > The maximum nr_accesses of given DAMON context can be calculated by
> > dividing the aggregation interval by the sampling interval.  Some logics
> > in DAMON uses the maximum nr_accesses as a divisor.  Hence, the value
> > shouldn't be zero.  Such case is avoided since DAMON avoids setting the
> > agregation interval as samller than the sampling interval.  However,
> > since nr_accesses is unsigned int while the intervals are unsigned long,
> > the maximum nr_accesses could be zero while casting.
> 
> Actually, the issue was reported by Jakub, and I didn't add 'Reported-by:' tags
> for him.  I sure Andrew could add that on his own, but I want to minimize
> Andrew's load, so will send v2 of this patchset.  Andrew, please let me know if
> that doesn't help but only increasing your load.

Editing the changelogs is far quicker than updating a patch series ;)

btw, it's now conventional to add a link to the reporter's report.  The
new "Closes:" tag, immediately after the Reported-by:.  But it's not a
big deal.

Re: [PATCH 0/5] avoid divide-by-zero due to max_nr_accesses overflow

[SeongJae Park]

On Fri, 20 Oct 2023 17:19:01 +0000 SeongJae Park <sj@kernel.org> wrote:

> On Thu, 19 Oct 2023 19:49:19 +0000 SeongJae Park <sj@kernel.org> wrote:
> 
> > The maximum nr_accesses of given DAMON context can be calculated by
> > dividing the aggregation interval by the sampling interval.  Some logics
> > in DAMON uses the maximum nr_accesses as a divisor.  Hence, the value
> > shouldn't be zero.  Such case is avoided since DAMON avoids setting the
> > agregation interval as samller than the sampling interval.  However,
> > since nr_accesses is unsigned int while the intervals are unsigned long,
> > the maximum nr_accesses could be zero while casting.
> 
> Actually, the issue was reported by Jakub, and I didn't add 'Reported-by:' tags
> for him.  I sure Andrew could add that on his own, but I want to minimize
> Andrew's load, so will send v2 of this patchset.  Andrew, please let me know if
> that doesn't help but only increasing your load.

So sent the second version[1] with the
"Reported-by: akub Acs <acsjakub@amazon.de>" line, but then I noticed the patch
is already added to mm queue[2].  Somehow the notification mails delivered bit
later than usual.

Sorry for making this noise, Andrew.  Please add
"Reported-by: akub Acs <acsjakub@amazon.de>" to already added patches, or
replace those with the v2 if possible.  Also, please let me know if there's
anything I could help.

[1] https://lore.kernel.org/damon/20231020172317.64192-1-sj@kernel.org/
[2] https://lore.kernel.org/mm-commits/20231020171847.C6EEAC433C7@smtp.kernel.org/


Thanks,
SJ

> 
> 
> Thanks,
> SJ
> 

Re: [PATCH 0/5] avoid divide-by-zero due to max_nr_accesses overflow

[SeongJae Park]

On Fri, 20 Oct 2023 10:30:36 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 20 Oct 2023 17:19:01 +0000 SeongJae Park <sj@kernel.org> wrote:
> 
> > On Thu, 19 Oct 2023 19:49:19 +0000 SeongJae Park <sj@kernel.org> wrote:
> > 
> > > The maximum nr_accesses of given DAMON context can be calculated by
> > > dividing the aggregation interval by the sampling interval.  Some logics
> > > in DAMON uses the maximum nr_accesses as a divisor.  Hence, the value
> > > shouldn't be zero.  Such case is avoided since DAMON avoids setting the
> > > agregation interval as samller than the sampling interval.  However,
> > > since nr_accesses is unsigned int while the intervals are unsigned long,
> > > the maximum nr_accesses could be zero while casting.
> > 
> > Actually, the issue was reported by Jakub, and I didn't add 'Reported-by:' tags
> > for him.  I sure Andrew could add that on his own, but I want to minimize
> > Andrew's load, so will send v2 of this patchset.  Andrew, please let me know if
> > that doesn't help but only increasing your load.
> 
> Editing the changelogs is far quicker than updating a patch series ;)
> 
> btw, it's now conventional to add a link to the reporter's report.  The
> new "Closes:" tag, immediately after the Reported-by:.  But it's not a
> big deal.

Surely it is.  And in this case, the report was made privately, so no available
link.  I should have mentioned this early, sorry.  Anyway, thank you for your
help, Andrew :)


Thanks,
SJ