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
