Re: [syzbot] [damon?] divide error in damon_set_attrs

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2023/5/27 2:54, SeongJae Park wrote:
> Hi Kefeng and syzbot,
> 
> On Fri, 26 May 2023 20:59:12 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> 
>>
>>
>> On 2023/5/26 19:51, syzbot wrote:
>>> Hello,
>>>
>>> syzbot found the following issue on:
>>>
>>> HEAD commit:    44c026a73be8 Linux 6.4-rc3
>>> git tree:       upstream
>>> console output: https://syzkaller.appspot.com/x/log.txt?x=13a92b31280000
>>> kernel config:  https://syzkaller.appspot.com/x/.config?x=f389ffdf4e9ba3f0
>>> dashboard link: https://syzkaller.appspot.com/bug?extid=841a46899768ec7bec67
>>> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
>>> userspace arch: i386
>>>
>>> Unfortunately, I don't have any reproducer for this issue yet.
>>>
>>> Downloadable assets:
>>> disk image: https://storage.googleapis.com/syzbot-assets/35f16ee05df7/disk-44c026a7.raw.xz
>>> vmlinux: https://storage.googleapis.com/syzbot-assets/10399498a570/vmlinux-44c026a7.xz
>>> kernel image: https://storage.googleapis.com/syzbot-assets/5c72201ea4ba/bzImage-44c026a7.xz
>>>
>>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>>> Reported-by: syzbot+841a46899768ec7bec67@syzkaller.appspotmail.com
>>>
>>> divide error: 0000 [#1] PREEMPT SMP KASAN
>>> CPU: 1 PID: 13527 Comm: syz-executor.1 Not tainted 6.4.0-rc3-syzkaller #0
>>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/28/2023
>>> RIP: 0010:damon_nr_accesses_to_accesses_bp mm/damon/core.c:491 [inline]
>>> RIP: 0010:damon_nr_accesses_for_new_attrs mm/damon/core.c:497 [inline]
>>> RIP: 0010:damon_update_monitoring_result mm/damon/core.c:506 [inline]
>>> RIP: 0010:damon_update_monitoring_results mm/damon/core.c:534 [inline]
>>> RIP: 0010:damon_set_attrs+0x224/0x460 mm/damon/core.c:555
> 
> Thank you for finding and reporting this bug!
> 
> The code of the problem is as below:
> 
>      /* 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;
>      }
> 
> The problem can happen when 'aggr_interval' is smaller than 'sample_interval',
> because 'max_nr_accesses' becomes zero in the case, and resulting in divide by
> zero.
> 
> Same problem is in damon_accesses_bp_to_nr_accesses().
> 
>>
>> make aggr_interval great than or equal sample_interval?
>>
>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> index d9ef62047bf5..6fe1960f3d6b 100644
>> --- a/mm/damon/core.c
>> +++ b/mm/damon/core.c
>> @@ -525,8 +525,8 @@ static void damon_update_monitoring_results(struct
>> damon_ctx *ctx,
>>
>>           /* if any interval is zero, simply forgive conversion */
>>           if (!old_attrs->sample_interval || !old_attrs->aggr_interval ||
>> -                       !new_attrs->sample_interval ||
>> -                       !new_attrs->aggr_interval)
>> +           !new_attrs->sample_interval || !new_attrs->aggr_interval ||
>> +           new_attrs->aggr_interval < new_attrs->sample_interval)
>>                   return;
> 
> Nice and effective fix!  Nevertheless, I think aggregation interval smaller
> than sample interval is just a wrong input.  How about adding the check in
> damon_set_attrs()'s already existing attributes validation, like below?

Yes, move the check into damon_set_attrs() is better, and it seems that
we could move all the check into it, and drop the old_attrs check in
damon_update_monitoring_results(), what's you option?


diff --git a/mm/damon/core.c b/mm/damon/core.c
index d9ef62047bf5..1647f7f1f708 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -523,12 +523,6 @@ static void damon_update_monitoring_results(struct 
damon_ctx *ctx,
         struct damon_target *t;
         struct damon_region *r;

-       /* if any interval is zero, simply forgive conversion */
-       if (!old_attrs->sample_interval || !old_attrs->aggr_interval ||
-                       !new_attrs->sample_interval ||
-                       !new_attrs->aggr_interval)
-               return;
-
         damon_for_each_target(t, ctx)
                 damon_for_each_region(r, t)
                         damon_update_monitoring_result(
@@ -551,6 +545,10 @@ int damon_set_attrs(struct damon_ctx *ctx, struct 
damon_attrs *attrs)
                 return -EINVAL;
         if (attrs->min_nr_regions > attrs->max_nr_regions)
                 return -EINVAL;
+       if (attrs->sample_interval > attrs->aggr_interval)
+               return -EINVAL;
+       if (!attrs->sample_interval || !attrs->aggr_interval)
+               return -EINVAL;



> 
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -580,6 +580,8 @@ int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs)
>                  return -EINVAL;
>          if (attrs->min_nr_regions > attrs->max_nr_regions)
>                  return -EINVAL;
> +       if (attrs->aggr_interval < attrs->sample_interval)
> +               return -EINVAL;
> 
>          damon_update_monitoring_results(ctx, attrs);
>          ctx->attrs = *attrs;
> 
> Thanks,
> SJ