[PATCH] DAMON dbgfs_mk_context() error handling

[PATCH] DAMON dbgfs_mk_context() error handling

[Pulavarty, Badari]

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();

Re: [PATCH] DAMON dbgfs_mk_context() error handling

[SeongJae Park]

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();

RE: [PATCH] DAMON dbgfs_mk_context() error handling

[Pulavarty, Badari]

[-- 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;
 

RE: [PATCH] DAMON dbgfs_mk_context() error handling

[SeongJae Park]

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;

RE: [PATCH] DAMON dbgfs_mk_context() error handling

[Pulavarty, Badari]

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;

RE: [PATCH] DAMON dbgfs_mk_context() error handling

[SeongJae Park]

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;

[PATCH] DAMON dbgfs_mk_context() error handling

[Pulavarty, Badari]

[-- 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 --]