divide error in damon_set_attrs

syzbot syzbot 3 participants 8 messages May 26, 2023 – May 27, 2023

Subsystems (2)

Memory management DAMON

lore

Share & tools

lore

syzbot

author syzbot

2023-05-26 11:51 UTC #0 lore raw

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 [email protected]

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
Code: 03 42 0f b6 04 20 84 c0 74 08 3c 03 0f 8e b3 01 00 00 48 8b 43 08 31 d2 41 69 4f 18 10 27 00 00 48 f7 33 31 d2 48 89 c7 89 c8 <f7> f7 31 d2 49 8d 7f 30 89 c1 49 8b 46 08 49 f7 36 0f af c8 b8 59
RSP: 0018:ffffc90011627be8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff888015a88800 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff81e2f0e0 RDI: 0000000000000000
RBP: ffff88802b5ae710 R08: 0000000000000007 R09: 0000000000000000
R10: 0000000000000008 R11: 0000000000000000 R12: dffffc0000000000
R13: ffff88802b5ae700 R14: ffffc90011627c60 R15: ffff88802ef03058
FS:  0000000000000000(0000) GS:ffff8880b9900000(0063) knlGS:00000000f7f1db40
CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
CR2: 00000000f72b2f2b CR3: 00000000295e7000 CR4: 00000000003506e0
DR0: 00000000000017b9 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 dbgfs_attrs_write+0x225/0x280 mm/damon/dbgfs.c:101
 full_proxy_write+0x11c/0x190 fs/debugfs/file.c:236
 vfs_write+0x2ae/0xd50 fs/read_write.c:582
 ksys_write+0x12b/0x250 fs/read_write.c:637
 do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline]
 __do_fast_syscall_32+0x65/0xf0 arch/x86/entry/common.c:178
 do_fast_syscall_32+0x33/0x70 arch/x86/entry/common.c:203
 entry_SYSENTER_compat_after_hwframe+0x70/0x82
RIP: 0023:0xf7f22579
Code: b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 00 00 00 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d b4 26 00 00 00 00 8d b4 26 00 00 00 00
RSP: 002b:00000000f7f1d5cc EFLAGS: 00000296 ORIG_RAX: 0000000000000004
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000020000040
RDX: 0000000000000069 RSI: 0000000000000000 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
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
Code: 03 42 0f b6 04 20 84 c0 74 08 3c 03 0f 8e b3 01 00 00 48 8b 43 08 31 d2 41 69 4f 18 10 27 00 00 48 f7 33 31 d2 48 89 c7 89 c8 <f7> f7 31 d2 49 8d 7f 30 89 c1 49 8b 46 08 49 f7 36 0f af c8 b8 59
RSP: 0018:ffffc90011627be8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff888015a88800 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffffffff81e2f0e0 RDI: 0000000000000000
RBP: ffff88802b5ae710 R08: 0000000000000007 R09: 0000000000000000
R10: 0000000000000008 R11: 0000000000000000 R12: dffffc0000000000
R13: ffff88802b5ae700 R14: ffffc90011627c60 R15: ffff88802ef03058
FS:  0000000000000000(0000) GS:ffff8880b9800000(0063) knlGS:00000000f7f1db40
CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
CR2: 000000002cc30000 CR3: 00000000295e7000 CR4: 00000000003526f0
DR0: 00000000000017b9 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
----------------
Code disassembly (best guess):
   0:	03 42 0f             	add    0xf(%rdx),%eax
   3:	b6 04                	mov    $0x4,%dh
   5:	20 84 c0 74 08 3c 03 	and    %al,0x33c0874(%rax,%rax,8)
   c:	0f 8e b3 01 00 00    	jle    0x1c5
  12:	48 8b 43 08          	mov    0x8(%rbx),%rax
  16:	31 d2                	xor    %edx,%edx
  18:	41 69 4f 18 10 27 00 	imul   $0x2710,0x18(%r15),%ecx
  1f:	00
  20:	48 f7 33             	divq   (%rbx)
  23:	31 d2                	xor    %edx,%edx
  25:	48 89 c7             	mov    %rax,%rdi
  28:	89 c8                	mov    %ecx,%eax
* 2a:	f7 f7                	div    %edi <-- trapping instruction
  2c:	31 d2                	xor    %edx,%edx
  2e:	49 8d 7f 30          	lea    0x30(%r15),%rdi
  32:	89 c1                	mov    %eax,%ecx
  34:	49 8b 46 08          	mov    0x8(%r14),%rax
  38:	49 f7 36             	divq   (%r14)
  3b:	0f af c8             	imul   %eax,%ecx
  3e:	b8                   	.byte 0xb8
  3f:	59                   	pop    %rcx

---
This report is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at [email protected].

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

Kefeng Wang

2023-05-26 12:59 UTC #1 lore raw

Quoted 22 lines · syzbot

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 [email protected]

Quoted 8 lines

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

make aggr_interval great than or equal sample_interval?

mm/damon/core.c +2 -2

index d9ef62047bf5..6fe1960f3d6b 100644
@@ -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;

SeongJae Park

maintainer

2023-05-26 18:54 UTC #2 lore raw

Hi Kefeng and syzbot,

Quoted 25 lines · On Fri, 26 May 2023 20:59:12 +0800 Kefeng Wang <[email protected]>…

Quoted 22 lines · syzbot
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 [email protected]

Quoted 8 lines

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().

Quoted 8 lines

make aggr_interval great than or equal sample_interval?
mm/damon/core.c +0 -0
index d9ef62047bf5..6fe1960f3d6b 100644

mm/damon/core.c static void damon_update_monitoring_results(struct · line 531

Quoted 9 lines

mm/damon/core.c +2 -2

@@ -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?

mm/damon/core.c +2 -0

@@ -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

SeongJae Park

maintainer

2023-05-26 19:35 UTC #3 lore raw

On Fri, 26 May 2023 18:54:09 +0000 SeongJae Park <[email protected]> wrote:

[...]

Quoted 24 lines

Quoted 9 lines

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;
    }

Quoted 5 lines

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().

This is not true, because the function doesn't divide something by 'max_nr_acceses'. I was confused, sorry.

Thanks,
SJ

[...]

Kefeng Wang

2023-05-27 01:15 UTC #4 lore raw

In reply to SeongJae Park

Quoted 68 lines · SeongJae Park

Hi Kefeng and syzbot,
Quoted 25 lines · On Fri, 26 May 2023 20:59:12 +0800 Kefeng Wang <[email protected]>…
Quoted 22 lines · syzbot
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 [email protected]
Quoted 8 lines
Quoted 8 lines
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().
Quoted 10 lines
make aggr_interval great than or equal sample_interval?
mm/damon/core.c +0 -0
index d9ef62047bf5..6fe1960f3d6b 100644
@@ -525,8 +525,8 @@ static void damon_update_monitoring_results(struct
 damon_ctx *ctx,
 

Quoted 11 lines

mm/damon/core.c static void damon_update_monitoring_results(struct · line 531
Quoted 7 lines
mm/damon/core.c +2 -2
          /* 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?

mm/damon/core.c +4 -6

index d9ef62047bf5..1647f7f1f708 100644
@@ -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;

Quoted 15 lines

mm/damon/core.c +2 -0

@@ -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

SeongJae Park

maintainer

2023-05-27 01:46 UTC #5 lore raw

Hi Kefeng,

On Sat, 27 May 2023 09:15:01 +0800 Kefeng Wang <[email protected]> wrote:

[...]

Quoted 6 lines

Quoted 4 lines
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

Thank you for this kind comment!

Quoted 24 lines

, 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?

mm/damon/core.c +0 -6

index d9ef62047bf5..1647f7f1f708 100644
@@ -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(

mm/damon/core.c int damon_set_attrs(struct damon_ctx *ctx, struct · line 552

Quoted 8 lines

mm/damon/core.c +4 -0

@@ -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;

In my humble opinion, the validation for monitoring results and for general monitoring could be different. For example, zero aggreation/sampling intervals might make sense for fixed granularity working set size monitoring. Hence, I'd prefer keeping those checks in the damon_update_monitoring_results().

Thanks,
SJ

[...]

Kefeng Wang

2023-05-27 02:02 UTC #6 lore raw

Quoted 43 lines · SeongJae Park

Hi Kefeng,
On Sat, 27 May 2023 09:15:01 +0800 Kefeng Wang <[email protected]> wrote:
[...]
Quoted 6 lines
Quoted 4 lines
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
Thank you for this kind comment!
Quoted 29 lines
, 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?
mm/damon/core.c +0 -6
index d9ef62047bf5..1647f7f1f708 100644
@@ -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(
mm/damon/core.c +1 -0
@@ -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)

Quoted 8 lines

mm/damon/core.c int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs) · line 551
Quoted 3 lines
mm/damon/core.c +3 -0
+               return -EINVAL;
+       if (!attrs->sample_interval || !attrs->aggr_interval)
+               return -EINVAL;
In my humble opinion, the validation for monitoring results and for general monitoring could be different. For example, zero aggreation/sampling intervals might make sense for fixed granularity working set size monitoring. Hence, I'd prefer keeping those checks in the damon_update_monitoring_results().

ok, will keep that, I check the damon_set_attrs() called by lru_sort/reclaim monitor and sysfs/dbgfs, the above changes should be ok, maybe missing something, the working set size monitoring is not public for now?

Quoted 6 lines

Thanks,
SJ
[...]

SeongJae Park

maintainer

2023-05-27 02:09 UTC #7 lore raw

Quoted 56 lines · On Sat, 27 May 2023 10:02:38 +0800 Kefeng Wang <[email protected]>…

Quoted 43 lines · SeongJae Park
Hi Kefeng,
On Sat, 27 May 2023 09:15:01 +0800 Kefeng Wang <[email protected]> wrote:
[...]
Quoted 6 lines
Quoted 4 lines
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
Thank you for this kind comment!
Quoted 29 lines
, 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?
mm/damon/core.c +0 -6
index d9ef62047bf5..1647f7f1f708 100644
@@ -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(
mm/damon/core.c +1 -0
@@ -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)
Quoted 8 lines
mm/damon/core.c int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs) · line 551
Quoted 3 lines
mm/damon/core.c +3 -0
+               return -EINVAL;
+       if (!attrs->sample_interval || !attrs->aggr_interval)
+               return -EINVAL;
In my humble opinion, the validation for monitoring results and for general monitoring could be different. For example, zero aggreation/sampling intervals might make sense for fixed granularity working set size monitoring. Hence, I'd prefer keeping those checks in the damon_update_monitoring_results().

Quoted 1 line

ok, will keep that,

Thank you for agreeing.

Quoted 4 lines

I check the damon_set_attrs() called by
lru_sort/reclaim monitor and sysfs/dbgfs, the above changes should be ok, maybe missing something, the working set size monitoring is not public for now?

You're correct. Working set size monitoring is not somewhat currently publicly exists, but only possible usage of DAMON at the moment.

Thanks,
SJ

[...]