linux-kernel - Re: [PATCH v24 07/14] mm/damon: Implement a debugfs-based user space 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]

Message-ID: <20210205155902.31102-1-sjpark@amazon.com>
Date:   Fri, 5 Feb 2021 16:59:02 +0100
From:   SeongJae Park <sjpark@...zon.com>
To:     Greg KH <greg@...ah.com>
CC:     SeongJae Park <sjpark@...zon.com>,
        SeongJae Park <sjpark@...zon.de>,
        <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>, <elver@...gle.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>,
        <zgf574564920@...il.com>, <linux-damon@...zon.com>,
        <linux-mm@...ck.org>, <linux-doc@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v24 07/14] mm/damon: Implement a debugfs-based user space interface

On Fri, 5 Feb 2021 16:29:41 +0100 Greg KH <greg@...ah.com> wrote:

> On Thu, Feb 04, 2021 at 04:31:43PM +0100, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@...zon.de>
> > 
> > DAMON is designed to be used by kernel space code such as the memory
> > management subsystems, and therefore it provides only kernel space API.
> > That said, letting the user space control DAMON could provide some
> > benefits to them.  For example, it will allow user space to analyze
> > their specific workloads and make their own special optimizations.
> > 
> > For such cases, this commit implements a simple DAMON application kernel
> > module, namely 'damon-dbgfs', which merely wraps the DAMON api and
> > exports those to the user space via the debugfs.
> > 
> > 'damon-dbgfs' exports three files, ``attrs``, ``target_ids``, and
> > ``monitor_on`` under its debugfs directory, ``<debugfs>/damon/``.
[...]
> > ---
> >  include/linux/damon.h |   3 +
> >  mm/damon/Kconfig      |   9 +
> >  mm/damon/Makefile     |   1 +
> >  mm/damon/core.c       |  47 +++++
> >  mm/damon/dbgfs.c      | 387 ++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 447 insertions(+)
> >  create mode 100644 mm/damon/dbgfs.c
[...]
> > diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> > new file mode 100644
> > index 000000000000..db15380737d1
> > --- /dev/null
> > +++ b/mm/damon/dbgfs.c
[...]
> > +
> > +static int dbgfs_fill_ctx_dir(struct dentry *dir, struct damon_ctx *ctx)
> > +{
> > +	const char * const file_names[] = {"attrs", "target_ids"};
> > +	const struct file_operations *fops[] = {&attrs_fops, &target_ids_fops};
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(file_names); i++) {
> > +		if (!debugfs_create_file(file_names[i], 0600, dir,
> > +					ctx, fops[i])) {
> > +			pr_err("failed to create %s file\n", file_names[i]);
> > +			return -ENOMEM;
> 
> No need to check the return value of this function, just keep going and
> ignore it as there's nothing to do and kernel code should not do
> different things based on the output of any debugfs calls.
> 
> Also, this check is totally wrong and doesn't do what you think it is
> doing...

Ok, I will drop the check.

> 
> > +static int __init __damon_dbgfs_init(void)
> > +{
> > +	struct dentry *dbgfs_root;
> > +	const char * const file_names[] = {"monitor_on"};
> > +	const struct file_operations *fops[] = {&monitor_on_fops};
> > +	int i;
> > +
> > +	dbgfs_root = debugfs_create_dir("damon", NULL);
> > +	if (IS_ERR(dbgfs_root)) {
> > +		pr_err("failed to create the dbgfs dir\n");
> > +		return PTR_ERR(dbgfs_root);
> 
> Again, no need to check anything, just pass the result of a debugfs call
> back into another one just fine.

Ok.

> 
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(file_names); i++) {
> > +		if (!debugfs_create_file(file_names[i], 0600, dbgfs_root,
> > +					NULL, fops[i])) {
> 
> Again, this isn't checking what you think it is, so please don't do it.

Got it.

I will fix those as you suggested in the next version.


Thanks,
SeongJae Park

> 
> thanks,
> 
> greg k-h