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

PATCH New Dan Carpenter 2 participants 5 messages Jan 26, 2015

Subsystems (2)

Memory management Slab allocator

lore

Share & tools

lore

Dan Carpenter

author

2015-01-26 08:56 UTC #0 lore raw

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

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  }

---
0-DAY kernel test infrastructure Open Source Technology Center
http://lists.01.org/mailman/listinfo/kbuild Intel Corporation

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Vladimir Davydov

2015-01-26 10:01 UTC #1 lore raw

PATCH 1/1 slab: update_memcg_params: explicitly check that old array != NULL

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 <[email protected]>

Signed-off-by Vladimir Davydov <[email protected]>

---
 mm/slab_common.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

mm/slab_common.c +5 -4

index bf4a42b2c5ba..0dd9eb4e0f87 100644
@@ -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)

Signature · 7 lines

1.7.10.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Dan Carpenter

author

2015-01-26 10:23 UTC #2 lore raw

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

Quoted 2 lines · Vladimir Davydov

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

Vladimir Davydov

2015-01-26 10:45 UTC #3 lore raw

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

Quoted 7 lines · Dan Carpenter

Quoted 2 lines · Vladimir Davydov
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.

Quoted 19 lines

//----- test code
void frob(void *p){}
struct foo {
	int *x, *y, *z;
};
int main(void)
{
	struct foo *x = NULL;

	frob(x->y);

	return 0;
}

Quoted 7 lines

//---- 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

Dan Carpenter

author

2015-01-26 15:14 UTC #4 lore raw

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

Quoted 11 lines · Vladimir Davydov

Quoted 7 lines · Dan Carpenter
Quoted 2 lines · Vladimir Davydov
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