* [PATCH] DAMON dbgfs_mk_context() error handling
@ 2022-08-12 18:01 Pulavarty, Badari
2022-08-14 16:32 ` SeongJae Park
0 siblings, 1 reply; 7+ messages in thread
From: Pulavarty, Badari @ 2022-08-12 18:01 UTC (permalink / raw)
To: damon; +Cc: linux-mm
damon dbgfs_mk_context() does not handle error from debugfs_create_dir() correctly.
For example, if one tries to create a context with existing name, debugfs_create_dir() fails
with -EEXIST, but dbgfs_mk_context() assumes the call is successful and adds another entry - which
will cause failures when try to enable 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/target_ids
echo "on" > /sys/kernel/debug/damon/monitor_on <<< fails to enable monitor
Signed-off-by: Badari Pulavarty badari.pulavarty@intel.com
---
--- orig/mm/damon/dbgfs.c 2022-08-05 13:35:54.416831666 -0400
+++ new/mm/damon/dbgfs.c 2022-08-05 13:44:25.121849930 -0400
@@ -721,6 +721,9 @@ static int dbgfs_mk_context(char *name)
return -ENOENT;
new_dir = debugfs_create_dir(name, root);
+ if (IS_ERR(new_dir)) {
+ return PTR_ERR(new_dir);
+ }
dbgfs_dirs[dbgfs_nr_ctxs] = new_dir;
new_ctx = dbgfs_new_ctx();
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] DAMON dbgfs_mk_context() error handling
2022-08-12 18:01 [PATCH] DAMON dbgfs_mk_context() error handling Pulavarty, Badari
@ 2022-08-14 16:32 ` SeongJae Park
2022-08-15 21:51 ` Pulavarty, Badari
0 siblings, 1 reply; 7+ messages in thread
From: SeongJae Park @ 2022-08-14 16:32 UTC (permalink / raw)
To: Pulavarty, Badari; +Cc: damon, linux-mm
Hi Badari,
On Fri, 12 Aug 2022 18:01:25 +0000 "Pulavarty, Badari" <badari.pulavarty@intel.com> wrote:
> damon dbgfs_mk_context() does not handle error from debugfs_create_dir() correctly.
Let's wrpa the body lines at 75 columns[1].
>
> For example, if one tries to create a context with existing name, debugfs_create_dir() fails
> with -EEXIST, but dbgfs_mk_context() assumes the call is successful and adds another entry - which
> will cause failures when try to enable 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/target_ids
> echo "on" > /sys/kernel/debug/damon/monitor_on <<< fails to enable monitor
>
> Signed-off-by: Badari Pulavarty badari.pulavarty@intel.com
> ---
> --- orig/mm/damon/dbgfs.c 2022-08-05 13:35:54.416831666 -0400
> +++ new/mm/damon/dbgfs.c 2022-08-05 13:44:25.121849930 -0400
> @@ -721,6 +721,9 @@ static int dbgfs_mk_context(char *name)
> return -ENOENT;
>
> new_dir = debugfs_create_dir(name, root);
> + if (IS_ERR(new_dir)) {
> + return PTR_ERR(new_dir);
> + }
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?
[1] https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format
[2] https://lore.kernel.org/linux-mm/20210205155902.31102-1-sjpark@amazon.com/
Thanks,
SJ
> dbgfs_dirs[dbgfs_nr_ctxs] = new_dir;
>
> new_ctx = dbgfs_new_ctx();
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] DAMON dbgfs_mk_context() error handling
2022-08-14 16:32 ` SeongJae Park
@ 2022-08-15 21:51 ` Pulavarty, Badari
2022-08-15 23:42 ` SeongJae Park
0 siblings, 1 reply; 7+ messages in thread
From: Pulavarty, Badari @ 2022-08-15 21:51 UTC (permalink / raw)
To: SeongJae Park; +Cc: damon, linux-mm
[-- 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?
>
> [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 --]
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>
---
--- 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;
+ }
+
new_dir = debugfs_create_dir(name, root);
dbgfs_dirs[dbgfs_nr_ctxs] = new_dir;
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] DAMON dbgfs_mk_context() error handling
2022-08-15 21:51 ` Pulavarty, Badari
@ 2022-08-15 23:42 ` SeongJae Park
2022-08-15 23:54 ` Pulavarty, Badari
2022-08-16 0:12 ` SeongJae Park
0 siblings, 2 replies; 7+ messages in thread
From: SeongJae Park @ 2022-08-15 23:42 UTC (permalink / raw)
To: Pulavarty, Badari; +Cc: SeongJae Park, damon, linux-mm
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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] DAMON dbgfs_mk_context() error handling
2022-08-15 23:42 ` SeongJae Park
@ 2022-08-15 23:54 ` Pulavarty, Badari
2022-08-16 0:12 ` SeongJae Park
1 sibling, 0 replies; 7+ messages in thread
From: Pulavarty, Badari @ 2022-08-15 23:54 UTC (permalink / raw)
To: SeongJae Park; +Cc: damon, linux-mm
Sorry. I am having e-mail client issues, its messing up patches..
Once I fix them, I will resend the patch.
Thanks,
Badari
-----Original Message-----
From: SeongJae Park <sj@kernel.org>
Sent: Monday, August 15, 2022 4:42 PM
To: Pulavarty, Badari <badari.pulavarty@intel.com>
Cc: SeongJae Park <sj@kernel.org>; damon@lists.linux.dev; linux-mm@kvack.org
Subject: RE: [PATCH] DAMON dbgfs_mk_context() error handling
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@amazo
> > n.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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] DAMON dbgfs_mk_context() error handling
2022-08-15 23:42 ` SeongJae Park
2022-08-15 23:54 ` Pulavarty, Badari
@ 2022-08-16 0:12 ` SeongJae Park
1 sibling, 0 replies; 7+ messages in thread
From: SeongJae Park @ 2022-08-16 0:12 UTC (permalink / raw)
To: SeongJae Park; +Cc: Pulavarty, Badari, damon, linux-mm
On Mon, 15 Aug 2022 23:42:10 +0000 SeongJae Park <sj@kernel.org> wrote:
> 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?
Also, please add 'Fixes:' tag and Cc stable@vger.kernel.org to the patch.
Thanks,
SJ
>
> Thanks,
> SJ
>
> > new_dir = debugfs_create_dir(name, root);
> > dbgfs_dirs[dbgfs_nr_ctxs] = new_dir;
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] DAMON dbgfs_mk_context() error handling
@ 2022-08-12 17:57 Pulavarty, Badari
0 siblings, 0 replies; 7+ messages in thread
From: Pulavarty, Badari @ 2022-08-12 17:57 UTC (permalink / raw)
To: damon; +Cc: linux-mm
[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]
damon dbgfs_mk_context() does not handle error from debugfs_create_dir() correctly.
For example, if one tries to create a context with existing name, debugfs_create_dir() fails
with -EEXIST, but dbgfs_mk_context() assumes the call is successful and adds another entry - which
will cause failures when try to enable 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/target_ids
echo "on" > /sys/kernel/debug/damon/monitor_on <<< fails to enable monitor
Signed-off-by: Badari Pulavarty badari.pulavarty@intel.com<mailto:badari.pulavarty@intel.com>
---
--- orig/mm/damon/dbgfs.c 2022-08-05 13:35:54.416831666 -0400
+++ new/mm/damon/dbgfs.c 2022-08-05 13:44:25.121849930 -0400
@@ -721,6 +721,9 @@ static int dbgfs_mk_context(char *name)
return -ENOENT;
new_dir = debugfs_create_dir(name, root);
+ if (IS_ERR(new_dir)) {
+ return PTR_ERR(new_dir);
+ }
dbgfs_dirs[dbgfs_nr_ctxs] = new_dir;
new_ctx = dbgfs_new_ctx();
[-- Attachment #2: Type: text/html, Size: 4245 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-08-16 0:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-12 18:01 [PATCH] DAMON dbgfs_mk_context() error handling Pulavarty, Badari
2022-08-14 16:32 ` SeongJae Park
2022-08-15 21:51 ` Pulavarty, Badari
2022-08-15 23:42 ` SeongJae Park
2022-08-15 23:54 ` Pulavarty, Badari
2022-08-16 0:12 ` SeongJae Park
-- strict thread matches above, loose matches on Subject: below --
2022-08-12 17:57 Pulavarty, Badari
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).