2021-03-09 09:57:28

by Jia-Ju Bai

Subject: [PATCH] fs: proc: fix error return code of proc_map_files_readdir()

When get_task_mm() returns NULL to mm, no error return code of
proc_map_files_readdir() is assigned.
To fix this bug, ret is assigned with -ENOENT in this case.

Fixes: f0c3b5093add ("[readdir] convert procfs")
Reported-by: TOTE Robot <[email protected]>
Signed-off-by: Jia-Ju Bai <[email protected]>
---
fs/proc/base.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 3851bfcdba56..254cc6ac65fb 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2332,8 +2332,10 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
goto out_put_task;

mm = get_task_mm(task);
- if (!mm)
+ if (!mm) {
+ ret = -ENOENT;
goto out_put_task;
+ }

ret = mmap_read_lock_killable(mm);
if (ret) {
--
2.17.1

2021-03-09 18:32:26

by Eric Biggers

Subject: Re: [PATCH] fs: proc: fix error return code of proc_map_files_readdir()

On Tue, Mar 09, 2021 at 01:55:27AM -0800, Jia-Ju Bai wrote:
> When get_task_mm() returns NULL to mm, no error return code of
> proc_map_files_readdir() is assigned.
> To fix this bug, ret is assigned with -ENOENT in this case.
>
> Fixes: f0c3b5093add ("[readdir] convert procfs")
> Reported-by: TOTE Robot <[email protected]>
> Signed-off-by: Jia-Ju Bai <[email protected]>
> ---
> fs/proc/base.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 3851bfcdba56..254cc6ac65fb 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2332,8 +2332,10 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
> goto out_put_task;
>
> mm = get_task_mm(task);
> - if (!mm)
> + if (!mm) {
> + ret = -ENOENT;
> goto out_put_task;
> + }
>
> ret = mmap_read_lock_killable(mm);

Is there something in particular that makes you think that returning ENOENT is
the correct behavior in this case? Try 'ls /proc/$pid/map_files' where pid is a
kernel thread; it's an empty directory, which is probably intentional. Your
patch would change reading the directory to fail with ENOENT.

- Eric

2021-03-09 19:06:48

by Alexey Dobriyan

Subject: Re: [PATCH] fs: proc: fix error return code of proc_map_files_readdir()

On Tue, Mar 09, 2021 at 10:30:23AM -0800, Eric Biggers wrote:
> On Tue, Mar 09, 2021 at 01:55:27AM -0800, Jia-Ju Bai wrote:
> > When get_task_mm() returns NULL to mm, no error return code of
> > proc_map_files_readdir() is assigned.
> > To fix this bug, ret is assigned with -ENOENT in this case.

> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -2332,8 +2332,10 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
> > goto out_put_task;
> >
> > mm = get_task_mm(task);
> > - if (!mm)
> > + if (!mm) {
> > + ret = -ENOENT;
> > goto out_put_task;
> > + }
> >
> > ret = mmap_read_lock_killable(mm);
>
> Is there something in particular that makes you think that returning ENOENT is
> the correct behavior in this case? Try 'ls /proc/$pid/map_files' where pid is a
> kernel thread; it's an empty directory, which is probably intentional. Your
> patch would change reading the directory to fail with ENOENT.

Yes. 0 from readdir means "no more stuff", not an error.

2021-03-10 01:17:03

by Jia-Ju Bai

Subject: Re: [PATCH] fs: proc: fix error return code of proc_map_files_readdir()

On 2021/3/10 3:05, Alexey Dobriyan wrote:
> On Tue, Mar 09, 2021 at 10:30:23AM -0800, Eric Biggers wrote:
>
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -2332,8 +2332,10 @@ proc_map_files_readdir(struct file *file, struct dir_context *ctx)
>>> goto out_put_task;
>>>
>>> mm = get_task_mm(task);
>>> - if (!mm)
>>> + if (!mm) {
>>> + ret = -ENOENT;
>>> goto out_put_task;
>>> + }
>>>
>>> ret = mmap_read_lock_killable(mm);
>> Is there something in particular that makes you think that returning ENOENT is
>> the correct behavior in this case? Try 'ls /proc/$pid/map_files' where pid is a
>> kernel thread; it's an empty directory, which is probably intentional. Your
>> patch would change reading the directory to fail with ENOENT.
> Yes. 0 from readdir means "no more stuff", not an error.

Thanks for your reply and explanation.
I am sorry for the false report...


Best wishes,
Jia-Ju Bai