LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL
       [not found] <20150126085638.GA6507@mwanda>
@ 2015-01-26 10:01 ` Vladimir Davydov
  2015-01-26 10:23   ` Dan Carpenter
  0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Davydov @ 2015-01-26 10:01 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Dan Carpenter, Johannes Weiner, Michal Hocko, Christoph Lameter,
	Pekka Enberg, David Rientjes, Joonsoo Kim, linux-mm,
	linux-kernel

tree:   git://git.cmpxchg.org/linux-mmotm.git master
head:   c64429bcc60a702f19f5cfdb5c39277863278a8c
commit: 5d06629c100b942a51f02b4d886c116ba3afb32a [200/417] slab: embed memcg_cache_params to kmem_cache

mm/slab_common.c:166 update_memcg_params() warn: variable dereferenced before check 'old' (see line 162)

git remote add mmotm git://git.cmpxchg.org/linux-mmotm.git
git remote update mmotm
git checkout 5d06629c100b942a51f02b4d886c116ba3afb32a
vim +/old +166 mm/slab_common.c

5d06629c Vladimir Davydov 2015-01-24  156                                       lockdep_is_held(&slab_mutex));
5d06629c Vladimir Davydov 2015-01-24  157       new = kzalloc(sizeof(struct memcg_cache_array) +
5d06629c Vladimir Davydov 2015-01-24  158                     new_array_size * sizeof(void *), GFP_KERNEL);
5d06629c Vladimir Davydov 2015-01-24  159       if (!new)
6f817f4c Vladimir Davydov 2014-10-09  160               return -ENOMEM;
6f817f4c Vladimir Davydov 2014-10-09  161
5d06629c Vladimir Davydov 2015-01-24 @162       memcpy(new->entries, old->entries,
88a0b848 Vladimir Davydov 2015-01-24  163              memcg_nr_cache_ids * sizeof(void *));
6f817f4c Vladimir Davydov 2014-10-09  164
5d06629c Vladimir Davydov 2015-01-24  165       rcu_assign_pointer(s->memcg_params.memcg_caches, new);
5d06629c Vladimir Davydov 2015-01-24 @166       if (old)
5d06629c Vladimir Davydov 2015-01-24  167               kfree_rcu(old, rcu);
6f817f4c Vladimir Davydov 2014-10-09  168       return 0;
6f817f4c Vladimir Davydov 2014-10-09  169  }

This warning is false-positive, because @old equals NULL iff
@memcg_nr_cache_ids equals 0. Moreover, this function had been acting in
exactly the same fashion before it was reworked by the culprit. Anyways,
let's add an explicit check if @old is not NULL before passing it to
@memcpy() to make static analysis tools happy.

fixes: slab-embed-memcg_cache_params-to-kmem_cache
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/slab_common.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/mm/slab_common.c b/mm/slab_common.c
index bf4a42b2c5ba..0dd9eb4e0f87 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -153,15 +153,16 @@ static int update_memcg_params(struct kmem_cache *s, int new_array_size)
 	if (!is_root_cache(s))
 		return 0;
 
-	old = rcu_dereference_protected(s->memcg_params.memcg_caches,
-					lockdep_is_held(&slab_mutex));
 	new = kzalloc(sizeof(struct memcg_cache_array) +
 		      new_array_size * sizeof(void *), GFP_KERNEL);
 	if (!new)
 		return -ENOMEM;
 
-	memcpy(new->entries, old->entries,
-	       memcg_nr_cache_ids * sizeof(void *));
+	old = rcu_dereference_protected(s->memcg_params.memcg_caches,
+					lockdep_is_held(&slab_mutex));
+	if (old)
+		memcpy(new->entries, old->entries,
+		       memcg_nr_cache_ids * sizeof(void *));
 
 	rcu_assign_pointer(s->memcg_params.memcg_caches, new);
 	if (old)
-- 
1.7.10.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL
  2015-01-26 10:01 ` [PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL Vladimir Davydov
@ 2015-01-26 10:23   ` Dan Carpenter
  2015-01-26 10:45     ` Vladimir Davydov
  0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2015-01-26 10:23 UTC (permalink / raw)
  To: Vladimir Davydov
  Cc: Andrew Morton, Johannes Weiner, Michal Hocko, Christoph Lameter,
	Pekka Enberg, David Rientjes, Joonsoo Kim, linux-mm,
	linux-kernel

On Mon, Jan 26, 2015 at 01:01:19PM +0300, Vladimir Davydov wrote:
> This warning is false-positive, because @old equals NULL iff
> @memcg_nr_cache_ids equals 0.

I don't see how it could be a false positive.  The "old" pointer is
dereferenced inside the call to memset() so unless memset is a macro the
compiler isn't going to optimize the dereference away.


//----- test code

void frob(void *p){}

struct foo {
	int *x, *y, *z;
};

int main(void)
{
	struct foo *x = NULL;

	frob(x->y);

	return 0;
}

//---- end


If we compile with gcc test.c then it segfaults.  With -02 the compiler
is able to tell that frob() is an empty function and it doesn't
segfault.  In the kernel code, there is no way for the compiler to
optimize the memset() away so it will Oops.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL
  2015-01-26 10:23   ` Dan Carpenter
@ 2015-01-26 10:45     ` Vladimir Davydov
  2015-01-26 15:13       ` Dan Carpenter
  0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Davydov @ 2015-01-26 10:45 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Andrew Morton, Johannes Weiner, Michal Hocko, Christoph Lameter,
	Pekka Enberg, David Rientjes, Joonsoo Kim, linux-mm,
	linux-kernel

On Mon, Jan 26, 2015 at 01:23:05PM +0300, Dan Carpenter wrote:
> On Mon, Jan 26, 2015 at 01:01:19PM +0300, Vladimir Davydov wrote:
> > This warning is false-positive, because @old equals NULL iff
> > @memcg_nr_cache_ids equals 0.
> 
> I don't see how it could be a false positive.  The "old" pointer is
> dereferenced inside the call to memset() so unless memset is a macro the
> compiler isn't going to optimize the dereference away.

old->entries is not dereferenced: memcg_cache_array->entries is not a
pointer - it is embedded to the memcg_cache_array struct.

> 
> 
> //----- test code
> 
> void frob(void *p){}
> 
> struct foo {
> 	int *x, *y, *z;
> };
> 
> int main(void)
> {
> 	struct foo *x = NULL;
> 
> 	frob(x->y);
> 
> 	return 0;
> }
> 
> //---- end
> 
> 
> If we compile with gcc test.c then it segfaults.  With -02 the compiler
> is able to tell that frob() is an empty function and it doesn't
> segfault.  In the kernel code, there is no way for the compiler to
> optimize the memset() away so it will Oops.

Just change

- 	int *x, *y, *z;
+	int *x, *z;
+	int *y[0];

and it won't.

Thanks,
Vladimir

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL
  2015-01-26 10:45     ` Vladimir Davydov
@ 2015-01-26 15:13       ` Dan Carpenter
  0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2015-01-26 15:13 UTC (permalink / raw)
  To: Vladimir Davydov
  Cc: Andrew Morton, Johannes Weiner, Michal Hocko, Christoph Lameter,
	Pekka Enberg, David Rientjes, Joonsoo Kim, linux-mm,
	linux-kernel

On Mon, Jan 26, 2015 at 01:45:34PM +0300, Vladimir Davydov wrote:
> On Mon, Jan 26, 2015 at 01:23:05PM +0300, Dan Carpenter wrote:
> > On Mon, Jan 26, 2015 at 01:01:19PM +0300, Vladimir Davydov wrote:
> > > This warning is false-positive, because @old equals NULL iff
> > > @memcg_nr_cache_ids equals 0.
> > 
> > I don't see how it could be a false positive.  The "old" pointer is
> > dereferenced inside the call to memset() so unless memset is a macro the
> > compiler isn't going to optimize the dereference away.
> 
> old->entries is not dereferenced: memcg_cache_array->entries is not a
> pointer - it is embedded to the memcg_cache_array struct.

Ah.  Ok.  Thanks.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-01-26 15:14 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20150126085638.GA6507@mwanda>
2015-01-26 10:01 ` [PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL Vladimir Davydov
2015-01-26 10:23   ` Dan Carpenter
2015-01-26 10:45     ` Vladimir Davydov
2015-01-26 15:13       ` Dan Carpenter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).