mm/damon: fix damon_call()-related leak and deadlock

DAMON_SYSFS can leak memory due to improper handling of damon_call() failure. It can also cause a deadlock due to a race between damon_call() and kdamond_fn() termination. Fix those.

SeongJae Park (2):
mm/damon/sysfs: dealloc repeat_call_control if damon_call() fails mm/damon/core: fix damon_call() vs kdamond_fn() exit race deadlock

 include/linux/damon.h |  1 +
 mm/damon/core.c       | 41 ++++++++++-------------------------------
 mm/damon/sysfs.c      |  3 ++-
 3 files changed, 13 insertions(+), 32 deletions(-)

base-commit: 2b3fbc1796d335685d9b7a825c621914a1c97d1d

2.47.3

damon_call() for repeat_call_control of DAMON_SYSFS could fail if somehow the kdamond is stopped before the damon_call(). It could happen, for example, when te damon context was made for monitroing of a virtual address processes, and the process is terminated immediately, before the damon_call() invocation. In the case, the dyanmically allocated repeat_call_control is not deallocated and leaked.

Fix the leak by deallocating the repeat_call_control under the damon_call() failure.

This issue is discovered by sashiko [1].

[1] https://lore.kernel.org/[email protected]

---
 mm/damon/sysfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 6a44a2f3d8fc9..eefa959aa30ae 100644
@@ -1670,7 +1670,8 @@ static int damon_sysfs_turn_damon_on(struct damon_sysfs_kdamond *kdamond)
 	repeat_call_control->data = kdamond;
 	repeat_call_control->repeat = true;
 	repeat_call_control->dealloc_on_cancel = true;
-	damon_call(ctx, repeat_call_control);
+	if (damon_call(ctx, repeat_call_control))
+		kfree(repeat_call_control);
 	return err;
 }
 

2.47.3

When kdamond_fn() main loop is finished, the function cancels all remaining damon_call() requests and unset the damon_ctx->kdamond so that API callers can show the context is terminated. damon_call() adds the caller's request to the queue first. After that, it shows if the kdamond is of the damon_ctx is still running (damon_ctx->kdamond is set). Only if the kdamond is running, damon_call() starts waiting for the kdamond's handling of the newly added request.

The damon_call() requests registration and damon_ctx->kdamond unset are protected by different mutexes, though. Hence, damon_call() could race with damon_ctx->kdamond unset, and results in deadlocks.

For example, let's suppose kdamond successfully finished the damon_call() requests cancelling. Right after that, damon_call() is called for the context. It registers the new request, and show the context is still running, because damon_ctx->kdamond unset is not yet done. Hence the damon_call() caller starts waiting for the handling of the request. However, the kdamond is already on the termination steps, so never handles the new request. As a result, the damon_call() caller threads infinitely waits.

Fix this by introducing another damon_ctx field, namely call_controls_obsolete. It is protected by the damon_ctx->call_controls_lock, which protects damon_call() registration. Initialize (unset) it in kdamond_init_ctx() and set just before the cancelling of remaining damon_call() is executed. damon_call() reads the obsolete field under the lock and avoid adding a new request.

After this change, only requests that guaranteed to be handled or cancelled are registered. Hence the after-registration DAMON context termination check is no more needed. Remove it together.

The issue is found by sashiko [1].

[1] https://lore.kernel.org/[email protected]

---
 include/linux/damon.h |  1 +
 mm/damon/core.c       | 41 ++++++++++-------------------------------
 2 files changed, 11 insertions(+), 31 deletions(-)

index d9a3babbafc16..5129de70e7b70 100644
@@ -818,6 +818,7 @@ struct damon_ctx {
 
 	/* lists of &struct damon_call_control */
 	struct list_head call_controls;
+	bool call_controls_obsolete;
 	struct mutex call_controls_lock;
 
 	struct damos_walk_control *walk_control;

2.47.3

Forwarding Sashiko review for doing discussions via mails.

# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit b886bf00a72b1462f29533c91c431cd2ba3fa3a6
Author: SeongJae Park <[email protected]>
mm/damon/core: fix damon_call() vs kdamond_fn() exit race deadlock

This commit introduces a call_controls_obsolete flag to protect damon_call() from racing with the kdamond termination sequence. This replaces the previous after-registration damon_is_running() check.

[ ... ]

[ ... ]

        complete(&ctx->kdamond_started);
        kdamond_init_ctx(ctx);

# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/[email protected]
#
# hkml [1] generated a draft of this mail.  It can be regenerated
# using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             [email protected]
#
# [1] https://github.com/sjp38/hackermail

Yes. But it is clearly wrong usage of this function. I will add a comment clarifying this, e.g., "this function shouldn't be called for unstarted DAMON context. In the case, it could be indefinitely sleep."

Good catch, I will do the call_controls_obsolete unset before the complete() call.

Yes. I'm working on it for another patch.

Thanks,
SJ

[...]

Forwarding sashiko.dev review status for this thread.

# review url: https://sashiko.dev/#/patchset/[email protected]

- [RFC PATCH 1/2] mm/damon/sysfs: dealloc repeat_call_control if damon_call() fails
- status: Reviewed
- review: No issues found.
- [RFC PATCH 2/2] mm/damon/core: fix damon_call() vs kdamond_fn() exit race deadlock
- status: Reviewed
- review: ISSUES MAY FOUND

# hkml [1] generated a draft of this mail.  It can be regenerated
# using below command:
#
#     hkml patch sashiko_dev --thread_status --for_forwarding \
#             [email protected]
#
# [1] https://github.com/sjp38/hackermail

Sent using hkml (https://github.com/sjp38/hackermail)

The patches are hotfixes, and have no reason to be one series.

I will send the first patch as an individual one without RFC, soon.

For the second patch, I will go one more RFC round.

Thanks,
SJ

[...]