Re: [PATCH] DAMON dbgfs_mk_context() error handling

[SeongJae Park <sj@kernel.org>]

Hi Badari,

On Fri, 12 Aug 2022 18:01:25 +0000 "Pulavarty, Badari" <badari.pulavarty@intel.com> wrote:

> damon dbgfs_mk_context() does not handle error from debugfs_create_dir() correctly.

Let's wrpa the body lines at 75 columns[1].

> 
> For example, if one tries to create a context with existing name, debugfs_create_dir() fails
> with -EEXIST, but dbgfs_mk_context() assumes the call is successful and adds another entry - which
> will cause failures when try to enable the monitor.
> 
> Test case:
> 
> echo "off" >  /sys/kernel/debug/damon/monitor_on
> echo "abc" > /sys/kernel/debug/damon/mk_context
> echo "abc" > /sys/kernel/debug/damon/mk_context
> echo <pid> > /sys/kernel/debug/damon/target_ids
> echo "on" > /sys/kernel/debug/damon/monitor_on  <<< fails to enable monitor
> 
> Signed-off-by: Badari Pulavarty badari.pulavarty@intel.com
> ---
> --- orig/mm/damon/dbgfs.c       2022-08-05 13:35:54.416831666 -0400
> +++ new/mm/damon/dbgfs.c        2022-08-05 13:44:25.121849930 -0400
> @@ -721,6 +721,9 @@ static int dbgfs_mk_context(char *name)
>                 return -ENOENT;
> 
>         new_dir = debugfs_create_dir(name, root);
> +       if (IS_ERR(new_dir)) {
> +               return PTR_ERR(new_dir);
> +       }

Nice finding, thank you!  But, the return value of the debugfs call is
intentionally ignored[2].  How about doing the duplicated name check in
dbgfs_mk_context() itself before the debugfs_create_dir() call?

[1] https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format
[2] https://lore.kernel.org/linux-mm/20210205155902.31102-1-sjpark@amazon.com/


Thanks,
SJ

>         dbgfs_dirs[dbgfs_nr_ctxs] = new_dir;
> 
>         new_ctx = dbgfs_new_ctx();