From: Raphael S.Carvalho <[email protected]>
Starting point: create_pid_namespace()
Suppose create_pid_cachep() was executed sucessfully, thus:
pcache was allocated by kmalloc().
cachep received a cache created by kmem_cache_create().
and pcache->list was added to the list pid_caches_lh.
So what would happen if proc_alloc_inum() returns an error?
The resources allocated by create_pid_namespace() would be deallocated!
How about those resources allocated by create_pid_cachep()?
By knowing that, I created this patch in order to fix that!
Signed-off-by: Raphael S.Carvalho <[email protected]>
---
kernel/pid_namespace.c | 16 +++++++++++-----
1 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index c1c3dc1..d94e4b6 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -37,7 +37,7 @@ static struct kmem_cache *pid_ns_cachep;
* @nr_ids: the number of numerical ids this pid will have to carry
*/
-static struct kmem_cache *create_pid_cachep(int nr_ids)
+static struct pid_cache *create_pid_cachep(int nr_ids)
{
struct pid_cache *pcache;
struct kmem_cache *cachep;
@@ -63,7 +63,7 @@ static struct kmem_cache *create_pid_cachep(int nr_ids)
list_add(&pcache->list, &pid_caches_lh);
out:
mutex_unlock(&pid_caches_mutex);
- return pcache->cachep;
+ return pcache;
err_cachep:
kfree(pcache);
@@ -85,6 +85,7 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
struct pid_namespace *parent_pid_ns)
{
struct pid_namespace *ns;
+ struct pid_cache *pcache;
unsigned int level = parent_pid_ns->level + 1;
int i;
int err;
@@ -103,15 +104,16 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
if (!ns->pidmap[0].page)
goto out_free;
- ns->pid_cachep = create_pid_cachep(level + 1);
- if (ns->pid_cachep == NULL)
+ pcache = create_pid_cachep(level + 1);
+ if (pcache == NULL)
goto out_free_map;
err = proc_alloc_inum(&ns->proc_inum);
if (err)
- goto out_free_map;
+ goto out_free_cachep;
kref_init(&ns->kref);
+ ns->pid_cachep = pcache->cachep;
ns->level = level;
ns->parent = get_pid_ns(parent_pid_ns);
ns->user_ns = get_user_ns(user_ns);
@@ -126,6 +128,10 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
return ns;
+out_free_cachep:
+ kmem_cache_destroy(pcache->cachep);
+ list_del(&pcache->list);
+ kfree(pcache);
out_free_map:
kfree(ns->pidmap[0].page);
out_free:
--
1.7.2.5
On 2013/03/05 11:26, Eric W. Biederman wrote:
> From: Raphael S.Carvalho <[email protected]>
>
> Starting point: create_pid_namespace()
>
> Suppose create_pid_cachep() was executed sucessfully, thus:
> pcache was allocated by kmalloc().
> cachep received a cache created by kmem_cache_create().
> and pcache->list was added to the list pid_caches_lh.
>
> So what would happen if proc_alloc_inum() returns an error?
> The resources allocated by create_pid_namespace() would be deallocated!
> How about those resources allocated by create_pid_cachep()?
> By knowing that, I created this patch in order to fix that!
>
> Signed-off-by: Raphael S.Carvalho <[email protected]>
> ---
Actually I noticed this problem and I think it is not a BUG.
Since the pid_cache is created for all pid namespace which have the same level.
Even this pid namespace is failed to create, the pid_cache will not be leaked, Other
pid namespace which has the same level will use the pid_cache and no need to
allocate it again. In other words, the pid_cache for every level pid namespace will
only be created once.
I also think this patch add a bug,because there may be other pid namespace's pid_cachep
points to the same pid_cache which will be free at the by label out_free_cachep.
On Tue, Mar 5, 2013 at 12:51 AM, Gao feng <[email protected]> wrote:
> On 2013/03/05 11:26, Eric W. Biederman wrote:
>> From: Raphael S.Carvalho <[email protected]>
>>
>> Starting point: create_pid_namespace()
>>
>> Suppose create_pid_cachep() was executed sucessfully, thus:
>> pcache was allocated by kmalloc().
>> cachep received a cache created by kmem_cache_create().
>> and pcache->list was added to the list pid_caches_lh.
>>
>> So what would happen if proc_alloc_inum() returns an error?
>> The resources allocated by create_pid_namespace() would be deallocated!
>> How about those resources allocated by create_pid_cachep()?
>> By knowing that, I created this patch in order to fix that!
>>
>> Signed-off-by: Raphael S.Carvalho <[email protected]>
>> ---
>
> Actually I noticed this problem and I think it is not a BUG.
> Since the pid_cache is created for all pid namespace which have the same level.
> Even this pid namespace is failed to create, the pid_cache will not be leaked, Other
> pid namespace which has the same level will use the pid_cache and no need to
> allocate it again. In other words, the pid_cache for every level pid namespace will
> only be created once.
>
> I also think this patch add a bug,because there may be other pid namespace's pid_cachep
> points to the same pid_cache which will be free at the by label out_free_cachep.
>
Yeah, I found the snippet of code which searches for the pcache with
the same level.
46 list_for_each_entry(pcache, &pid_caches_lh, list)
47 if (pcache->nr_ids == nr_ids)
48 goto out;
Regards,
Raphael S.Carvalho
On Tue, Mar 05, 2013 at 02:04:45AM -0300, Raphael S Carvalho wrote:
> >
> > Actually I noticed this problem and I think it is not a BUG.
> > Since the pid_cache is created for all pid namespace which have the same level.
> > Even this pid namespace is failed to create, the pid_cache will not be leaked, Other
> > pid namespace which has the same level will use the pid_cache and no need to
> > allocate it again. In other words, the pid_cache for every level pid namespace will
> > only be created once.
> >
> > I also think this patch add a bug,because there may be other pid namespace's pid_cachep
> > points to the same pid_cache which will be free at the by label out_free_cachep.
> >
Yup, drop this patch.
>
> Yeah, I found the snippet of code which searches for the pcache with
> the same level.
> 46 list_for_each_entry(pcache, &pid_caches_lh, list)
> 47 if (pcache->nr_ids == nr_ids)
> 48 goto out;