RE: [PATCH] DAMON dbgfs_mk_context() error handling

[SeongJae Park <sj@kernel.org>]

Hi Badari,


On Mon, 15 Aug 2022 21:51:04 +0000 "Pulavarty, Badari" <badari.pulavarty@intel.com> wrote:

> [-- Attachment #1: Type: text/plain, Size: 350 bytes --]
> 
> Hi SI,
> 
> > 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?

Please don't unwrap wrapped message.  My monitor is not so wide :'(

> >
> > [2] https://lore.kernel.org/linux-mm/20210205155902.31102-1-sjpark@amazon.com/
> 
> How about this?
> 
> Thanks,
> Badari
> 
> 
> 
> [-- Attachment #2: damon-mkcontext-fix.patch --]
> [-- Type: application/octet-stream, Size: 858 bytes --]

Could you please don't send a patch as an attached file of the mail but put it
in the mail body so that we can easily read the patch and comment in line?

> 
> damon dbgfs_mk_context() should check to make sure there is no existing 
> context with the same name. Otherwise, it will cause failure when we 
> enabling 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/abc/target_ids
> echo "on" > /sys/kernel/debug/damon/monitor_on  <<< fails 
> 
> Signed-off-by: Badari Pulavarty <badari.pulavarty@intel.com>
> ---

It would be good to put the changelog of this patch here:
https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format

> --- a/mm/damon/dbgfs.c	2022-08-15 14:27:38.308806431 -0700
> +++ b/mm/damon/dbgfs.c	2022-08-15 14:33:31.661163048 -0700
> @@ -817,6 +817,12 @@
>  	if (!root)
>  		return -ENOENT;
>  
> +	new_dir = debugfs_lookup(name, root);
> +	if (new_dir) {
> +		dput(new_dir);
> +		return -EEXIST;
> +	}
> +

The change looks ok to me at a glance, but the attached file seems not an
appropriate patch.  Could you please repost this as a formal patch as suggested
for a better review?

Thanks,
SJ

>  	new_dir = debugfs_create_dir(name, root);
>  	dbgfs_dirs[dbgfs_nr_ctxs] = new_dir;