* [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak).
@ 2013-03-05 3:26 Eric W. Biederman
2013-03-05 3:51 ` Gao feng
0 siblings, 1 reply; 6+ messages in thread
From: Eric W. Biederman @ 2013-03-05 3:26 UTC (permalink / raw)
To: Andrew Morton, Oleg Nesterov, Cyrill Gorcunov, Serge Hallyn
Cc: linux-kernel, Raphael S.Carvalho
From: Raphael S.Carvalho <raphael.scarv@gmail.com>
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 <raphael.scarv@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak).
2013-03-05 3:26 [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak) Eric W. Biederman
@ 2013-03-05 3:51 ` Gao feng
2013-03-05 5:04 ` Raphael S Carvalho
0 siblings, 1 reply; 6+ messages in thread
From: Gao feng @ 2013-03-05 3:51 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Andrew Morton, Oleg Nesterov, Cyrill Gorcunov, Serge Hallyn,
linux-kernel
On 2013/03/05 11:26, Eric W. Biederman wrote:
> From: Raphael S.Carvalho <raphael.scarv@gmail.com>
>
> 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 <raphael.scarv@gmail.com>
> ---
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak).
2013-03-05 3:51 ` Gao feng
@ 2013-03-05 5:04 ` Raphael S Carvalho
2013-03-05 5:51 ` Cyrill Gorcunov
0 siblings, 1 reply; 6+ messages in thread
From: Raphael S Carvalho @ 2013-03-05 5:04 UTC (permalink / raw)
To: Gao feng
Cc: Andrew Morton, Oleg Nesterov, Cyrill Gorcunov, Serge Hallyn,
linux-kernel
On Tue, Mar 5, 2013 at 12:51 AM, Gao feng <gaofeng@cn.fujitsu.com> wrote:
> On 2013/03/05 11:26, Eric W. Biederman wrote:
>> From: Raphael S.Carvalho <raphael.scarv@gmail.com>
>>
>> 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 <raphael.scarv@gmail.com>
>> ---
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak).
2013-03-05 5:04 ` Raphael S Carvalho
@ 2013-03-05 5:51 ` Cyrill Gorcunov
0 siblings, 0 replies; 6+ messages in thread
From: Cyrill Gorcunov @ 2013-03-05 5:51 UTC (permalink / raw)
To: Raphael S Carvalho
Cc: Gao feng, Andrew Morton, Oleg Nesterov, Serge Hallyn,
linux-kernel
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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak).
@ 2013-03-05 3:28 Raphael S.Carvalho
2013-03-05 6:32 ` Eric W. Biederman
0 siblings, 1 reply; 6+ messages in thread
From: Raphael S.Carvalho @ 2013-03-05 3:28 UTC (permalink / raw)
To: Eric W. Biederman, Andrew Morton, Oleg Nesterov, Cyrill Gorcunov,
Serge Hallyn
Cc: linux-kernel, Raphael S.Carvalho
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 <raphael.scarv@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak).
2013-03-05 3:28 Raphael S.Carvalho
@ 2013-03-05 6:32 ` Eric W. Biederman
0 siblings, 0 replies; 6+ messages in thread
From: Eric W. Biederman @ 2013-03-05 6:32 UTC (permalink / raw)
To: Raphael S.Carvalho
Cc: Andrew Morton, Oleg Nesterov, Cyrill Gorcunov, Serge Hallyn,
linux-kernel
"Raphael S.Carvalho" <raphael.scarv@gmail.com> writes:
> 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!
pid caches are not per namespace. There are per pid namespace depth
and shared among many pid namespaces so in general a leak is fine.
We definitely can't do what you are doing. There are no checks that
another pid namespace doesn't have pids allocated from the pid cache
you are freeing nor any checks to see that the pid cache was allocated
uniquely per pid namespace.
Under the right circumstances you might be able to free pid caches
but it is hard to figure out what those circumstances are and I don't
expect it is worth the trouble.
Eric
> Signed-off-by: Raphael S.Carvalho <raphael.scarv@gmail.com>
> ---
> 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:
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-05 6:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-05 3:26 [PATCH 1/1] kernel/pid_namespace.c: Fixing a lack of cleanup (Probable resources leak) Eric W. Biederman
2013-03-05 3:51 ` Gao feng
2013-03-05 5:04 ` Raphael S Carvalho
2013-03-05 5:51 ` Cyrill Gorcunov
-- strict thread matches above, loose matches on Subject: below --
2013-03-05 3:28 Raphael S.Carvalho
2013-03-05 6:32 ` Eric W. Biederman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox