All of lore.kernel.org
 help / color / mirror / Atom feed
* [EDT][PATCH] kernel/exit.c : Fix missing read_unlock
@ 2015-05-21  5:00 Maninder Singh
  2015-05-21  6:03 ` Ingo Molnar
  2015-05-21 18:16 ` Oleg Nesterov
  0 siblings, 2 replies; 10+ messages in thread
From: Maninder Singh @ 2015-05-21  5:00 UTC (permalink / raw)
  To: akpm, oleg, mhocko, peterz, mingo, riel, ionut.m.alexa, peter,
	linux-kernel, v.narang, AKHILESH KUMAR

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 1144 bytes --]

 EP-F6AA0618C49C4AEDA73BFF1B39950BAB
Hi,

From: Maninder Singh <maninder1.s@samsung.com>

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 <maninder1.s@samsung.com>
Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Reviewd-by: Akhilesh Kumar <akhilesh.k@samsung.com>
---
 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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [EDT][PATCH] kernel/exit.c : Fix missing read_unlock
  2015-05-21  5:00 [EDT][PATCH] kernel/exit.c : Fix missing read_unlock Maninder Singh
@ 2015-05-21  6:03 ` Ingo Molnar
  2015-05-21 10:32   ` Frans Klaver
  2015-05-21 18:16 ` Oleg Nesterov
  1 sibling, 1 reply; 10+ messages in thread
From: Ingo Molnar @ 2015-05-21  6:03 UTC (permalink / raw)
  To: Maninder Singh
  Cc: akpm, oleg, mhocko, peterz, riel, ionut.m.alexa, peter,
	linux-kernel, v.narang, AKHILESH KUMAR, Peter Zijlstra


* Maninder Singh <maninder1.s@samsung.com> wrote:

>  EP-F6AA0618C49C4AEDA73BFF1B39950BAB
> Hi,
> 
> From: Maninder Singh <maninder1.s@samsung.com>
> 
> 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 <maninder1.s@samsung.com>
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Reviewd-by: Akhilesh Kumar <akhilesh.k@samsung.com>
> ---
>  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 <oleg@redhat.com>
 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [EDT][PATCH] kernel/exit.c : Fix missing read_unlock
  2015-05-21  6:03 ` Ingo Molnar
@ 2015-05-21 10:32   ` Frans Klaver
  2015-05-21 10:56     ` Ingo Molnar
  0 siblings, 1 reply; 10+ messages in thread
From: Frans Klaver @ 2015-05-21 10:32 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Maninder Singh, Andrew Morton, oleg, Michal Hocko,
	Peter Zijlstra, Rik van Riel, ionut.m.alexa, Peter Hurley,
	linux-kernel, v.narang, AKHILESH KUMAR, Peter Zijlstra

On Thu, May 21, 2015 at 8:03 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Maninder Singh <maninder1.s@samsung.com> wrote:
>
>>  EP-F6AA0618C49C4AEDA73BFF1B39950BAB
>> Hi,
>>
>> From: Maninder Singh <maninder1.s@samsung.com>
>>
>> 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 <maninder1.s@samsung.com>
>> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
>> Reviewd-by: Akhilesh Kumar <akhilesh.k@samsung.com>
>> ---
>>  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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [EDT][PATCH] kernel/exit.c : Fix missing read_unlock
  2015-05-21 10:32   ` Frans Klaver
@ 2015-05-21 10:56     ` Ingo Molnar
  2015-05-21 11:39       ` Frans Klaver
  0 siblings, 1 reply; 10+ messages in thread
From: Ingo Molnar @ 2015-05-21 10:56 UTC (permalink / raw)
  To: Frans Klaver
  Cc: Maninder Singh, Andrew Morton, oleg, Michal Hocko,
	Peter Zijlstra, Rik van Riel, ionut.m.alexa, Peter Hurley,
	linux-kernel, v.narang, AKHILESH KUMAR, Peter Zijlstra


* Frans Klaver <fransklaver@gmail.com> wrote:

> On Thu, May 21, 2015 at 8:03 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Maninder Singh <maninder1.s@samsung.com> wrote:
> >
> >>  EP-F6AA0618C49C4AEDA73BFF1B39950BAB
> >> Hi,
> >>
> >> From: Maninder Singh <maninder1.s@samsung.com>
> >>
> >> 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 <maninder1.s@samsung.com>
> >> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> >> Reviewd-by: Akhilesh Kumar <akhilesh.k@samsung.com>
> >> ---
> >>  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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [EDT][PATCH] kernel/exit.c : Fix missing read_unlock
  2015-05-21 10:56     ` Ingo Molnar
@ 2015-05-21 11:39       ` Frans Klaver
  0 siblings, 0 replies; 10+ messages in thread
From: Frans Klaver @ 2015-05-21 11:39 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Maninder Singh, Andrew Morton, oleg, Michal Hocko,
	Peter Zijlstra, Rik van Riel, ionut.m.alexa, Peter Hurley,
	linux-kernel, v.narang, AKHILESH KUMAR, Peter Zijlstra

On 21 May 2015 12:56:22 CEST, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Frans Klaver <fransklaver@gmail.com> wrote:
>
>> On Thu, May 21, 2015 at 8:03 AM, Ingo Molnar <mingo@kernel.org>
>wrote:
>> >
>> > * Maninder Singh <maninder1.s@samsung.com> wrote:
>> >
>> >>  EP-F6AA0618C49C4AEDA73BFF1B39950BAB
>> >> Hi,
>> >>
>> >> From: Maninder Singh <maninder1.s@samsung.com>
>> >>
>> >> 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 <maninder1.s@samsung.com>
>> >> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
>> >> Reviewd-by: Akhilesh Kumar <akhilesh.k@samsung.com>
>> >> ---
>> >>  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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [EDT][PATCH] kernel/exit.c : Fix missing read_unlock
  2015-05-21  5:00 [EDT][PATCH] kernel/exit.c : Fix missing read_unlock Maninder Singh
  2015-05-21  6:03 ` Ingo Molnar
@ 2015-05-21 18:16 ` Oleg Nesterov
  1 sibling, 0 replies; 10+ messages in thread
From: Oleg Nesterov @ 2015-05-21 18:16 UTC (permalink / raw)
  To: Maninder Singh
  Cc: akpm, mhocko, peterz, mingo, riel, ionut.m.alexa, peter,
	linux-kernel, v.narang, AKHILESH KUMAR

On 05/21, Maninder Singh wrote:
>
>  EP-F6AA0618C49C4AEDA73BFF1B39950BAB
> Hi,
>
> From: Maninder Singh <maninder1.s@samsung.com>
>
> 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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [EDT][PATCH] kernel/exit.c : Fix missing read_unlock
@ 2015-05-22  3:36 Maninder Singh
  0 siblings, 0 replies; 10+ messages in thread
From: Maninder Singh @ 2015-05-22  3:36 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: akpm, mhocko, peterz, mingo, riel, ionut.m.alexa, peter, linux-kernel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 1262 bytes --]

 EP-F6AA0618C49C4AEDA73BFF1B39950BAB

Hi Oleg,

>> Hi,
>>
>> From: Maninder Singh <maninder1.s@samsung.com>
>>
>> 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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [EDT][PATCH] kernel/exit.c : Fix missing read_unlock
@ 2015-05-21 11:44 Maninder Singh
  0 siblings, 0 replies; 10+ messages in thread
From: Maninder Singh @ 2015-05-21 11:44 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: akpm, oleg, mhocko, peterz, riel, ionut.m.alexa, peter,
	linux-kernel, Vaneet Narang, AKHILESH KUMAR, Peter Zijlstra

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 2666 bytes --]

EP-F6AA0618C49C4AEDA73BFF1B39950BAB

Hi Ingo,

>> >> Hi,
>> >> 
>> >> From: Maninder Singh <maninder1.s@samsung.com>
>> >> 
>> >> 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 <maninder1.s@samsung.com>
>> >> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
>> >> Reviewd-by: Akhilesh Kumar <akhilesh.k@samsung.com>
>> >> ---
>> >>  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 <oleg@redhat.com>
>> > 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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [EDT][PATCH] kernel/exit.c : Fix missing read_unlock
  2015-05-21  6:34 Maninder Singh
@ 2015-05-21 10:58 ` Ingo Molnar
  0 siblings, 0 replies; 10+ messages in thread
From: Ingo Molnar @ 2015-05-21 10:58 UTC (permalink / raw)
  To: Maninder Singh
  Cc: akpm, oleg, mhocko, peterz, riel, ionut.m.alexa, peter,
	linux-kernel, Vaneet Narang, AKHILESH KUMAR, Peter Zijlstra


* Maninder Singh <maninder1.s@samsung.com> wrote:

> EP-F6AA0618C49C4AEDA73BFF1B39950BAB
> >> Hi,
> >> 
> >> From: Maninder Singh <maninder1.s@samsung.com>
> >> 
> >> 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 <maninder1.s@samsung.com>
> >> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> >> Reviewd-by: Akhilesh Kumar <akhilesh.k@samsung.com>
> >> ---
> >>  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 <oleg@redhat.com>
> > 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [EDT][PATCH] kernel/exit.c : Fix missing read_unlock
@ 2015-05-21  6:34 Maninder Singh
  2015-05-21 10:58 ` Ingo Molnar
  0 siblings, 1 reply; 10+ messages in thread
From: Maninder Singh @ 2015-05-21  6:34 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: akpm, oleg, mhocko, peterz, riel, ionut.m.alexa, peter,
	linux-kernel, Vaneet Narang, AKHILESH KUMAR, Peter Zijlstra

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 2119 bytes --]

EP-F6AA0618C49C4AEDA73BFF1B39950BAB
>> Hi,
>> 
>> From: Maninder Singh <maninder1.s@samsung.com>
>> 
>> 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 <maninder1.s@samsung.com>
>> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
>> Reviewd-by: Akhilesh Kumar <akhilesh.k@samsung.com>
>> ---
>>  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 <oleg@redhat.com>
> 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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-05-22  3:36 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-21  5:00 [EDT][PATCH] kernel/exit.c : Fix missing read_unlock Maninder Singh
2015-05-21  6:03 ` Ingo Molnar
2015-05-21 10:32   ` Frans Klaver
2015-05-21 10:56     ` Ingo Molnar
2015-05-21 11:39       ` Frans Klaver
2015-05-21 18:16 ` Oleg Nesterov
2015-05-21  6:34 Maninder Singh
2015-05-21 10:58 ` Ingo Molnar
2015-05-21 11:44 Maninder Singh
2015-05-22  3:36 Maninder Singh

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.