* [PATCH] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully. @ 2013-08-20 2:53 Chen Gang 2013-08-20 14:10 ` Serge Hallyn 0 siblings, 1 reply; 6+ messages in thread From: Chen Gang @ 2013-08-20 2:53 UTC (permalink / raw) To: Eric W. Biederman, Serge Hallyn, Oleg Nesterov, Andy Lutomirski, dhowells@redhat.com Cc: linux-kernel@vger.kernel.org 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. Signed-off-by: Chen Gang <gang.chen@asianux.com> --- 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully. 2013-08-20 2:53 [PATCH] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully Chen Gang @ 2013-08-20 14:10 ` Serge Hallyn 2013-08-20 14:37 ` Oleg Nesterov 0 siblings, 1 reply; 6+ messages in thread From: Serge Hallyn @ 2013-08-20 14:10 UTC (permalink / raw) To: Chen Gang Cc: Eric W. Biederman, Serge Hallyn, Oleg Nesterov, Andy Lutomirski, dhowells@redhat.com, linux-kernel@vger.kernel.org Quoting Chen Gang (gang.chen@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 <gang.chen@asianux.com> > --- > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully. 2013-08-20 14:10 ` Serge Hallyn @ 2013-08-20 14:37 ` Oleg Nesterov 2013-08-21 4:15 ` Chen Gang 0 siblings, 1 reply; 6+ messages in thread From: Oleg Nesterov @ 2013-08-20 14:37 UTC (permalink / raw) To: Serge Hallyn Cc: Chen Gang, Eric W. Biederman, Serge Hallyn, Andy Lutomirski, dhowells@redhat.com, linux-kernel@vger.kernel.org On 08/20, Serge Hallyn wrote: > > Quoting Chen Gang (gang.chen@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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully. 2013-08-20 14:37 ` Oleg Nesterov @ 2013-08-21 4:15 ` Chen Gang 2013-08-21 11:57 ` Oleg Nesterov 0 siblings, 1 reply; 6+ messages in thread From: Chen Gang @ 2013-08-21 4:15 UTC (permalink / raw) To: Oleg Nesterov Cc: Serge Hallyn, Eric W. Biederman, Serge Hallyn, Andy Lutomirski, dhowells@redhat.com, linux-kernel@vger.kernel.org On 08/20/2013 10:37 PM, Oleg Nesterov wrote: > On 08/20, Serge Hallyn wrote: >> >> Quoting Chen Gang (gang.chen@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully. 2013-08-21 4:15 ` Chen Gang @ 2013-08-21 11:57 ` Oleg Nesterov 2013-08-22 1:30 ` Chen Gang 0 siblings, 1 reply; 6+ messages in thread From: Oleg Nesterov @ 2013-08-21 11:57 UTC (permalink / raw) To: Chen Gang Cc: Serge Hallyn, Eric W. Biederman, Serge Hallyn, Andy Lutomirski, dhowells@redhat.com, linux-kernel@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully. 2013-08-21 11:57 ` Oleg Nesterov @ 2013-08-22 1:30 ` Chen Gang 0 siblings, 0 replies; 6+ messages in thread From: Chen Gang @ 2013-08-22 1:30 UTC (permalink / raw) To: Oleg Nesterov Cc: Serge Hallyn, Eric W. Biederman, Serge Hallyn, Andy Lutomirski, dhowells@redhat.com, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-22 3:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-20 2:53 [PATCH] kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully Chen Gang 2013-08-20 14:10 ` Serge Hallyn 2013-08-20 14:37 ` Oleg Nesterov 2013-08-21 4:15 ` Chen Gang 2013-08-21 11:57 ` Oleg Nesterov 2013-08-22 1:30 ` Chen Gang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox