The patch titled
Subject: mm/damon/sysfs: update monitoring target regions for online input commit
has been added to the -mm mm-hotfixes-unstable branch. Its filename is
mm-damon-sysfs-update-monitoring-target-regions-for-online-input-commit.patch
This patch will shortly appear at
https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patche…
This patch will later appear in the mm-hotfixes-unstable branch at
git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a
reply-to-all to that, adding suitable additional cc's
*** Remember to use Documentation/process/submit-checklist.rst when testing your code ***
The -mm tree is included into linux-next via the mm-everything
branch at git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
and is updated there every 2-3 working days
------------------------------------------------------
From: SeongJae Park <sj(a)kernel.org>
Subject: mm/damon/sysfs: update monitoring target regions for online input commit
Date: Tue, 31 Oct 2023 17:01:31 +0000
When user input is committed online, DAMON sysfs interface is ignoring the
user input for the monitoring target regions. Such request is valid and
useful for fixed monitoring target regions-based monitoring ops like
'paddr' or 'fvaddr'.
Update the region boundaries as user specified, too. Note that the
monitoring results of the regions that overlap between the latest
monitoring target regions and the new target regions are preserved.
Treat empty monitoring target regions user request as a request to just
make no change to the monitoring target regions. Otherwise, users should
set the monitoring target regions same to current one for every online
input commit, and it could be challenging for dynamic monitoring target
regions update DAMON ops like 'vaddr'. If the user really need to remove
all monitoring target regions, they can simply remove the target and then
create the target again with empty target regions.
Link: https://lkml.kernel.org/r/20231031170131.46972-1-sj@kernel.org
Fixes: da87878010e5 ("mm/damon/sysfs: support online inputs update")
Signed-off-by: SeongJae Park <sj(a)kernel.org>
Cc: <stable(a)vger.kernel.org> [5.19+]
Signed-off-by: Andrew Morton <akpm(a)linux-foundation.org>
---
mm/damon/sysfs.c | 47 ++++++++++++++++++++++++++++-----------------
1 file changed, 30 insertions(+), 17 deletions(-)
--- a/mm/damon/sysfs.c~mm-damon-sysfs-update-monitoring-target-regions-for-online-input-commit
+++ a/mm/damon/sysfs.c
@@ -1150,34 +1150,47 @@ destroy_targets_out:
return err;
}
-static int damon_sysfs_update_target(struct damon_target *target,
- struct damon_ctx *ctx,
- struct damon_sysfs_target *sys_target)
+static int damon_sysfs_update_target_pid(struct damon_target *target, int pid)
{
- struct pid *pid;
- struct damon_region *r, *next;
-
- if (!damon_target_has_pid(ctx))
- return 0;
+ struct pid *pid_new;
- pid = find_get_pid(sys_target->pid);
- if (!pid)
+ pid_new = find_get_pid(pid);
+ if (!pid_new)
return -EINVAL;
- /* no change to the target */
- if (pid == target->pid) {
- put_pid(pid);
+ if (pid_new == target->pid) {
+ put_pid(pid_new);
return 0;
}
- /* remove old monitoring results and update the target's pid */
- damon_for_each_region_safe(r, next, target)
- damon_destroy_region(r, target);
put_pid(target->pid);
- target->pid = pid;
+ target->pid = pid_new;
return 0;
}
+static int damon_sysfs_update_target(struct damon_target *target,
+ struct damon_ctx *ctx,
+ struct damon_sysfs_target *sys_target)
+{
+ int err;
+
+ if (damon_target_has_pid(ctx)) {
+ err = damon_sysfs_update_target_pid(target, sys_target->pid);
+ if (err)
+ return err;
+ }
+
+ /*
+ * Do monitoring target region boundary update only if one or more
+ * regions are set by the user. This is for keeping current monitoring
+ * target results and range easier, especially for dynamic monitoring
+ * target regions update ops like 'vaddr'.
+ */
+ if (sys_target->regions->nr)
+ err = damon_sysfs_set_regions(target, sys_target->regions);
+ return err;
+}
+
static int damon_sysfs_set_targets(struct damon_ctx *ctx,
struct damon_sysfs_targets *sysfs_targets)
{
_
Patches currently in -mm which might be from sj(a)kernel.org are
mm-damon-sysfs-remove-requested-targets-when-online-commit-inputs.patch
mm-damon-sysfs-remove-requested-targets-when-online-commit-inputs-fix.patch
mm-damon-sysfs-update-monitoring-target-regions-for-online-input-commit.patch
[ Adding Masami and stable ]
On Tue, 31 Oct 2023 00:27:07 +0000
Beau Belgrave <beaub(a)linux.microsoft.com> wrote:
> On Mon, Oct 30, 2023 at 05:31:51PM -0400, Steven Rostedt wrote:
> > On Mon, 30 Oct 2023 12:42:23 -0400
> > Steven Rostedt <rostedt(a)goodmis.org> wrote:
> >
> > > > I still get the splat about the trace_array_put when running
> > > > user_event's ftrace selftest:
> > > >
> > > > [ 26.665931] ------------[ cut here ]------------
> > > > [ 26.666663] WARNING: CPU: 12 PID: 291 at kernel/trace/trace.c:516 tracing_release_file_tr+0x46/0x50
> > > > [ 26.667470] Modules linked in:
> > > > [ 26.667808] CPU: 12 PID: 291 Comm: ftrace_test Not tainted 6.6.0-rc7-next-20231026 #3
> > > > [ 26.668665] RIP: 0010:tracing_release_file_tr+0x46/0x50
> > > > [ 26.669093] Code: d1 03 01 8b 83 c0 1e 00 00 85 c0 74 1d 83 e8 01 48 c7 c7 80 5b ef bc 89 83 c0 1e 00 00 e8 f2 b5 03 01 31 c0 5b e9 75 ee 27 01 <0f> 0b eb df 66 0f 1f 44 00 00 90 90 90 90 90 90 90 90 90 90 90 90
> > > > [ 26.670580] RSP: 0018:ffffb6ef858ffee8 EFLAGS: 00010246
> > > > [ 26.671128] RAX: 0000000000000000 RBX: ffff9d7ae2364058 RCX: 0000000000000000
> > > > [ 26.671793] RDX: 0000000000000000 RSI: ffffffffbcb6b38b RDI: 00000000ffffffff
> > > > [ 26.672444] RBP: ffff9d7ac3e72200 R08: 0000000000000000 R09: 0000000000000000
> > > > [ 26.673072] R10: ffffb6ef858ffee8 R11: ffffffffbb28526f R12: 00000000000f801f
> > > > [ 26.673705] R13: ffff9d7b661a2020 R14: ffff9d7ac6057728 R15: 0000000000000000
> > > > [ 26.674339] FS: 00007fa852fa6740(0000) GS:ffff9d81a6300000(0000) knlGS:0000000000000000
> > > > [ 26.674978] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > [ 26.675506] CR2: 00007fa852c2a250 CR3: 0000000105d92001 CR4: 0000000000370eb0
> > > > [ 26.676142] Call Trace:
> > > > [ 26.676357] <TASK>
> > > > [ 26.676572] ? __warn+0x7f/0x160
> > > > [ 26.677092] ? tracing_release_file_tr+0x46/0x50
> > > > [ 26.677540] ? report_bug+0x1c3/0x1d0
> > > > [ 26.677871] ? handle_bug+0x3c/0x70
> > > > [ 26.678196] ? exc_invalid_op+0x14/0x70
> > > > [ 26.678520] ? asm_exc_invalid_op+0x16/0x20
> > > > [ 26.678845] ? tracing_release_file_tr+0x1f/0x50
> > > > [ 26.679268] ? tracing_release_file_tr+0x46/0x50
> > > > [ 26.679691] ? tracing_release_file_tr+0x1f/0x50
> > > > [ 26.680105] __fput+0xab/0x300
> > > > [ 26.680437] __x64_sys_close+0x38/0x80
> > >
> > > Hmm, this doesn't tell me much. Let me go play with the user_event self
> > > tests.
> >
> > I added a bunch of printk()s and I'm thinking there's a race in user event
> > (or dynamic event) code.
> >
>
> I did as well, however, I don't see how user events would be involved
> other than allowing a trace_remove_event_call() with open enable fds?
>
> I believe the scenario is open the enable file and keep the fd open.
>
> While the fd is open to the enable file, call trace_remove_event_call().
>
> If trace_remove_event_call() is called for an event with a tr->ref > 0,
> should it fail or work? (It currently works without issue.)
>
> Should writes to the fd still work after the event it is related to has
> been removed?
>
> I don't see how user_events could prevent this, it seems
> trace_remove_event_call() should fail if files for it are still open?
>
This is a separate issue from eventfs (good, because I think I have solved
all the known bugs for that one - phew!).
Anyway, I checkout the code just before adding the eventfs, and did the following:
# echo 'p:sched schedule' > /sys/kernel/tracing/kprobe_events
# exec 5>>/sys/kernel/tracing/events/kprobes/sched/enable
# > /sys/kernel/tracing/kprobe_events
# exec 5>&-
And it worked fine. The above creates a kprobe event, opens the enable file
of that event with the bash file descriptor #5, removes the kprobe event,
and then closes the file descriptor #5.
But the I applied:
f5ca233e2e66d ("tracing: Increase trace array ref count on enable and filter files")
And do the above commands again and BOOM! it crashes with:
[ 217.879087] BUG: kernel NULL pointer dereference, address: 0000000000000028
[ 217.881121] #PF: supervisor read access in kernel mode
[ 217.882532] #PF: error_code(0x0000) - not-present page
[ 217.883932] PGD 0 P4D 0
[ 217.884672] Oops: 0000 [#1] PREEMPT SMP PTI
[ 217.885821] CPU: 6 PID: 877 Comm: bash Not tainted 6.5.0-rc4-test-00008-g2c6b6b1029d4-dirty #186
[ 217.888178] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 217.890684] RIP: 0010:tracing_release_file_tr+0xc/0x50
[ 217.892097] Code: 5d 41 5c c3 cc cc cc cc 66 0f 1f 44 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa 53 48 8b 87 80 04 00 00 <48> 8b 58 28 48 85 db 74 2d 31 f6 48 c7 c7 c0 b3 1e 93 e8 2d 48 ca
[ 217.897102] RSP: 0018:ffffa5d400587eb0 EFLAGS: 00010282
[ 217.898531] RAX: 0000000000000000 RBX: ffff907d06aa6c00 RCX: 0000000000000000
[ 217.900471] RDX: 0000000000000000 RSI: ffff907d06aa6c00 RDI: ffff907d0bf21bd0
[ 217.902403] RBP: 00000000000d801e R08: 0000000000000001 R09: ffff907d0bf21bd0
[ 217.904350] R10: 0000000000000001 R11: 0000000000000001 R12: ffff907d0bf21bd0
[ 217.906282] R13: ffff907d103708e0 R14: ffff907d0a178c30 R15: 0000000000000000
[ 217.908215] FS: 00007ff49c150740(0000) GS:ffff907e77d00000(0000) knlGS:0000000000000000
[ 217.910405] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 217.911970] CR2: 0000000000000028 CR3: 00000001051ec005 CR4: 0000000000170ee0
[ 217.913924] Call Trace:
[ 217.914624] <TASK>
[ 217.915232] ? __die+0x23/0x70
[ 217.916105] ? page_fault_oops+0x17d/0x4d0
[ 217.917262] ? exc_page_fault+0x7f/0x200
[ 217.918350] ? asm_exc_page_fault+0x26/0x30
[ 217.919513] ? tracing_release_file_tr+0xc/0x50
[ 217.920780] __fput+0xfb/0x2a0
[ 217.921651] task_work_run+0x5d/0x90
[ 217.922652] exit_to_user_mode_prepare+0x231/0x240
[ 217.923981] syscall_exit_to_user_mode+0x1a/0x50
[ 217.925248] do_syscall_64+0x4b/0xc0
[ 217.926176] entry_SYSCALL_64_after_hwframe+0x6e/0xd8
Look familiar?
It's now midnight and I've been at this all day. I'm going to look more at
this tomorrow. It's not going to be easy :-( I'm not sure what exactly to
do. We may need to prevent dynamic events from being deleted if there's any
of its files opened (enable, format, etc).
That is, if you try to delete the event, it will give you an -EBUSY, just
like having them enabled would.
More good news, that commit is in 6.6 *and* marked for stable :-p
-- Steve
When user input is committed online, DAMON sysfs interface is ignoring
the user input for the monitoring target regions. Such request is valid
and useful for fixed monitoring target regions-based monitoring ops like
'paddr' or 'fvaddr'.
Update the region boundaries as user specified, too. Note that the
monitoring results of the regions that overlap between the latest
monitoring target regions and the new target regions are preserved.
Treat empty monitoring target regions user request as a request to just
make no change to the monitoring target regions. Otherwise, users
should set the monitoring target regions same to current one for every
online input commit, and it could be challenging for dynamic monitoring
target regions update DAMON ops like 'vaddr'. If the user really need
to remove all monitoring target regions, they can simply remove the
target and then create the target again with empty target regions.
Fixes: da87878010e5 ("mm/damon/sysfs: support online inputs update")
Cc: <stable(a)vger.kernel.org> # 5.19.x
Signed-off-by: SeongJae Park <sj(a)kernel.org>
---
mm/damon/sysfs.c | 47 ++++++++++++++++++++++++++++++-----------------
1 file changed, 30 insertions(+), 17 deletions(-)
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index 1a231bde18f9..e27846708b5a 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -1150,34 +1150,47 @@ static int damon_sysfs_add_target(struct damon_sysfs_target *sys_target,
return err;
}
-static int damon_sysfs_update_target(struct damon_target *target,
- struct damon_ctx *ctx,
- struct damon_sysfs_target *sys_target)
+static int damon_sysfs_update_target_pid(struct damon_target *target, int pid)
{
- struct pid *pid;
- struct damon_region *r, *next;
-
- if (!damon_target_has_pid(ctx))
- return 0;
+ struct pid *pid_new;
- pid = find_get_pid(sys_target->pid);
- if (!pid)
+ pid_new = find_get_pid(pid);
+ if (!pid_new)
return -EINVAL;
- /* no change to the target */
- if (pid == target->pid) {
- put_pid(pid);
+ if (pid_new == target->pid) {
+ put_pid(pid_new);
return 0;
}
- /* remove old monitoring results and update the target's pid */
- damon_for_each_region_safe(r, next, target)
- damon_destroy_region(r, target);
put_pid(target->pid);
- target->pid = pid;
+ target->pid = pid_new;
return 0;
}
+static int damon_sysfs_update_target(struct damon_target *target,
+ struct damon_ctx *ctx,
+ struct damon_sysfs_target *sys_target)
+{
+ int err;
+
+ if (damon_target_has_pid(ctx)) {
+ err = damon_sysfs_update_target_pid(target, sys_target->pid);
+ if (err)
+ return err;
+ }
+
+ /*
+ * Do monitoring target region boundary update only if one or more
+ * regions are set by the user. This is for keeping current monitoring
+ * target results and range easier, especially for dynamic monitoring
+ * target regions update ops like 'vaddr'.
+ */
+ if (sys_target->regions->nr)
+ err = damon_sysfs_set_regions(target, sys_target->regions);
+ return err;
+}
+
static int damon_sysfs_set_targets(struct damon_ctx *ctx,
struct damon_sysfs_targets *sysfs_targets)
{
--
2.34.1