On 11/9/18 6:17 PM, Dan Carpenter wrote:
> Hello Vlastimil Babka,
Hi,
> The patch 1291523f2c1d: "mm, slab/slub: introduce kmalloc-reclaimable
> caches" from Oct 26, 2018, leads to the following static checker
> warning:
>
> ./include/linux/slab.h:585 kmalloc_node()
> warn: array off by one? 'kmalloc_caches[kmalloc_type(flags)]' '0-3 == 3'
I believe that's a false positive.
> ./include/linux/slab.h
> 298 /*
> 299 * Whenever changing this, take care of that kmalloc_type() and
> 300 * create_kmalloc_caches() still work as intended.
> 301 */
> 302 enum kmalloc_cache_type {
> 303 KMALLOC_NORMAL = 0,
> 304 KMALLOC_RECLAIM,
> 305 #ifdef CONFIG_ZONE_DMA
> 306 KMALLOC_DMA,
> 307 #endif
> 308 NR_KMALLOC_TYPES
>
>
> The kmalloc_caches[] array has NR_KMALLOC_TYPES elements.
Yes.
> 309 };
> 310
> 311 #ifndef CONFIG_SLOB
> 312 extern struct kmem_cache *
> 313 kmalloc_caches[NR_KMALLOC_TYPES][KMALLOC_SHIFT_HIGH + 1];
> 314
> 315 static __always_inline enum kmalloc_cache_type kmalloc_type(gfp_t flags)
> 316 {
> 317 int is_dma = 0;
> 318 int type_dma = 0;
> 319 int is_reclaimable;
> 320
> 321 #ifdef CONFIG_ZONE_DMA
> 322 is_dma = !!(flags & __GFP_DMA);
> 323 type_dma = is_dma * KMALLOC_DMA;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> KMALLOC_DMA is the last possible valid index.
Yes, but type_dma gains the value of KMALLOC_DMA only when is_dma is 1.
> 324 #endif
> 325
> 326 is_reclaimable = !!(flags & __GFP_RECLAIMABLE);
> 327
> 328 /*
> 329 * If an allocation is both __GFP_DMA and __GFP_RECLAIMABLE, return
> 330 * KMALLOC_DMA and effectively ignore __GFP_RECLAIMABLE
> 331 */
> 332 return type_dma + (is_reclaimable & !is_dma) * KMALLOC_RECLAIM;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> We're adding one to it.
Only when !is_dma is 1, which means then that type_dma is 0. So it's safe.
> This is mm/ so I assume this works,
I'll... take that as a compliment :D
> but it's
> pretty confusing.
Indeed. Static checkers seem to hate my too clever code, so it's already
going away [1]. Maybe your static checker can be improved to evaluate
this better? There's already a gcc bug [2] inspired by the whole thing.
Thanks!
Vlastimil
[1]
https://lore.kernel.org/lkml/cbc1fc52-dc8c-aa38-8f29-22da8bcd91c1@xxxxxxx/T/#u
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87954
> 333 }
>
> regards,
> dan carpenter
>
