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 <<< fails
This commit fixes the issue by checking if the name already exist and immediately returning '-EEXIST' in the case.
Fixes: 75c1c2b53c78 ("mm/damon/dbgfs: support multiple contexts") Cc: stable@vger.kernel.org # 5.15.x Signed-off-by: Badari Pulavarty badari.pulavarty@intel.com Signed-off-by: SeongJae Park sj@kernel.org --- Changes from v1 (https://lore.kernel.org/damon/DM6PR11MB3978994F75A4104D714437379C679@DM6PR11...) - 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 second version of the patch based on Badari's final proposal and repost on behalf of Badari.
mm/damon/dbgfs.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c index 51d67c8050dd..364b44063c2f 100644 --- 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;
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()?
On Fri, 19 Aug 2022 14:08:09 -0700 Andrew Morton akpm@linux-foundation.org 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()?
I was merely following Greg's previous advice for ignoring the return value[1] of the function, but I might misunderstanding his intention, so CC-ing Greg. Greg, may I ask your opinion?
[1] https://lore.kernel.org/linux-mm/YB1kZaD%2F7omxXztF@kroah.com/
Thanks, SJ
On Fri, 19 Aug 2022 21:16:31 +0000 SeongJae Park sj@kernel.org wrote:
It would be simpler (and less racy) to check the debugfs_create_dir() return value for IS_ERR()?
I was merely following Greg's previous advice for ignoring the return value[1] of the function, but I might misunderstanding his intention, so CC-ing Greg. Greg, may I ask your opinion?
[1] https://lore.kernel.org/linux-mm/YB1kZaD%2F7omxXztF@kroah.com/
Thing is, the correct functioning of the debugfs interfaces is utterly critical to damon. And that's apart from these memory leak and oops-we-killed-damon issues.
So damon simply cannot ignore the state of its debugfs interfaces and keep going along - because if something goes wrong at the debugfs layer, damon is dead and useless and the machine needs a reboot.
Perhaps this means that damon should not be using debugfs for its interfaces at all. Or it means that the debugfs interfaces are misdesigned. I go with the latter, which, alas, also affirms the former.
From a quick scan it appears that a significant minority (20%?) of drivers are checking the debugfs_create_dir() return value.
On Fri, 19 Aug 2022 15:44:51 -0700 Andrew Morton akpm@linux-foundation.org wrote:
On Fri, 19 Aug 2022 21:16:31 +0000 SeongJae Park sj@kernel.org wrote:
It would be simpler (and less racy) to check the debugfs_create_dir() return value for IS_ERR()?
I was merely following Greg's previous advice for ignoring the return value[1] of the function, but I might misunderstanding his intention, so CC-ing Greg. Greg, may I ask your opinion?
[1] https://lore.kernel.org/linux-mm/YB1kZaD%2F7omxXztF@kroah.com/
Thing is, the correct functioning of the debugfs interfaces is utterly critical to damon. And that's apart from these memory leak and oops-we-killed-damon issues.
So damon simply cannot ignore the state of its debugfs interfaces and keep going along - because if something goes wrong at the debugfs layer, damon is dead and useless and the machine needs a reboot.
Perhaps this means that damon should not be using debugfs for its interfaces at all. Or it means that the debugfs interfaces are misdesigned. I go with the latter, which, alas, also affirms the former.
I'd save my word about the latter, but agreed on the former. Fortunately we already have an alternative (DAMON sysfs interface), and the debugfs interface deprecation plan was announced for a while ago. Not sure if the deprecation will be well as hoped, though.
From a quick scan it appears that a significant minority (20%?) of drivers are checking the debugfs_create_dir() return value.
Maybe partly owing to Greg's previous efforts for removing the checks[1,2]? Anyway, based on the previous discussions, I'd expect Greg might prefer not checking the return code.
Anyway, waiting for his opinion.
[1] https://lore.kernel.org/all/20190122152151.16139-14-gregkh@linuxfoundation.o... [2] https://lore.kernel.org/lkml/20190122152151.16139-7-gregkh@linuxfoundation.o...
Thanks, SJ
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 :)
thanks,
greg k-h
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
On 8/20/22 00:19, SeongJae Park 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.
Meh...
SJ, I have seen most (if not all of) your patches uses descriptive mood instead of imperative. Better say "Fix the issue by checking ...".
linux-stable-mirror@lists.linaro.org
-
Andrew Morton -
Bagas Sanjaya -
Greg KH -
SeongJae Park