[bug report] block: Move bdi_unregister() to del_gendisk()

[bug report] block: Move bdi_unregister() to del_gendisk()

[Dan Carpenter]

Hello Jan Kara,

The patch 165a5e22fafb: "block: Move bdi_unregister() to
del_gendisk()" from Feb 8, 2017, leads to the following static
checker warning:

	drivers/block/rbd.c:4117 rbd_free_disk()
	warn: variable dereferenced before check 'disk->queue' (see line 4116)

drivers/block/rbd.c
  4107  static void rbd_free_disk(struct rbd_device *rbd_dev)
  4108  {
  4109          struct gendisk *disk = rbd_dev->disk;
  4110  
  4111          if (!disk)
  4112                  return;
  4113  
  4114          rbd_dev->disk = NULL;
  4115          if (disk->flags & GENHD_FL_UP) {
  4116                  del_gendisk(disk);
                        ^^^^^^^^^^^^^^^^^
The patch introduces a new dereference inside this function call.

  4117                  if (disk->queue)
                            ^^^^^^^^^^^
Check is too late.

  4118                          blk_cleanup_queue(disk->queue);
  4119                  blk_mq_free_tag_set(&rbd_dev->tag_set);
  4120          }
  4121          put_disk(disk);
  4122  }

regards,
dan carpenter

Re: [bug report] block: Move bdi_unregister() to del_gendisk()

[Dan Carpenter]

Also:

    arch/powerpc/sysdev/axonram.c:277 axon_ram_probe()
    error: we previously assumed 'bank->disk->queue' could be null (see line 231)

regards,
dan carpenter

Re: [bug report] block: Move bdi_unregister() to del_gendisk()

[Jan Kara]

On Tue 07-03-17 13:24:55, Dan Carpenter wrote:
> Also:
> 
>     arch/powerpc/sysdev/axonram.c:277 axon_ram_probe()
>     error: we previously assumed 'bank->disk->queue' could be null (see line 231)

So this is real however the error path of axon_ram_probe() looks hosed. You
are not supposed to call del_gendisk() if you did not call
device_add_disk() before and axon_ram_probe() leaks the gendisk structure
because it forgets to call put_disk(). I'll look into fixing this.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [bug report] block: Move bdi_unregister() to del_gendisk()

[Jan Kara]

Hi,

thanks for report! So don't see a way how rbd could pass gendisk with NULL
disk->queue to del_gendisk(). Also blk_unregister_queue() just after the
dereference I've added does WARN_ON(!disk->queue) so we would know about
such case if it could happen for whatever caller of del_gendisk(). However
for future safety, I could add a check for disk->queue being non-NULL to
del_gendisk().

								Honza

On Tue 07-03-17 03:13:22, Dan Carpenter wrote:
> Hello Jan Kara,
> 
> The patch 165a5e22fafb: "block: Move bdi_unregister() to
> del_gendisk()" from Feb 8, 2017, leads to the following static
> checker warning:
> 
> 	drivers/block/rbd.c:4117 rbd_free_disk()
> 	warn: variable dereferenced before check 'disk->queue' (see line 4116)
> 
> drivers/block/rbd.c
>   4107  static void rbd_free_disk(struct rbd_device *rbd_dev)
>   4108  {
>   4109          struct gendisk *disk = rbd_dev->disk;
>   4110  
>   4111          if (!disk)
>   4112                  return;
>   4113  
>   4114          rbd_dev->disk = NULL;
>   4115          if (disk->flags & GENHD_FL_UP) {
>   4116                  del_gendisk(disk);
>                         ^^^^^^^^^^^^^^^^^
> The patch introduces a new dereference inside this function call.
> 
>   4117                  if (disk->queue)
>                             ^^^^^^^^^^^
> Check is too late.
> 
>   4118                          blk_cleanup_queue(disk->queue);
>   4119                  blk_mq_free_tag_set(&rbd_dev->tag_set);
>   4120          }
>   4121          put_disk(disk);
>   4122  }
> 
> regards,
> dan carpenter
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR