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

[Feng Hong]

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@xxxxxxxxxx] 
Sent: 2012年9月21日 1:07
To: Feng Hong
Cc: akpm@xxxxxxxxxxxxxxxxxxxx; gorcunov@xxxxxxxxxx; keescook@xxxxxxxxxxxx; serge.hallyn@xxxxxxxxxxxxx; ebiederm@xxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH V2] poweroff: fix bug in orderly_poweroff

Quoting hongfeng (hongfeng@xxxxxxxxxxx):
> 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@xxxxxxxxxxxxx>

> Change-Id: I2f9ebbb90c0c2443780080ec9507c8d004e5da74
> Signed-off-by: Feng Hong <hongfeng@xxxxxxxxxxx>
> Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> --- Begin Message ---
> 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@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Serge Hallyn <serge.hallyn@xxxxxxxxxxxxx>
> Cc: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> Cc: "Rafael J. Wysocki" <rjw@xxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
>
>  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@xxxxxxxxxxx are
>
> poweroff-fix-bug-in-orderly_poweroff.patch
>
>
> --- End Message ---