Rakib Mullick <rakib.mull...@gmail.com> writes:
> On 3/7/13, Eric W. Biederman <ebied...@xmission.com> wrote:
>> Fengguang Wu <fengguang...@intel.com> writes:
>>
>>> Greetings,
>>>
>>> I got the below oops and the first bad commit is
>>
>> Doh! On a second look that change is totally wrong. Of course we need
>> to up the ref-count every time we create a new process. Especially if
>> we don't do anything with namespaces.
>>
>> I was looking at it from the wrong angle last night. I should have
>> known better.
>>
>> Patch dropped.
>>
>
> Sad to know :( . From the debug messages, it's kmemcheck report. I
> can't related the problem specified with the patch I've proposed.
>
> It seems at task exit path, at switch_task_namespaces() - after my
> patch atomic_dec_and_test(&ns->count) becomes true (-1), thus
> free_nsproxy() gets called. But, free_nsproxy() shouldn't get called
> here.
>
> Am I right? Or there's something else?
When a new task is created one of two things needs to happen.
A) A reference count needs to be added to the current nsproxy.
B) B a new nsproxy needs to be created.
The way that code works today is far from a shiny example of totally
clear code but it is not incorrect.
By moving get_nsproxy down below the first return 0, you removed taking
the reference count in the one case it is important.
Arguably we should apply the patch below for clarity, and I just might
queue it up for 3.10.
Eric
diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index afc0456..11b8b3f 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -125,22 +125,16 @@ int copy_namespaces(unsigned long flags, struct
task_struct *tsk)
struct nsproxy *old_ns = tsk->nsproxy;
struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
struct nsproxy *new_ns;
- int err = 0;
-
- if (!old_ns)
- return 0;
-
- get_nsproxy(old_ns);
if (!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
- CLONE_NEWPID | CLONE_NEWNET)))
+ CLONE_NEWPID | CLONE_NEWNET))) {
+ get_nsproxy(old_ns);
return 0;
-
- if (!ns_capable(user_ns, CAP_SYS_ADMIN)) {
- err = -EPERM;
- goto out;
}
+ if (!ns_capable(user_ns, CAP_SYS_ADMIN))
+ return -EPERM;
+
/*
* CLONE_NEWIPC must detach from the undolist: after switching
* to a new ipc namespace, the semaphore arrays from the old
@@ -148,22 +142,15 @@ int copy_namespaces(unsigned long flags, struct
task_struct *tsk)
* means share undolist with parent, so we must forbid using
* it along with CLONE_NEWIPC.
*/
- if ((flags & CLONE_NEWIPC) && (flags & CLONE_SYSVSEM)) {
- err = -EINVAL;
- goto out;
- }
+ if ((flags & CLONE_NEWIPC) && (flags & CLONE_SYSVSEM))
+ return -EINVAL;
new_ns = create_new_namespaces(flags, tsk, user_ns, tsk->fs);
- if (IS_ERR(new_ns)) {
- err = PTR_ERR(new_ns);
- goto out;
- }
+ if (IS_ERR(new_ns))
+ return PTR_ERR(new_ns);
tsk->nsproxy = new_ns;
-
-out:
- put_nsproxy(old_ns);
- return err;
+ return 0;
}
void free_nsproxy(struct nsproxy *ns)
--
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/
