kernel: user_namespace: always set the return parameter 'new_cred' when call unshare_userns() successfully.

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 <[email protected]>

---
 include/linux/user_namespace.h |    1 +
 kernel/user_namespace.c        |    4 +++-
 2 files changed, 4 insertions(+), 1 deletions(-)

include/linux/user_namespace.h +1 -0

index b6b215f..3159af5 100644
@@ -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;
 }
 

kernel/user_namespace.c +3 -1

index 6e50a44..6b90818 100644
@@ -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) {

Signature · 1 line

1.7.7.6

Quoted 5 lines · Chen Gang

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?

Quoted 10 lines · Serge Hallyn

Quoted 5 lines · Chen Gang

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.

Quoted 11 lines · Oleg Nesterov

Quoted 4 lines · Serge Hallyn

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.

Quoted 23 lines · Chen Gang

Quoted 11 lines · Oleg Nesterov

Quoted 4 lines · Serge Hallyn

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.

Quoted 2 lines

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.