Re: [PATCH] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully.
Quoting Chen Gang (gang.c...@asianux.com):
> When unshare_userns() succeed, recommend to always set the return
> parameter which may be used by caller.
>
> The caller has rights to call it with 'new_cred' uninitialized, if
> succeed, the caller can assume the 'new_cred' has been initialized.
But the only existing caller (sys_unshare) does in fact initialize it to
NULL. So while this patch does no harm, is it necessary?
> Signed-off-by: Chen Gang
> ---
> include/linux/user_namespace.h |1 +
> kernel/user_namespace.c|4 +++-
> 2 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index b6b215f..3159af5 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -75,6 +75,7 @@ static inline int unshare_userns(unsigned long
> unshare_flags,
> {
> if (unshare_flags & CLONE_NEWUSER)
> return -EINVAL;
> + *new_cred = NULL;
> return 0;
> }
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 6e50a44..6b90818 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -107,8 +107,10 @@ int unshare_userns(unsigned long unshare_flags, struct
> cred **new_cred)
> struct cred *cred;
> int err = -ENOMEM;
>
> - if (!(unshare_flags & CLONE_NEWUSER))
> + if (!(unshare_flags & CLONE_NEWUSER)) {
> + *new_cred = NULL;
> return 0;
> + }
>
> cred = prepare_creds();
> if (cred) {
> --
> 1.7.7.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@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] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully.
On 08/20, Serge Hallyn wrote: > > Quoting Chen Gang (gang.c...@asianux.com): > > When unshare_userns() succeed, recommend to always set the return > > parameter which may be used by caller. > > > > The caller has rights to call it with 'new_cred' uninitialized, if > > succeed, the caller can assume the 'new_cred' has been initialized. > > But the only existing caller (sys_unshare) does in fact initialize it to > NULL. So while this patch does no harm, is it necessary? Agreed. Plus, with this patch unshare_userns() becomes "inconsistent" compared to other unshare_ helpers. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully.
On 08/20/2013 10:37 PM, Oleg Nesterov wrote: > On 08/20, Serge Hallyn wrote: >> >> Quoting Chen Gang (gang.c...@asianux.com): >>> When unshare_userns() succeed, recommend to always set the return >>> parameter which may be used by caller. >>> >>> The caller has rights to call it with 'new_cred' uninitialized, if >>> succeed, the caller can assume the 'new_cred' has been initialized. >> >> But the only existing caller (sys_unshare) does in fact initialize it to >> NULL. So while this patch does no harm, is it necessary? > > Agreed. > > Plus, with this patch unshare_userns() becomes "inconsistent" compared > to other unshare_ helpers. > > Oleg. > > > Hmm... for static functions, they don't need, but for extern functions, recommend to do so. For "unshare_ helpers", I find 3 extern functions: unshare_files() which already set value. unshare_userns() and unshare_nsproxy_namespaces() which not set. In my opinion, recommend to always set the return parameter when succeed, for the 2 left extern functions. Thanks. -- Chen Gang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully.
On 08/21, Chen Gang wrote: > > On 08/20/2013 10:37 PM, Oleg Nesterov wrote: > > On 08/20, Serge Hallyn wrote: > >> > >> > >> But the only existing caller (sys_unshare) does in fact initialize it to > >> NULL. So while this patch does no harm, is it necessary? > > > > Agreed. > > > > Plus, with this patch unshare_userns() becomes "inconsistent" compared > > to other unshare_ helpers. > > > > Hmm... for static functions, they don't need, but for extern functions, > recommend to do so. > > > For "unshare_ helpers", I find 3 extern functions: > > unshare_files() which already set value. > unshare_userns() and unshare_nsproxy_namespaces() which not set. > > In my opinion, recommend to always set the return parameter when > succeed, for the 2 left extern functions. I do not think that static/extern should make any difference. To me, the cleanup should make unshare_userns() return "cred *" (or NULL or ERR_PTR()) But this is subjective and in this case we should change other unshare_ helpers too. Doesn't worth the trouble. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully.
On 08/21/2013 07:57 PM, Oleg Nesterov wrote: > On 08/21, Chen Gang wrote: >> >> On 08/20/2013 10:37 PM, Oleg Nesterov wrote: >>> On 08/20, Serge Hallyn wrote: But the only existing caller (sys_unshare) does in fact initialize it to NULL. So while this patch does no harm, is it necessary? >>> >>> Agreed. >>> >>> Plus, with this patch unshare_userns() becomes "inconsistent" compared >>> to other unshare_ helpers. >>> >> >> Hmm... for static functions, they don't need, but for extern functions, >> recommend to do so. >> >> >> For "unshare_ helpers", I find 3 extern functions: >> >> unshare_files() which already set value. >> unshare_userns() and unshare_nsproxy_namespaces() which not set. >> >> In my opinion, recommend to always set the return parameter when >> succeed, for the 2 left extern functions. > > I do not think that static/extern should make any difference. To me, > the cleanup should make unshare_userns() return "cred *" (or NULL or > ERR_PTR()) But this is subjective and in this case we should change > other unshare_ helpers too. Doesn't worth the trouble. > Normal static function (excluding callback function) is used inside, it doesn't belong to interface, but extern function belongs to interface (it is used by outside which may be out of control by inside). For interface, need implement its definitions precisely, in our case, when return succeed, if 'new_cred' should be NULL, inside need set it explicitly to be sure of it to outside. And for the other extern "unshare_ helpers", recommend to do so, too. Currently, it is really not a bug, but in the future, it may will be. > Oleg. > > > Thanks. -- Chen Gang -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
