Hi Greg,
On Sat, 20 Aug 2022 19:32:37 +0200 Greg KH gregkh@linuxfoundation.org wrote:
On Fri, Aug 19, 2022 at 02:08:09PM -0700, Andrew Morton wrote:
On Fri, 19 Aug 2022 17:19:30 +0000 SeongJae Park sj@kernel.org wrote:
From: Badari Pulavarty badari.pulavarty@intel.com
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 silently fails but the context is added in the internal data structure. 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 <<< failsThis commit fixes the issue by checking if the name already exist and immediately returning '-EEXIST' in the case.
...
--- a/mm/damon/dbgfs.c +++ b/mm/damon/dbgfs.c @@ -795,7 +795,7 @@ static void dbgfs_destroy_ctx(struct damon_ctx *ctx) */ static int dbgfs_mk_context(char *name) {
- struct dentry *root, **new_dirs, *new_dir;
- struct dentry *root, **new_dirs, *new_dir, *dir; struct damon_ctx **new_ctxs, *new_ctx;
if (damon_nr_running_ctxs()) @@ -817,6 +817,12 @@ static int dbgfs_mk_context(char *name) if (!root) return -ENOENT;
- dir = debugfs_lookup(name, root);
- if (dir) {
dput(dir);return -EEXIST;- }
- new_dir = debugfs_create_dir(name, root); dbgfs_dirs[dbgfs_nr_ctxs] = new_dir;
It would be simpler (and less racy) to check the debugfs_create_dir() return value for IS_ERR()?
Yes, if you _HAVE_ to know if the code works properly (i.e. because your feature totally depends on debugfs like damon does), or you have a potential duplicate name like this, then sure, check the return value and do something based on it.
It's odd enough that you should put a comment above the check just so I don't go and send a patch to delete it later on :)
Thank you for the kind explanation, Greg. I will revise this patch to simply check the return value with a comment noticing it's really needed due to the potential duplicate names.
Thanks, SJ
thanks,
greg k-h