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

Message ID 20231019194924.100347-1-sj@kernel.org
Headers
Series avoid divide-by-zero due to max_nr_accesses overflow |

Message

SeongJae Park Oct. 19, 2023, 7:49 p.m. UTC
  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
  

Comments

SeongJae Park Oct. 20, 2023, 5:19 p.m. UTC | #1
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
> 
>
  
Andrew Morton Oct. 20, 2023, 5:30 p.m. UTC | #2
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.
  
SeongJae Park Oct. 20, 2023, 5:31 p.m. UTC | #3
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
>
  
SeongJae Park Oct. 20, 2023, 6:02 p.m. UTC | #4
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