[PATCH] poweroff: fix bug in orderly_poweroff

[PATCH] poweroff: fix bug in orderly_poweroff

[hongfeng]

orderly_poweroff is trying to poweroff platform by two steps:
step 1: Call userspace application to poweroff
step 2: If userspace poweroff fail, then do a force power off if force param is set.

The bug here is, step 1 is always successful with param UMH_NO_WAIT,
should change to UMH_WAIT_PROC which will monitor the return value ofuserspace application.

Change-Id: I2f9ebbb90c0c2443780080ec9507c8d004e5da74
Signed-off-by: Feng Hong <hongfeng@marvell.com>
---
 kernel/sys.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 241507f..1b30b30 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2204,7 +2204,7 @@ static int __orderly_poweroff(void)
 		return -ENOMEM;
 	}
 
-	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_NO_WAIT,
+	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_WAIT_PROC,
 				      NULL, argv_cleanup, NULL);
 	if (ret == -ENOMEM)
 		argv_free(argv);
-- 
1.7.0.4

Re: [PATCH] poweroff: fix bug in orderly_poweroff

[Kees Cook]

On Tue, Sep 18, 2012 at 8:27 PM, hongfeng <hongfeng@marvell.com> wrote:
> orderly_poweroff is trying to poweroff platform by two steps:
> step 1: Call userspace application to poweroff
> step 2: If userspace poweroff fail, then do a force power off if force param is set.
>
> The bug here is, step 1 is always successful with param UMH_NO_WAIT,
> should change to UMH_WAIT_PROC which will monitor the return value ofuserspace application.
>
> Signed-off-by: Feng Hong <hongfeng@marvell.com>

Seems right to me.

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] poweroff: fix bug in orderly_poweroff

[Eric W. Biederman]

hongfeng <hongfeng@marvell.com> writes:

> orderly_poweroff is trying to poweroff platform by two steps:
> step 1: Call userspace application to poweroff
> step 2: If userspace poweroff fail, then do a force power off if force param is set.
>
> The bug here is, step 1 is always successful with param UMH_NO_WAIT,

This code has existed for 5 years.  Is this a recent regression?  Why
has no one complained before?

It looks to me that step 2 is:
step 2: If we can not launch the userspace poweroff fail.

> should change to UMH_WAIT_PROC which will monitor the return value
> ofuserspace application.

Is it safe to block indefinitely in the callers waiting for userspace?

If the caller is not running in a kernel thread then we can easily get
into a case where the userspace caller will block waiting for us when we
are waiting for the userspace caller.

I don't want to impeded progress but I don't see the evidence that this
change is good enough.


> Change-Id: I2f9ebbb90c0c2443780080ec9507c8d004e5da74
> Signed-off-by: Feng Hong <hongfeng@marvell.com>
> ---
>  kernel/sys.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 241507f..1b30b30 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2204,7 +2204,7 @@ static int __orderly_poweroff(void)
>  		return -ENOMEM;
>  	}
>  
> -	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_NO_WAIT,
> +	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_WAIT_PROC,
>  				      NULL, argv_cleanup, NULL);
>  	if (ret == -ENOMEM)
>  		argv_free(argv);

RE: [PATCH] poweroff: fix bug in orderly_poweroff

[Feng Hong]

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

Hi, Eric

1. We are developing on an Android phone platform, we use thermal framework to monitor the temperature, when the temperature above like 110 degree, thermal framework will use orderly_shutdown to shutdown phone, however, on Android platform there is no " /sbin/poweroff " cmd ready . Then we want "fail ret" to trigger force shutdown (use kernel_power_off), but always we get "suc ret"
2. Here the caller just wait for "poweroff" userspace application, if it block the called, then it's the "poweroff" problem itself
3. As in the original orderly_shutdown design, we must get the right "ret", if this ret is always "0", then it obey orderly_poweroff design goal. Step 2: force shutdown is always useless code. 
int orderly_poweroff(bool force)
{
        int ret = __orderly_poweroff();

        if (ret && force) {
                printk(KERN_WARNING "Failed to start orderly shutdown: "
                       "forcing the issue\n");

                /*
                 * I guess this should try to kick off some daemon to sync and
                 * poweroff asap.  Or not even bother syncing if we're doing an
                 * emergency shutdown?
                 */
                emergency_sync();
                kernel_power_off();
        }

        return ret;
}1
--
Best Regards,
Feng Hong
Application Processor Software Engnieer 
Marvell Technology (Shanghai) Ltd


-----Original Message-----
From: Eric W. Biederman [mailto:ebiederm@xmission.com] 
Sent: 2012Äê9ÔÂ19ÈÕ 12:47
To: Feng Hong
Cc: akpm@linux-foundation.org; gorcunov@openvz.org; keescook@chromium.org; serge.hallyn@canonical.com; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] poweroff: fix bug in orderly_poweroff

hongfeng <hongfeng@marvell.com> writes:

> orderly_poweroff is trying to poweroff platform by two steps:
> step 1: Call userspace application to poweroff
> step 2: If userspace poweroff fail, then do a force power off if force param is set.
>
> The bug here is, step 1 is always successful with param UMH_NO_WAIT,

This code has existed for 5 years.  Is this a recent regression?  Why
has no one complained before?

It looks to me that step 2 is:
step 2: If we can not launch the userspace poweroff fail.

> should change to UMH_WAIT_PROC which will monitor the return value
> ofuserspace application.

Is it safe to block indefinitely in the callers waiting for userspace?

If the caller is not running in a kernel thread then we can easily get
into a case where the userspace caller will block waiting for us when we
are waiting for the userspace caller.

I don't want to impeded progress but I don't see the evidence that this
change is good enough.


> Change-Id: I2f9ebbb90c0c2443780080ec9507c8d004e5da74
> Signed-off-by: Feng Hong <hongfeng@marvell.com>
> ---
>  kernel/sys.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 241507f..1b30b30 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2204,7 +2204,7 @@ static int __orderly_poweroff(void)
>  		return -ENOMEM;
>  	}
>  
> -	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_NO_WAIT,
> +	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_WAIT_PROC,
>  				      NULL, argv_cleanup, NULL);
>  	if (ret == -ENOMEM)
>  		argv_free(argv);
ÿôèº{.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¥

Re: [PATCH] poweroff: fix bug in orderly_poweroff

[Eric W. Biederman]

Feng Hong <hongfeng@marvell.com> writes:

> Hi, Eric
>
> 1. We are developing on an Android phone platform, we use thermal
> framework to monitor the temperature, when the temperature above like
> 110 degree, thermal framework will use orderly_shutdown to shutdown
> phone, however, on Android platform there is no " /sbin/poweroff " cmd
> ready . Then we want "fail ret" to trigger force shutdown (use
> kernel_power_off), but always we get "suc ret"

> 2. Here the caller just wait for "poweroff" userspace application, if
> it block the called, then it's the "poweroff" problem itself

> 3. As in the original orderly_shutdown design, we must get the right
> "ret", if this ret is always "0", then it obey orderly_poweroff design
> goal. Step 2: force shutdown is always useless code. 

That sounds like a clear case that we need to change it to
UMH_WAIT_EXEC.

Changing it to UMH_WAIT_PROC seems much more dangerous.

Eric

RE: [PATCH] poweroff: fix bug in orderly_poweroff

[Feng Hong]

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

Hi, Eric,

I agree with your idea, I'll prepare another patch, thanks for remind this possible issue.

--
Best Regards,
Feng Hong
Application Processor Software Engnieer 
Marvell Technology (Shanghai) Ltd


-----Original Message-----
From: Eric W. Biederman [mailto:ebiederm@xmission.com] 
Sent: 2012Äê9ÔÂ19ÈÕ 13:58
To: Feng Hong
Cc: akpm@linux-foundation.org; gorcunov@openvz.org; keescook@chromium.org; serge.hallyn@canonical.com; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] poweroff: fix bug in orderly_poweroff

Feng Hong <hongfeng@marvell.com> writes:

> Hi, Eric
>
> 1. We are developing on an Android phone platform, we use thermal
> framework to monitor the temperature, when the temperature above like
> 110 degree, thermal framework will use orderly_shutdown to shutdown
> phone, however, on Android platform there is no " /sbin/poweroff " cmd
> ready . Then we want "fail ret" to trigger force shutdown (use
> kernel_power_off), but always we get "suc ret"

> 2. Here the caller just wait for "poweroff" userspace application, if
> it block the called, then it's the "poweroff" problem itself

> 3. As in the original orderly_shutdown design, we must get the right
> "ret", if this ret is always "0", then it obey orderly_poweroff design
> goal. Step 2: force shutdown is always useless code. 

That sounds like a clear case that we need to change it to
UMH_WAIT_EXEC.

Changing it to UMH_WAIT_PROC seems much more dangerous.

Eric
ÿôèº{.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¥