[PATCH V2] poweroff: fix bug in orderly_poweroff

[PATCH V2] 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_EXEC which will monitor whether user application successful run.

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

diff --git a/kernel/sys.c b/kernel/sys.c
index 241507f..a624d4c 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_EXEC,
 				      NULL, argv_cleanup, NULL);
 	if (ret == -ENOMEM)
 		argv_free(argv);
-- 
1.7.0.4

Re: [PATCH V2] poweroff: fix bug in orderly_poweroff

[Serge E. Hallyn]

Quoting hongfeng (hongfeng@marvell.com):
> 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,

Note that a changelog here explaining that you switched to UMH_WAIT_EXEC
per Eric's suggestion would be both informative and courteous.

> should change to UMH_WAIT_EXEC which will monitor whether user application successful run.

Is this actually sufficient for you?  The exec will have started, but
may for whatever (very unlikely) reason fail.  If you're happy with
it,

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> Change-Id: I2f9ebbb90c0c2443780080ec9507c8d004e5da74
> Signed-off-by: Feng Hong <hongfeng@marvell.com>
> Acked-by: Kees Cook <keescook@chromium.org>
> ---
>  kernel/sys.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 241507f..a624d4c 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_EXEC,
>  				      NULL, argv_cleanup, NULL);
>  	if (ret == -ENOMEM)
>  		argv_free(argv);
> -- 
> 1.7.0.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH V2] poweroff: fix bug in orderly_poweroff

[Feng Hong]

[-- Attachment #1: Type: text/plain, Size: 3039 bytes --]

Hi, Serge,

I am just a graduate and it's my first time to send a patch to opensource, so thank you very much for reminding me the "changelog affairs", it seems this patch has been added to -mm tree as attached mail, and I have no chance to change the comments, right ? Then I must remember this and be careful next time. Thanks again for reminding me !

>Is this actually sufficient for you?  The exec will have started, but may for whatever (very unlikely) reason fail.  If you're happy with it,
I think UMH_WAIT_EXEC is sufficient for me, as in our system there is no "/sbin/poweroff" existed. On the other hand, UMH_WAIT_PROC is not suitable here as Eric analysis; if using UMH_WAIT_EXEC, and the user application fail, I'd prefer to complain bad application. So using UMH_WAIT_EXEC and UMH_WAIT_PROC has a tradeoff here, what do you think so ?

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

-----Original Message-----
From: Serge E. Hallyn [mailto:serge@hallyn.com] 
Sent: 2012年9月21日 1:07
To: Feng Hong
Cc: akpm@linux-foundation.org; gorcunov@openvz.org; keescook@chromium.org; serge.hallyn@canonical.com; ebiederm@xmission.com; linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2] poweroff: fix bug in orderly_poweroff

Quoting hongfeng (hongfeng@marvell.com):
> 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,

Note that a changelog here explaining that you switched to UMH_WAIT_EXEC
per Eric's suggestion would be both informative and courteous.

> should change to UMH_WAIT_EXEC which will monitor whether user application successful run.

Is this actually sufficient for you?  The exec will have started, but
may for whatever (very unlikely) reason fail.  If you're happy with
it,

Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

> Change-Id: I2f9ebbb90c0c2443780080ec9507c8d004e5da74
> Signed-off-by: Feng Hong <hongfeng@marvell.com>
> Acked-by: Kees Cook <keescook@chromium.org>
> ---
>  kernel/sys.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 241507f..a624d4c 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_EXEC,
>  				      NULL, argv_cleanup, NULL);
>  	if (ret == -ENOMEM)
>  		argv_free(argv);
> -- 
> 1.7.0.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[-- Attachment #2: Type: message/rfc822, Size: 4294 bytes --]

From: "akpm@linux-foundation.org" <akpm@linux-foundation.org>
To: "mm-commits@vger.kernel.org" <mm-commits@vger.kernel.org>
Cc: Feng Hong <hongfeng@marvell.com>, "ebiederm@xmission.com" <ebiederm@xmission.com>, "keescook@chromium.org" <keescook@chromium.org>, "rjw@sisk.pl" <rjw@sisk.pl>, "serge.hallyn@canonical.com" <serge.hallyn@canonical.com>
Subject: + poweroff-fix-bug-in-orderly_poweroff.patch added to -mm tree
Date: Wed, 19 Sep 2012 12:12:39 -0700
Message-ID: <20120919191240.2DDC120004E@hpza10.eem.corp.google.com>


The patch titled
     Subject: poweroff: fix bug in orderly_poweroff()
has been added to the -mm tree.  Its filename is
     poweroff-fix-bug-in-orderly_poweroff.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

The -mm tree is included into linux-next and is updated
there every 3-4 working days

------------------------------------------------------
From: hongfeng <hongfeng@marvell.com>
Subject: poweroff: fix bug in orderly_poweroff()

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_EXEC which will monitor whether user application
successful run.

Signed-off-by: Feng Hong <hongfeng@marvell.com>
Acked-by: Kees Cook <keescook@chromium.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/sys.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN kernel/sys.c~poweroff-fix-bug-in-orderly_poweroff kernel/sys.c
--- a/kernel/sys.c~poweroff-fix-bug-in-orderly_poweroff
+++ a/kernel/sys.c
@@ -2205,7 +2205,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_EXEC,
                                      NULL, argv_cleanup, NULL);
        if (ret == -ENOMEM)
                argv_free(argv);
_

Patches currently in -mm which might be from hongfeng@marvell.com are

poweroff-fix-bug-in-orderly_poweroff.patch

Re: [PATCH V2] poweroff: fix bug in orderly_poweroff

[Andrew Morton]

On Thu, 20 Sep 2012 17:16:35 -0700
Feng Hong <hongfeng@marvell.com> wrote:

> I am just a graduate and it's my first time to send a patch to
> opensource, so thank you very much for reminding me the "changelog
> affairs", it seems this patch has been added to -mm tree as attached
> mail, and I have no chance to change the comments, right ?  Then I must
> remember this and be careful next time.  Thanks again for reminding me!

It depends.  If the person who committed the patch was using a git tree
then it can be difficult for them to alter a changelog.

But the -mm tree is not mastered in git (for this and other reasons),
and I alter changelogs all the time.  So please feel free to send
replacement text and I shall copy-n-paste that text straight into the patch.

RE: [PATCH V2] 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: 2800 bytes --]

Hi, Serge, Andrew,

Will following an acceptable change log ?

/////////////////////////////////////////////////////////////////////////////////////////////////
orderly_poweroff is trying to poweroff platform by two steps:
step 1: Call user space application to poweroff
step 2: If user space 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, which obey
the design goal of orderly_poweroff.

We have two choices here:
UMH_WAIT_EXEC which means wait for the exec, but not the process;
UMH_WAIT_PROC which means wait for the process to complete.
we need to trade off the two choices:

If using UMH_WAIT_EXEC, there is potential issue comments by Serge E. Hallyn: The exec will
have started, but may for whatever (very unlikely) reason fail.

If using UMH_WAIT_PROC, there is potential issue comments by Eric W. Biederman: If the caller
is not running in a kernel thread then we can easily get into a case where the user space caller
will block waiting for us when we are waiting for the user space caller.

Thanks for their excellent ideas, based on the above discussion, we finally choose UMH_WAIT_EXEC,
which is much more safe, if the user application really fails, we just complain the application
itself, it seems a better choice here.
//////////////////////////////////////////////////////////////////////////////////////////////////////////////

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


-----Original Message-----
From: Andrew Morton [mailto:akpm@linux-foundation.org] 
Sent: 2012Äê9ÔÂ21ÈÕ 8:25
To: Feng Hong
Cc: Serge E. Hallyn; gorcunov@openvz.org; keescook@chromium.org; serge.hallyn@canonical.com; ebiederm@xmission.com; linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2] poweroff: fix bug in orderly_poweroff

On Thu, 20 Sep 2012 17:16:35 -0700
Feng Hong <hongfeng@marvell.com> wrote:

> I am just a graduate and it's my first time to send a patch to
> opensource, so thank you very much for reminding me the "changelog
> affairs", it seems this patch has been added to -mm tree as attached
> mail, and I have no chance to change the comments, right ?  Then I must
> remember this and be careful next time.  Thanks again for reminding me!

It depends.  If the person who committed the patch was using a git tree
then it can be difficult for them to alter a changelog.

But the -mm tree is not mastered in git (for this and other reasons),
and I alter changelogs all the time.  So please feel free to send
replacement text and I shall copy-n-paste that text straight into the patch.

ÿôèº{.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 V2] poweroff: fix bug in orderly_poweroff

[Serge Hallyn]

Quoting Feng Hong (hongfeng@marvell.com):
> Hi, Serge,
> 
> I am just a graduate and it's my first time to send a patch to opensource, so thank you very much for reminding me the "changelog affairs", it seems this patch has been added to -mm tree as attached mail, and I have no chance to change the comments, right ? Then I must remember this and be careful next time. Thanks again for reminding me !

Sorry, your description was fine, what i meant was something below your
patch description that looks like

Change since v1:
	[date] Per Eric's sugestion, switch from UMH_WAIT_PROC to UMH_WAIT_EXEC.

> >Is this actually sufficient for you?  The exec will have started, but may for whatever (very unlikely) reason fail.  If you're happy with it,
> I think UMH_WAIT_EXEC is sufficient for me, as in our system there is no "/sbin/poweroff" existed. On the other hand, UMH_WAIT_PROC is not suitable here as Eric analysis; if using UMH_WAIT_EXEC, and the user application fail, I'd prefer to complain bad application. So using UMH_WAIT_EXEC and UMH_WAIT_PROC has a tradeoff here, what do you think so ?

Yup, that sounds fine to me, I just wanted to make sure you were ok with the
fact that application failure (after successful exec) will be ignored.

thanks,
-serge

[PATCH V2] 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_EXEC which will monitor whether user application successful run.

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..a624d4c 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_EXEC,
 				      NULL, argv_cleanup, NULL);
 	if (ret == -ENOMEM)
 		argv_free(argv);
-- 
1.7.0.4