linux-kernel - Re: [PATCH v19 09/15] mm/damon: Implement a debugfs interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 10 Aug 2020 09:41:43 +0200
From:   SeongJae Park <sjpark@...zon.com>
To:     SeongJae Park <sjpark@...zon.com>
CC:     <akpm@...ux-foundation.org>, <Jonathan.Cameron@...wei.com>,
        <aarcange@...hat.com>, <acme@...nel.org>,
        <alexander.shishkin@...ux.intel.com>, <amit@...nel.org>,
        <benh@...nel.crashing.org>, <brendan.d.gregg@...il.com>,
        <brendanhiggins@...gle.com>, <cai@....pw>,
        <colin.king@...onical.com>, <corbet@....net>, <david@...hat.com>,
        <dwmw@...zon.com>, <fan.du@...el.com>, <foersleo@...zon.de>,
        <gthelen@...gle.com>, <irogers@...gle.com>, <jolsa@...hat.com>,
        <kirill@...temov.name>, <mark.rutland@....com>, <mgorman@...e.de>,
        <minchan@...nel.org>, <mingo@...hat.com>, <namhyung@...nel.org>,
        <peterz@...radead.org>, <rdunlap@...radead.org>,
        <riel@...riel.com>, <rientjes@...gle.com>, <rostedt@...dmis.org>,
        <rppt@...nel.org>, <sblbir@...zon.com>, <shakeelb@...gle.com>,
        <shuah@...nel.org>, <sj38.park@...il.com>, <snu@...zon.de>,
        <vbabka@...e.cz>, <vdavydov.dev@...il.com>,
        <yang.shi@...ux.alibaba.com>, <ying.huang@...el.com>,
        <linux-damon@...zon.com>, <linux-mm@...ck.org>,
        <linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v19 09/15] mm/damon: Implement a debugfs interface

On Tue, 4 Aug 2020 11:14:10 +0200 SeongJae Park <sjpark@...zon.com> wrote:

> From: SeongJae Park <sjpark@...zon.de>
> 
> This commit implements a debugfs interface for DAMON.  It works for the
> virtual address spaces monitoring.
[...]
> +
> +#define targetid_is_pid(ctx)	\
> +	(ctx->target_valid == kdamond_vm_target_valid)
> +
[...]
> +
> +static ssize_t debugfs_target_ids_write(struct file *file,
> +		const char __user *buf, size_t count, loff_t *ppos)
> +{
> +	struct damon_ctx *ctx = &damon_user_ctx;
> +	char *kbuf;
> +	unsigned long *targets;
> +	ssize_t nr_targets;
> +	ssize_t ret = count;
> +	struct damon_target *target;
> +	int i;
> +	int err;
> +
> +	kbuf = user_input_str(buf, count, ppos);
> +	if (IS_ERR(kbuf))
> +		return PTR_ERR(kbuf);
> +
> +	targets = str_to_target_ids(kbuf, ret, &nr_targets);
> +	if (!targets) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	if (targetid_is_pid(ctx)) {
> +		for (i = 0; i < nr_targets; i++)
> +			targets[i] = (unsigned long)find_get_pid(
> +					(int)targets[i]);
> +	}
> +
> +	mutex_lock(&ctx->kdamond_lock);
> +	if (ctx->kdamond) {
> +		ret = -EINVAL;
> +		goto unlock_out;
> +	}
> +
> +	if (targetid_is_pid(ctx)) {
> +		damon_for_each_target(target, ctx)
> +			put_pid((struct pid *)target->id);

If non-pid target ids were set before by the kernel API, this will cause a
problem.  Therefore, the DAMON users should cleanup there target ids properly.
However, I found that this could be easily missed.  Indeed, my new test code
missed the cleanup.  Moreover, it would be hard to do that when concurrent
DAMON users exist.

One straightforward fix would be making 'damon_set_targets()' to remember last
target id type and do 'put_pid()' if the last target id type was pid, instead
of here.  This will work, but make the address space independent part to be
coupled with the dependent part.

Or, we could add another callback for cleanup and let debugfs code to register
a function doing 'put_pid()' and remove of the targets as the callback.  This
approach will allow the address space independent part to be remain
independent.

I will fix this problem with the second approach in the next spin.


Thanks,
SeongJae Park