[PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL

[PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL

[Vladimir Davydov]

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

Re: [PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL

[Dan Carpenter]

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

Re: [PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL

[Vladimir Davydov]

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

Re: [PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL

[Dan Carpenter]

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