* [mmotm:master 200/417] mm/slab_common.c:166 update_memcg_params() warn: variable dereferenced before check 'old' (see line 162) @ 2015-01-26 8:56 Dan Carpenter 2015-01-26 10:01 ` [PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL Vladimir Davydov 0 siblings, 1 reply; 5+ messages in thread From: Dan Carpenter @ 2015-01-26 8:56 UTC (permalink / raw) To: kbuild, Vladimir Davydov Cc: Johannes Weiner, Andrew Morton, Linux Memory Management List, Dan Carpenter 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 majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH -mm] slab: update_memcg_params: explicitly check that old array != NULL 2015-01-26 8:56 [mmotm:master 200/417] mm/slab_common.c:166 update_memcg_params() warn: variable dereferenced before check 'old' (see line 162) Dan Carpenter @ 2015-01-26 10:01 ` Vladimir Davydov 2015-01-26 10:23 ` Dan Carpenter 0 siblings, 1 reply; 5+ 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 5+ 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; 5+ 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 5+ 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; 5+ 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 5+ 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; 5+ 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-01-26 15:14 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-01-26 8:56 [mmotm:master 200/417] mm/slab_common.c:166 update_memcg_params() warn: variable dereferenced before check 'old' (see line 162) Dan Carpenter 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).