re: blk-mq: cleanup after blk_mq_init_rq_map failures

re: blk-mq: cleanup after blk_mq_init_rq_map failures

[Dan Carpenter]

Hello Robert Elliott,

The patch 5676e7b6db02: "blk-mq: cleanup after blk_mq_init_rq_map
failures" from Sep 2, 2014, leads to the following static checker
warning:

	block/blk-mq.c:2041 blk_mq_alloc_tag_set()
	warn: calling kfree() when 'set->tags' is always NULL.

block/blk-mq.c
  2027          set->tags = kmalloc_node(set->nr_hw_queues *
  2028                                   sizeof(struct blk_mq_tags *),
  2029                                   GFP_KERNEL, set->numa_node);
  2030          if (!set->tags)
  2031                  return -ENOMEM;
  2032  
  2033          if (blk_mq_alloc_rq_maps(set))
  2034                  goto enomem;
  2035  
  2036          mutex_init(&set->tag_list_lock);
  2037          INIT_LIST_HEAD(&set->tag_list);
  2038  
  2039          return 0;
  2040  enomem:
  2041          kfree(set->tags);
  2042          set->tags = NULL;
  2043          return -ENOMEM;
  2044  }

The problem with this code is that __blk_mq_alloc_rq_maps() sets
"set->tags" to NULL.  It's a layering violation and actually it will
probably Oops before we hit the goto.  Let's take a look:

block/blk-mq.c
  1948  static int __blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
  1949  {
  1950          int i;
  1951  
  1952          for (i = 0; i < set->nr_hw_queues; i++) {
  1953                  set->tags[i] = blk_mq_init_rq_map(set, i);
  1954                  if (!set->tags[i])
  1955                          goto out_unwind;
  1956          }
  1957  
  1958          return 0;
  1959  
  1960  out_unwind:
  1961          while (--i >= 0)
  1962                  blk_mq_free_rq_map(set, set->tags[i], i);
  1963  
  1964          set->tags = NULL;
                ^^^^^^^^^^^^^^^^
Set to NULL here.

  1965          return -ENOMEM;
  1966  }
  1967  
  1968  /*
  1969   * Allocate the request maps associated with this tag_set. Note that this
  1970   * may reduce the depth asked for, if memory is tight. set->queue_depth
  1971   * will be updated to reflect the allocated depth.
  1972   */
  1973  static int blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set)
  1974  {
  1975          unsigned int depth;
  1976          int err;
  1977  
  1978          depth = set->queue_depth;
  1979          do {
  1980                  err = __blk_mq_alloc_rq_maps(set);
  1981                  if (!err)
  1982                          break;
  1983  
  1984                  set->queue_depth >>= 1;
  1985                  if (set->queue_depth < set->reserved_tags + BLK_MQ_TAG_MIN) {
  1986                          err = -ENOMEM;
  1987                          break;
  1988                  }
  1989          } while (set->queue_depth);
                  ^^^^^^^^^^^^^^^^^^^^^^^^
If we loop through this loop then the second call to
__blk_mq_alloc_rq_maps() will probably Oops.

  1990  

regards,
dan carpenter