Re: [PATCHv5] UBI: new module ubiblk: block layer on top of UBI

[Artem Bityutskiy <dedekind1@gmail.com>]

Hi David,

some review below.

> +/**
> + * ubiblk_release - close a UBI volume (close the volume descriptor)
> + *
> + * @gd: the disk that was previously opened
> + * @mode: don't care
> + */
> +static int ubiblk_release(struct gendisk *gd, fmode_t mode)
> +{
> +	struct ubiblk_dev *dev = gd->private_data;
> +
> +	mutex_lock(&dev->vol_lock);
> +
> +	dev->refcnt--;
> +	if (dev->refcnt == 0) {
> +		kfree(dev->vi);
> +		dev->vi = NULL;
> +
> +		ubi_close_volume(dev->desc);
> +		dev->desc = NULL;
> +	}
> +
> +	dev_dbg(disk_to_dev(dev->gd), "released, mode=%d\n", mode);
> +
> +	mutex_unlock(&dev->vol_lock);
> +	return 0;
> +}

Looks buggy. I'd expect this function to  remove the device from the
list as well as free it, as well as hold the devlist_mutex...


...

> +/**
> + * ubiblk_create - create a ubiblk device proxying a UBI volume
> + *
> + * @vi: the UBI volume information data structure
> + *
> + * An UBI volume has been created ; create a corresponding ubiblk device:
> + * Initialize the locks, the structure, the block layer infos and start a
> + * req_task.
> + */
> +static int ubiblk_create(struct ubi_volume_info *vi)
> +{
> +	struct ubiblk_dev *dev;
> +	struct gendisk *gd;
> +	int ret = 0;
> +
> +	mutex_lock(&devlist_lock);
> +	/* Check that the volume isn't already proxyfied */
> +	if (ubiblk_find_dev(vi)) {
> +		ret = -EEXIST;
> +		goto out_devlist;
> +	}
> +
> +	dev = kzalloc(sizeof(struct ubiblk_dev), GFP_KERNEL);
> +	if (!dev) {
> +		ret = -ENOMEM;
> +		goto out_devlist;
> +	}
> +
> +	list_add(&dev->list, &ubiblk_devs);
> +
> +	mutex_init(&dev->vol_lock);
> +	mutex_lock(&dev->vol_lock);

I think upi do not need to take vol_lock. You are creating the volume,
and no one can open it anyway because you have 'devlist_lock' now. 
...

> +	set_capacity(gd,
> +		     (dev->vi->size *
> +		      dev->vi->usable_leb_size) >> 9);

A temporary variable would be neater.

> +	set_disk_ro(gd, 1);
> +	dev->gd = gd;
> +
> +	spin_lock_init(&dev->queue_lock);
> +	dev->rq = blk_init_queue(ubi_ubiblk_request, &dev->queue_lock);
> +	if (!dev->rq) {
> +		pr_err("blk_init_queue failed\n");
> +		ret = -ENODEV;
> +		goto out_queue;
> +	}
> +	dev->rq->queuedata = dev;
> +	blk_queue_logical_block_size(dev->rq, BLK_SIZE);
> +	dev->gd->queue = dev->rq;
> +
> +	/* Stolen from mtd_blkdevs.c */

s/Stolen/borrowed/ ?
...


> +/**
> + * ubiblk_remove - destroy a ubiblk device
> + *
> + * @vi: the UBI volume information data structure
> + *
> + * A UBI volume has been removed or we are requested to unproxify a volume ;
> + * destroy the corresponding ubiblk device
> + */
> +static int ubiblk_remove(struct ubi_volume_info *vi)
> +{
> +	struct ubiblk_dev *dev = NULL;
> +
> +	mutex_lock(&devlist_lock);
> +
> +	dev = ubiblk_find_dev(vi);
> +
> +	if (!dev) {
> +		mutex_unlock(&devlist_lock);
> +		pr_warn("trying to remove %s, but it isn't handled\n",
> +			vi->name);
> +		return -ENODEV;
> +	}
> +
> +	if (dev->desc) {
> +		mutex_unlock(&devlist_lock);
> +		return -EBUSY;
> +	}

Racy. Your dev->desc is protected by the "vol_lock", here you did not
take it, but you should. And then you can release it after
"list_del(&dev->list)".

> +
> +	del_gendisk(dev->gd);
> +	blk_cleanup_queue(dev->rq);
> +	kthread_stop(dev->req_task);
> +	put_disk(dev->gd);
> +
> +	kfree(dev->vi);
> +
> +	list_del(&dev->list);
> +	kfree(dev);
> +
> +	mutex_unlock(&devlist_lock);
> +	pr_info("unproxyfied %s\n", vi->name);
> +	return 0;
> +}

...

> +#ifdef CONFIG_COMPAT
> +static long ubiblk_ctrl_compat_ioctl(struct file *file, unsigned int cmd,
> +				     unsigned long arg)
> +{
> +	unsigned long translated_arg = (unsigned long)compat_ptr(arg);
> +
> +	return ubiblk_ctrl_ioctl(file, cmd, translated_arg);
> +}
> +#endif
> +
> +/* ubiblk control device (receives ioctls) */
> +static const struct file_operations ubiblk_ctrl_ops = {
> +	.owner = THIS_MODULE,
> +	.unlocked_ioctl = ubiblk_ctrl_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl = ubiblk_ctrl_compat_ioctl,
> +#endif
> +	.llseek = no_llseek,
> +};

You do not need compat ioctl. This is needed only for old code which
does nasty things like using pointers in ioctl data structures. The idea
is that new code uses proper ioctl data structures instead. Please, kill
this. 

...

> +static int __init ubi_ubiblk_init(void)
> +{
> +	int ret = 0;
> +
> +	ret = register_blkdev(0, "ubiblk");
> +	if (ret <= 0)
> +		return ret;

Error code "0" means success, so if you return 0 here, you'll confuse
the upper layers.

...

> +static void __exit ubi_ubiblk_exit(void)
> +{
> +	struct list_head *list_ptr, *next;
> +	struct ubiblk_dev *dev;
> +
> +	ubi_unregister_volume_notifier(&ubiblk_notifier);
> +	misc_deregister(&ctrl_dev);
> +
> +	list_for_each_safe(list_ptr, next, &ubiblk_devs) {
> +		dev = list_entry(list_ptr, struct ubiblk_dev, list);
> +
> +		/* TODO: it shouldn't happen, right ? */
> +		if (dev->desc)
> +			ubi_close_volume(dev->desc);

Since you are using mutexes now, can you remove the TODO line? If you
think "if (dev->desc)" is still needed, then please, add WARN_ON() in
the "else" part, or just kill the "if" part.

> +
> +		del_gendisk(dev->gd);
> +		blk_cleanup_queue(dev->rq);
> +		kthread_stop(dev->req_task);
> +		put_disk(dev->gd);
> +
> +		kfree(dev->vi);
> +		list_del(&dev->list); /* really needed ? */

Good question, I think we should not have comments like this in the
final driver.

> +		kfree(dev);
> +	}
> +
> +	unregister_blkdev(ubiblk_major, "ubiblk");
> +	pr_info("end of life\n");

This funny but let's remove it please.

> +}
> +
> +module_init(ubi_ubiblk_init);
> +module_exit(ubi_ubiblk_exit);
> +MODULE_DESCRIPTION("Read-only block transition layer on top of UBI");
> +MODULE_AUTHOR("David Wagner");
> +MODULE_LICENSE("GPL");

....

> +/**
> + * ubiblk_ctrl_req - additional ioctl data structure
> + * @ubi_num: UBI device number
> + * @vol_id: UBI volume identifier
    * @padding: reserved for future, must contain zeroes
> + */
> +struct ubiblk_ctrl_req {
> +	__s32 ubi_num;
> +	__s32 vol_id;
          __u8[8] padding;
> +} __packed;

Please, add paddings as suggested above.

-- 
Best Regards,
Artem Bityutskiy