linux-kernel - Re: [PATCH v2 2/2] mm/damon/dbgfs: Add adaptive_targets list check before enable monitor

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] [day] [month] [year] [list]

Date:   Wed, 20 Oct 2021 09:22:34 +0800
From:   Xin Hao <xhao@...ux.alibaba.com>
To:     SeongJae Park <sj@...nel.org>
Cc:     sjpark@...zon.de, akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] mm/damon/dbgfs: Add adaptive_targets list check
 before enable monitor_on


On 2021/10/19 下午11:49, SeongJae Park wrote:
> On Tue, 19 Oct 2021 15:42:43 +0000 SeongJae Park <sj@...nel.org> wrote:
>
>> Hello Xin,
>>
>> On Tue, 19 Oct 2021 22:26:04 +0800 Xin Hao <xhao@...ux.alibaba.com> wrote:
>>
>>> When the ctx->adaptive_targets list is empty,
>>> i did some test on monitor_on interface like this.
>> s/i/I/
>>
>>> echo on > /sys/kernel/debug/damon/monitor_on
>>> [  851.988307] damon: kdamond (5390) starts
>> Could you please add four spaces in the beginning of this example terminal
>> outputs?  Also, I think confirming targets are empty like below before writing
>> 'on' could make this example better.
>>
>>      # cat /sys/kernel/debug/damon/target_ids
>>      #
>>
>> or, like below.
>>
>>      # echo > /sys/kernel/debug/damon/target_ids
>>
>>> Though the ctx->adaptive_targets list is empty, but the
>>> kthread_run still be called, and the kdamond.x thread still
>>> be created, this is meaningless.
>>>
>>> So there adds a judgment in 'dbgfs_monitor_on_write',
>>> if the ctx->adaptive_targets list is empty, return -EINVAL.
>>>
>>> Signed-off-by: Xin Hao <xhao@...ux.alibaba.com>
>>> ---
>>>   include/linux/damon.h |  1 +
>>>   mm/damon/core.c       |  5 +++++
>>>   mm/damon/dbgfs.c      | 10 ++++++++--
>>>   3 files changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/linux/damon.h b/include/linux/damon.h
>>> index 715dadd21f7c..06acf73fe9db 100644
>>> --- a/include/linux/damon.h
>>> +++ b/include/linux/damon.h
>>> @@ -316,6 +316,7 @@ void damon_destroy_scheme(struct damos *s);
>>>
>>>   struct damon_target *damon_new_target(unsigned long id);
>>>   void damon_add_target(struct damon_ctx *ctx, struct damon_target *t);
>>> +int damon_target_empty(struct damon_ctx *ctx);
>> Seems 'bool' would be better for the return type?
> Also, I think 'damon_targets_empty()' (s/target/targets/) might be a better
> naming?
Yeah, thanks.
>
>
> Thanks,
> SJ
>
>>>   void damon_free_target(struct damon_target *t);
>>>   void damon_destroy_target(struct damon_target *t);
>>>   unsigned int damon_nr_regions(struct damon_target *t);
>>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>>> index 2f6785737902..c651b49249e5 100644
>>> --- a/mm/damon/core.c
>>> +++ b/mm/damon/core.c
>>> @@ -156,6 +156,11 @@ void damon_add_target(struct damon_ctx *ctx, struct damon_target *t)
>>>   	list_add_tail(&t->list, &ctx->adaptive_targets);
>>>   }
>>>
>>> +int damon_target_empty(struct damon_ctx *ctx)
>>> +{
>>> +	return list_empty(&ctx->adaptive_targets);
>>> +}
>>> +
>>>   static void damon_del_target(struct damon_target *t)
>>>   {
>>>   	list_del(&t->list);
>>> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
>>> index 38188347d8ab..bac42dd358d1 100644
>>> --- a/mm/damon/dbgfs.c
>>> +++ b/mm/damon/dbgfs.c
>>> @@ -865,9 +865,15 @@ static ssize_t dbgfs_monitor_on_write(struct file *file,
>>>   		return -EINVAL;
>>>   	}
>>>
>>> -	if (!strncmp(kbuf, "on", count))
>>> +	if (!strncmp(kbuf, "on", count)) {
>>> +		int i;
>>> +
>>> +		for (i = 0; i < dbgfs_nr_ctxs; i++) {
>>> +			if (damon_target_empty(dbgfs_ctxs[i]))
>>> +				return -EINVAL;
>>> +		}
>>>   		err = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
>>> -	else if (!strncmp(kbuf, "off", count))
>>> +	} else if (!strncmp(kbuf, "off", count))
>>>   		err = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
>>>   	else
>>>   		err = -EINVAL;
>> Please use braces for all above branches:
>>
>> https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces
>>
>>
>> Thanks,
>> SJ
>>
>>> --
>>> 2.31.0

-- 
Best Regards!
Xin Hao