EP-F6AA0618C49C4AEDA73BFF1B39950BAB
Hi,
From: Maninder Singh <[email protected]>
Subject: [PATCH 1/1] kernel/exit.c : Fix missing task_unlock
This patch adds missing read_unlock if do_wait_thread or ptrace_do_wait
returns non zero.
Signed-off-by: Maninder Singh <[email protected]>
Signed-off-by: Vaneet Narang <[email protected]>
Reviewd-by: Akhilesh Kumar <[email protected]>
---
kernel/exit.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index 22fcc05..31a061f 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1486,12 +1486,16 @@ repeat:
tsk = current;
do {
retval = do_wait_thread(wo, tsk);
- if (retval)
+ if (retval) {
+ read_unlock(&tasklist_lock);
goto end;
+ }
retval = ptrace_do_wait(wo, tsk);
- if (retval)
+ if (retval) {
+ read_unlock(&tasklist_lock);
goto end;
+ }
if (wo->wo_flags & __WNOTHREAD)
break;
--
1.7.1
Thanks
Maninder Singh????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
* Maninder Singh <[email protected]> wrote:
> EP-F6AA0618C49C4AEDA73BFF1B39950BAB
> Hi,
>
> From: Maninder Singh <[email protected]>
>
> Subject: [PATCH 1/1] kernel/exit.c : Fix missing task_unlock
>
> This patch adds missing read_unlock if do_wait_thread or ptrace_do_wait
> returns non zero.
>
> Signed-off-by: Maninder Singh <[email protected]>
> Signed-off-by: Vaneet Narang <[email protected]>
> Reviewd-by: Akhilesh Kumar <[email protected]>
> ---
> kernel/exit.c | 8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 22fcc05..31a061f 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1486,12 +1486,16 @@ repeat:
> tsk = current;
> do {
> retval = do_wait_thread(wo, tsk);
> - if (retval)
> + if (retval) {
> + read_unlock(&tasklist_lock);
> goto end;
> + }
>
> retval = ptrace_do_wait(wo, tsk);
> - if (retval)
> + if (retval) {
> + read_unlock(&tasklist_lock);
> goto end;
> + }
>
> if (wo->wo_flags & __WNOTHREAD)
> break;
That's surprising and the changelog is lacking.
So the last time that code was touched upstream was 7 years ago:
commit 64a16caf5e3417ee32f670debcb5857b02a9e08e
Author: Oleg Nesterov <[email protected]>
Date: Wed Jun 17 16:27:40 2009 -0700
do_wait: simplify retval/tsk_result/notask_error mess
please explain whether what you fix is:
1) an ancient bug that somehow nobody ever triggered (plus analysis
of why it wasn't triggered)
2) a new bug introduced by commit XYZ (plus analysis)
3) something else
Thanks,
Ingo
EP-F6AA0618C49C4AEDA73BFF1B39950BAB
>> Hi,
>>
>> From: Maninder Singh <[email protected]>
>>
>> Subject: [PATCH 1/1] kernel/exit.c : Fix missing task_unlock
>>
Subject: [PATCH 1/1] kernel/exit.c : Fix missing read_unlock
>> This patch adds missing read_unlock if do_wait_thread or ptrace_do_wait
>> returns non zero.
Reported By Prevent Under Missing unlock category(program hangs):-
missing_unlock: returning without unlocking tasklist_lock
>>
>> Signed-off-by: Maninder Singh <[email protected]>
>> Signed-off-by: Vaneet Narang <[email protected]>
>> Reviewd-by: Akhilesh Kumar <[email protected]>
>> ---
>> kernel/exit.c | 8 ++++++--
>> 1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/exit.c b/kernel/exit.c
>> index 22fcc05..31a061f 100644
>> --- a/kernel/exit.c
>> +++ b/kernel/exit.c
>> @@ -1486,12 +1486,16 @@ repeat:
>> tsk = current;
>> do {
>> retval = do_wait_thread(wo, tsk);
>> - if (retval)
>> + if (retval) {
>> + read_unlock(&tasklist_lock);
>> goto end;
>> + }
>>
>> retval = ptrace_do_wait(wo, tsk);
>> - if (retval)
>> + if (retval) {
>> + read_unlock(&tasklist_lock);
>> goto end;
>> + }
>>
>> if (wo->wo_flags & __WNOTHREAD)
>> break;
>
>That's surprising and the changelog is lacking.
>So the last time that code was touched upstream was 7 years ago:
> commit 64a16caf5e3417ee32f670debcb5857b02a9e08e
> Author: Oleg Nesterov <[email protected]>
> Date: Wed Jun 17 16:27:40 2009 -0700
> do_wait: simplify retval/tsk_result/notask_error mess
>please explain whether what you fix is:
> 1) an ancient bug that somehow nobody ever triggered (plus analysis
> of why it wasn't triggered)
> 2) a new bug introduced by commit XYZ (plus analysis)
> 3) something else
This issue is reported by Prevent Under category Missing Unlock, So we think it should be reported to maintainers.
>Thanks,
> Ingo
Thanks.????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Thu, May 21, 2015 at 8:03 AM, Ingo Molnar <[email protected]> wrote:
>
> * Maninder Singh <[email protected]> wrote:
>
>> EP-F6AA0618C49C4AEDA73BFF1B39950BAB
>> Hi,
>>
>> From: Maninder Singh <[email protected]>
>>
>> Subject: [PATCH 1/1] kernel/exit.c : Fix missing task_unlock
>>
>> This patch adds missing read_unlock if do_wait_thread or ptrace_do_wait
>> returns non zero.
>>
>> Signed-off-by: Maninder Singh <[email protected]>
>> Signed-off-by: Vaneet Narang <[email protected]>
>> Reviewd-by: Akhilesh Kumar <[email protected]>
>> ---
>> kernel/exit.c | 8 ++++++--
>> 1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/exit.c b/kernel/exit.c
>> index 22fcc05..31a061f 100644
>> --- a/kernel/exit.c
>> +++ b/kernel/exit.c
>> @@ -1486,12 +1486,16 @@ repeat:
>> tsk = current;
>> do {
>> retval = do_wait_thread(wo, tsk);
>> - if (retval)
>> + if (retval) {
>> + read_unlock(&tasklist_lock);
>> goto end;
>> + }
>>
>> retval = ptrace_do_wait(wo, tsk);
>> - if (retval)
>> + if (retval) {
>> + read_unlock(&tasklist_lock);
>> goto end;
>> + }
>>
>> if (wo->wo_flags & __WNOTHREAD)
>> break;
>
> That's surprising <snip>
Still it looks like it is a legitimate change. I don't see where the
unlock would be done otherwise.
I do wonder if this would look nicer if the whole locked part would be
pulled out into a separate (inline) function. That would render the
repeated read_unlock()s unnecessary and possibly also prevent a
goto/label mess if that were to be attempted in-line.
Frans
* Frans Klaver <[email protected]> wrote:
> On Thu, May 21, 2015 at 8:03 AM, Ingo Molnar <[email protected]> wrote:
> >
> > * Maninder Singh <[email protected]> wrote:
> >
> >> EP-F6AA0618C49C4AEDA73BFF1B39950BAB
> >> Hi,
> >>
> >> From: Maninder Singh <[email protected]>
> >>
> >> Subject: [PATCH 1/1] kernel/exit.c : Fix missing task_unlock
> >>
> >> This patch adds missing read_unlock if do_wait_thread or ptrace_do_wait
> >> returns non zero.
> >>
> >> Signed-off-by: Maninder Singh <[email protected]>
> >> Signed-off-by: Vaneet Narang <[email protected]>
> >> Reviewd-by: Akhilesh Kumar <[email protected]>
> >> ---
> >> kernel/exit.c | 8 ++++++--
> >> 1 files changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/exit.c b/kernel/exit.c
> >> index 22fcc05..31a061f 100644
> >> --- a/kernel/exit.c
> >> +++ b/kernel/exit.c
> >> @@ -1486,12 +1486,16 @@ repeat:
> >> tsk = current;
> >> do {
> >> retval = do_wait_thread(wo, tsk);
> >> - if (retval)
> >> + if (retval) {
> >> + read_unlock(&tasklist_lock);
> >> goto end;
> >> + }
> >>
> >> retval = ptrace_do_wait(wo, tsk);
> >> - if (retval)
> >> + if (retval) {
> >> + read_unlock(&tasklist_lock);
> >> goto end;
> >> + }
> >>
> >> if (wo->wo_flags & __WNOTHREAD)
> >> break;
> >
> > That's surprising <snip>
>
> Still it looks like it is a legitimate change. I don't see where the
> unlock would be done otherwise.
No, it does not look like a legitimate change, that's why I asked the
questions. I think this patch breaks the kernel badly.
As it is explained in the comments as well, the various wait-loop
functions (do_wait_thread(), ptrace_do_wait()) fundamentally unlock
the tasklist_lock if they return an error.
NAK.
Thanks,
Ingo
* Maninder Singh <[email protected]> wrote:
> EP-F6AA0618C49C4AEDA73BFF1B39950BAB
> >> Hi,
> >>
> >> From: Maninder Singh <[email protected]>
> >>
> >> Subject: [PATCH 1/1] kernel/exit.c : Fix missing task_unlock
> >>
> Subject: [PATCH 1/1] kernel/exit.c : Fix missing read_unlock
>
> >> This patch adds missing read_unlock if do_wait_thread or ptrace_do_wait
> >> returns non zero.
>
> Reported By Prevent Under Missing unlock category(program hangs):-
> missing_unlock: returning without unlocking tasklist_lock
> >>
> >> Signed-off-by: Maninder Singh <[email protected]>
> >> Signed-off-by: Vaneet Narang <[email protected]>
> >> Reviewd-by: Akhilesh Kumar <[email protected]>
> >> ---
> >> kernel/exit.c | 8 ++++++--
> >> 1 files changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/exit.c b/kernel/exit.c
> >> index 22fcc05..31a061f 100644
> >> --- a/kernel/exit.c
> >> +++ b/kernel/exit.c
> >> @@ -1486,12 +1486,16 @@ repeat:
> >> tsk = current;
> >> do {
> >> retval = do_wait_thread(wo, tsk);
> >> - if (retval)
> >> + if (retval) {
> >> + read_unlock(&tasklist_lock);
> >> goto end;
> >> + }
> >>
> >> retval = ptrace_do_wait(wo, tsk);
> >> - if (retval)
> >> + if (retval) {
> >> + read_unlock(&tasklist_lock);
> >> goto end;
> >> + }
> >>
> >> if (wo->wo_flags & __WNOTHREAD)
> >> break;
> >
> >That's surprising and the changelog is lacking.
>
> >So the last time that code was touched upstream was 7 years ago:
>
> > commit 64a16caf5e3417ee32f670debcb5857b02a9e08e
> > Author: Oleg Nesterov <[email protected]>
> > Date: Wed Jun 17 16:27:40 2009 -0700
>
> > do_wait: simplify retval/tsk_result/notask_error mess
>
> >please explain whether what you fix is:
>
> > 1) an ancient bug that somehow nobody ever triggered (plus analysis
> > of why it wasn't triggered)
>
> > 2) a new bug introduced by commit XYZ (plus analysis)
>
> > 3) something else
>
> This issue is reported by Prevent Under category Missing Unlock, So
> we think it should be reported to maintainers.
Huh? In what way does your reply answer my questions?
Your patch is breaking the kernel, and badly so.
Thanks,
Ingo
On 21 May 2015 12:56:22 CEST, Ingo Molnar <[email protected]> wrote:
>
>* Frans Klaver <[email protected]> wrote:
>
>> On Thu, May 21, 2015 at 8:03 AM, Ingo Molnar <[email protected]>
>wrote:
>> >
>> > * Maninder Singh <[email protected]> wrote:
>> >
>> >> EP-F6AA0618C49C4AEDA73BFF1B39950BAB
>> >> Hi,
>> >>
>> >> From: Maninder Singh <[email protected]>
>> >>
>> >> Subject: [PATCH 1/1] kernel/exit.c : Fix missing task_unlock
>> >>
>> >> This patch adds missing read_unlock if do_wait_thread or
>ptrace_do_wait
>> >> returns non zero.
>> >>
>> >> Signed-off-by: Maninder Singh <[email protected]>
>> >> Signed-off-by: Vaneet Narang <[email protected]>
>> >> Reviewd-by: Akhilesh Kumar <[email protected]>
>> >> ---
>> >> kernel/exit.c | 8 ++++++--
>> >> 1 files changed, 6 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/kernel/exit.c b/kernel/exit.c
>> >> index 22fcc05..31a061f 100644
>> >> --- a/kernel/exit.c
>> >> +++ b/kernel/exit.c
>> >> @@ -1486,12 +1486,16 @@ repeat:
>> >> tsk = current;
>> >> do {
>> >> retval = do_wait_thread(wo, tsk);
>> >> - if (retval)
>> >> + if (retval) {
>> >> + read_unlock(&tasklist_lock);
>> >> goto end;
>> >> + }
>> >>
>> >> retval = ptrace_do_wait(wo, tsk);
>> >> - if (retval)
>> >> + if (retval) {
>> >> + read_unlock(&tasklist_lock);
>> >> goto end;
>> >> + }
>> >>
>> >> if (wo->wo_flags & __WNOTHREAD)
>> >> break;
>> >
>> > That's surprising <snip>
>>
>> Still it looks like it is a legitimate change. I don't see where the
>> unlock would be done otherwise.
>
>No, it does not look like a legitimate change, that's why I asked the
>questions. I think this patch breaks the kernel badly.
>
>As it is explained in the comments as well, the various wait-loop
>functions (do_wait_thread(), ptrace_do_wait()) fundamentally unlock
>the tasklist_lock if they return an error.
Ah, right. Given that I agree. I can imagine a static checker not
seeing that either. Sorry for the noise here.
Thanks,
Frans
EP-F6AA0618C49C4AEDA73BFF1B39950BAB
Hi Ingo,
>> >> Hi,
>> >>
>> >> From: Maninder Singh <[email protected]>
>> >>
>> >> Subject: [PATCH 1/1] kernel/exit.c : Fix missing task_unlock
>> >>
>> Subject: [PATCH 1/1] kernel/exit.c : Fix missing read_unlock
>>
>> >> This patch adds missing read_unlock if do_wait_thread or ptrace_do_wait
>> >> returns non zero.
>>
>> Reported By Prevent Under Missing unlock category(program hangs):-
>> missing_unlock: returning without unlocking tasklist_lock
>> >>
>> >> Signed-off-by: Maninder Singh <[email protected]>
>> >> Signed-off-by: Vaneet Narang <[email protected]>
>> >> Reviewd-by: Akhilesh Kumar <[email protected]>
>> >> ---
>> >> kernel/exit.c | 8 ++++++--
>> >> 1 files changed, 6 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/kernel/exit.c b/kernel/exit.c
>> >> index 22fcc05..31a061f 100644
>> >> --- a/kernel/exit.c
>> >> +++ b/kernel/exit.c
>> >> @@ -1486,12 +1486,16 @@ repeat:
>> >> tsk = current;
>> >> do {
>> >> retval = do_wait_thread(wo, tsk);
>> >> - if (retval)
>> >> + if (retval) {
>> >> + read_unlock(&tasklist_lock);
>> >> goto end;
>> >> + }
>> >>
>> >> retval = ptrace_do_wait(wo, tsk);
>> >> - if (retval)
>> >> + if (retval) {
>> >> + read_unlock(&tasklist_lock);
>> >> goto end;
>> >> + }
>> >>
>> >> if (wo->wo_flags & __WNOTHREAD)
>> >> break;
>> >
>> >That's surprising and the changelog is lacking.
>>
>> >So the last time that code was touched upstream was 7 years ago:
>>
>> > commit 64a16caf5e3417ee32f670debcb5857b02a9e08e
>> > Author: Oleg Nesterov <[email protected]>
>> > Date: Wed Jun 17 16:27:40 2009 -0700
>>
>> > do_wait: simplify retval/tsk_result/notask_error mess
>>
>> >please explain whether what you fix is:
>>
>> > 1) an ancient bug that somehow nobody ever triggered (plus analysis
>> > of why it wasn't triggered)
>>
>> > 2) a new bug introduced by commit XYZ (plus analysis)
>>
>> > 3) something else
>>
>> This issue is reported by Prevent Under category Missing Unlock, So
>> we think it should be reported to maintainers.
>Huh? In what way does your reply answer my questions?
we sent this fix because we were doing static analysis of kernel code by tool coverity analyzer (Prevent), and it shows this unlock mismatch,
Thats why we think to report this whether it is right or not .
>Your patch is breaking the kernel, and badly so.
Sorry for the noise .
>Thanks,
> Ingo
Thanks????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 05/21, Maninder Singh wrote:
>
> EP-F6AA0618C49C4AEDA73BFF1B39950BAB
> Hi,
>
> From: Maninder Singh <[email protected]>
>
> Subject: [PATCH 1/1] kernel/exit.c : Fix missing task_unlock
>
> This patch adds missing read_unlock if do_wait_thread or ptrace_do_wait
> returns non zero.
Confused...
wait_consider_task() should drop tasklist_lock if it returns non-zero?
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -1486,12 +1486,16 @@ repeat:
> tsk = current;
> do {
> retval = do_wait_thread(wo, tsk);
> - if (retval)
> + if (retval) {
> + read_unlock(&tasklist_lock);
> goto end;
> + }
>
> retval = ptrace_do_wait(wo, tsk);
> - if (retval)
> + if (retval) {
> + read_unlock(&tasklist_lock);
> goto end;
> + }
Well, the patch is obviously wrong. Because, again, tasklist_lock was
already unlocked if (say) wait_task_zombie() reaps a child.
If you think there is a case which forgets to unlock, please tell us
more.
Oleg.
EP-F6AA0618C49C4AEDA73BFF1B39950BAB
Hi Oleg,
>> Hi,
>>
>> From: Maninder Singh <[email protected]>
>>
>> Subject: [PATCH 1/1] kernel/exit.c : Fix missing task_unlock
>>
>> This patch adds missing read_unlock if do_wait_thread or ptrace_do_wait
>> returns non zero.
>
>Confused...
>
>wait_consider_task() should drop tasklist_lock if it returns non-zero?
>
>
>> --- a/kernel/exit.c
>> +++ b/kernel/exit.c
>> @@ -1486,12 +1486,16 @@ repeat:
>> tsk = current;
>> do {
>> retval = do_wait_thread(wo, tsk);
>> - if (retval)
>> + if (retval) {
>> + read_unlock(&tasklist_lock);
>> goto end;
>> + }
>>
>> retval = ptrace_do_wait(wo, tsk);
>> - if (retval)
>> + if (retval) {
>> + read_unlock(&tasklist_lock);
>> goto end;
>> + }
>
>Well, the patch is obviously wrong. Because, again, tasklist_lock was
>already unlocked if (say) wait_task_zombie() reaps a child.
Yes, agree, My wrong
>If you think there is a case which forgets to unlock, please tell us
>more.
I have checked It is getting unlocked in wait_task_zombie Sorry for unnecessary disturbance.
>Oleg.
Thanks
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?