linux-kernel - Re: [PATCH v29 07/13] 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: <20210614064800.14606-1-sjpark@amazon.de>
Date:   Mon, 14 Jun 2021 06:48:00 +0000
From:   SeongJae Park <sj38.park@...il.com>
To:     sieberf@...zon.com
Cc:     sj38.park@...il.com, Jonathan.Cameron@...wei.com, acme@...nel.org,
        akpm@...ux-foundation.org, alexander.shishkin@...ux.intel.com,
        amit@...nel.org, benh@...nel.crashing.org,
        brendanhiggins@...gle.com, corbet@....net, david@...hat.com,
        dwmw@...zon.com, elver@...gle.com, fan.du@...el.com,
        foersleo@...zon.de, greg@...ah.com, gthelen@...gle.com,
        guoju.fgj@...baba-inc.com, linux-damon@...zon.com,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, mgorman@...e.de, minchan@...nel.org,
        mingo@...hat.com, namhyung@...nel.org, peterz@...radead.org,
        riel@...riel.com, rientjes@...gle.com, rostedt@...dmis.org,
        rppt@...nel.org, shakeelb@...gle.com, shuah@...nel.org,
        sjpark@...zon.de, snu@...zon.de, vbabka@...e.cz,
        vdavydov.dev@...il.com, zgf574564920@...il.com
Subject: Re: [PATCH v29 07/13] mm/damon: Implement a debugfs-based user space interface

Thank you for the comments!

On Fri, 11 Jun 2021 20:59:04 +0200 <sieberf@...zon.com> wrote:

> + Returns non-empty string on success, negarive error code otherwise.
> 
> Nit: typo, 'negarive'

Good catch!  I will fix this in the next spin.

> 
> 
> +    if (ctx->kdamond) {
> +        if (targetid_is_pid(ctx))
> +            dbgfs_put_pids(targets, nr_targets);
> +        ret = -EBUSY;
> +        goto unlock_out;
> 
> I don't understand this block, what does EBUSY mean in this context?

That means DAMON is busy because someone already requested running the
monitoring context.  So, the user will be able to wait a little bit and try
later, or try to figure out who requested the monitoring.

> 
> +    if (sscanf(kbuf, "%s", kbuf) != 1) {
> +        kfree(kbuf);
> +        return -EINVAL;
> +    }
> 
> Nit, this diverts from other patterns use elsewhere where error 
> conditions that need to free memory use a goto label.

I want to use 'goto' only if it makes the code shorter and easier to read.  In
this case, this is the only one case that could use the 'goto' statement.  And,
this part would still be two lines like below:

    if (sscanf(kbuf, "%s", kbuf) != 1) {
    	ret = -EINVAL;
    	goto out;
    }

Meanwhile, to use the 'goto', we would need to add the label below, so we will
only add one more line.  Therefore, I think using 'goto' here doesn't really
reduce the lines of code or make it easier to read.

So, if you don't care, I'd like to use current code as is.

If I missed something, please let me know.


Thanks,
SeongJae Park