Hi Badari,
On Fri, 12 Aug 2022 18:01:25 +0000 "Pulavarty, Badari" <badari.pulavarty@xxxxxxxxx> 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@xxxxxxxxx
> ---
> --- 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@xxxxxxxxxx/
Thanks,
SJ
> dbgfs_dirs[dbgfs_nr_ctxs] = new_dir;
>
> new_ctx = dbgfs_new_ctx();
