* [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