Re: [PATCH v3] mm/damon/dbgfs: avoid duplicate context directory creation

[Greg KH]

On Sun, Aug 21, 2022 at 06:08:53PM +0000, SeongJae Park wrote:
> From: Badari Pulavarty <badari.pulavarty@xxxxxxxxx>
> 
> From: Badari Pulavarty <badari.pulavarty@xxxxxxxxx>

Twice?

> 
> When user tries to create a DAMON context via the DAMON debugfs
> interface with a name of an already existing context, the context
> directory creation fails but a new context is created and added in the
> internal data structure, due to absence of the directory creation
> success check.  As a result, memory could leak and DAMON cannot be
> turned on.  An example test case is as below:
> 
>     # cd /sys/kernel/debug/damon/
>     # echo "off" >  monitor_on
>     # echo paddr > target_ids
>     # echo "abc" > mk_context
>     # echo "abc" > mk_context
>     # echo $$ > abc/target_ids
>     # echo "on" > monitor_on  <<< fails
> 
> Return value of 'debugfs_create_dir()' is expected to be ignored in
> general, but this is an exceptional case as DAMON feature is depending
> on the debugfs functionality and it has the potential duplicate name
> issue.  This commit therefore fixes the issue by checking the directory
> creation failure and immediately return the error in the case.
> 
> Fixes: 75c1c2b53c78 ("mm/damon/dbgfs: support multiple contexts")
> Cc: <stable@xxxxxxxxxxxxxxx> # 5.15.x
> Signed-off-by: Badari Pulavarty <badari.pulavarty@xxxxxxxxx>
> Signed-off-by: SeongJae Park <sj@xxxxxxxxxx>
> ---
> Changes from v2
> (https://lore.kernel.org/damon/20220819171930.16166-1-sj@xxxxxxxxxx/)
> - Simply check the debugfs_create_dir() return value (Andrew Morton)
> - Put a comment for justifying check of the return value (Greg KH)
> 
> Changes from v1
> (https://lore.kernel.org/damon/DM6PR11MB3978994F75A4104D714437379C679@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/)
> - Manually check duplicate entry instead of checking
>   'debugfs_create_dir()' return value
> - Reword commit message and the test case
> 
> Seems Badari have some email client issue, so I (SJ) am making this
> third version of the patch based on Badari's final proposal and
> reposting on behalf of Badari.
> 
>  mm/damon/dbgfs.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> index 51d67c8050dd..3b55a1b219b5 100644
> --- a/mm/damon/dbgfs.c
> +++ b/mm/damon/dbgfs.c
> @@ -818,6 +818,9 @@ static int dbgfs_mk_context(char *name)
>  		return -ENOENT;
>  
>  	new_dir = debugfs_create_dir(name, root);
> +	/* Below check is required for a potential duplicated name case */
> +	if (IS_ERR(new_dir))
> +		return PTR_ERR(new_dir);

Did you just leak the memory allocated above this check?  It's hard to
determine as you are setting global variables.

thanks,

greg k-h