Re: [PATCH] mtd:avoid blktrans_open/release race and avoid insmod ftl.ko deadlock

[Alexander Sverdlin]

Hello!

On 17/03/17 02:33, Gu Zheng wrote:
> this modification can fix the is issue in commit
> 857814ee65dbc942b18b2dc713124043035e "mtd: fix: avoid race condition

I would propose to revert both above commit and the original offender 008c751e,
which aims for questionable module removal relaxation, but for the costs of 
subtle
race which can only be solved introducing first recursive lock in the kernel.

> when accessing mtd->usecount ",also can solve the issue about it happen
> dealock when ismod ftl.ko. the original process is as follows:
> init_ftl
> register_mtd_blktrans
> mutex_lock(_table_mutex) //mtd_table_mutex locked
> ftl_add_mtd
> add_mtd_blktrans_dev
> device_add_disk
> register_disk
> blkdev_get
> __blkdev_get
> blktrans_open
> mutex_lock(_table_mutex) //dead lock
> 
> this patch can prevent some mtd_table_mutex lock race undiscovered.
> 
> Signed-off-by: Gu Zheng 
> ---
>  drivers/mtd/mtd_blkdevs.c | 31 
>  drivers/mtd/mtdcore.c | 74 
> +++
>  drivers/mtd/mtdcore.h |  4 ++-
>  3 files changed, 71 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index 6b8d5cd..c194208 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -191,7 +191,7 @@ static int blktrans_open(struct block_device *bdev, 
> fmode_t mode)
>   if (!dev)
>   return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/
>  
> - mutex_lock(_table_mutex);
> + mtd_table_mutex_lock();
>   mutex_lock(>lock);
>  
>   if (dev->open)
> @@ -217,7 +217,7 @@ static int blktrans_open(struct block_device *bdev, 
> fmode_t mode)
>  unlock:
>   dev->open++;
>   mutex_unlock(>lock);
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>   blktrans_dev_put(dev);
>   return ret;
>  
> @@ -228,7 +228,7 @@ static int blktrans_open(struct block_device *bdev, 
> fmode_t mode)
>   module_put(dev->tr->owner);
>   kref_put(>ref, blktrans_dev_release);
>   mutex_unlock(>lock);
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>   blktrans_dev_put(dev);
>   return ret;
>  }
> @@ -240,7 +240,7 @@ static void blktrans_release(struct gendisk *disk, 
> fmode_t mode)
>   if (!dev)
>   return;
>  
> - mutex_lock(_table_mutex);
> + mtd_table_mutex_lock();
>   mutex_lock(>lock);
>  
>   if (--dev->open)
> @@ -256,7 +256,7 @@ static void blktrans_release(struct gendisk *disk, 
> fmode_t mode)
>   }
>  unlock:
>   mutex_unlock(>lock);
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>   blktrans_dev_put(dev);
>  }
>  
> @@ -323,10 +323,7 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
>   struct gendisk *gd;
>   int ret;
>  
> - if (mutex_trylock(_table_mutex)) {
> - mutex_unlock(_table_mutex);
> - BUG();
> - }
> + mtd_table_assert_mutex_locked();
>  
>   mutex_lock(_ref_mutex);
>   list_for_each_entry(d, >devs, list) {
> @@ -455,11 +452,7 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)
>  {
>   unsigned long flags;
>  
> - if (mutex_trylock(_table_mutex)) {
> - mutex_unlock(_table_mutex);
> - BUG();
> - }
> -
> + mtd_table_assert_mutex_locked();
>   if (old->disk_attributes)
>   sysfs_remove_group(_to_dev(old->disk)->kobj,
>   old->disk_attributes);
> @@ -531,13 +524,13 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
>   register_mtd_user(_notifier);
>  
>  
> - mutex_lock(_table_mutex);
> + mtd_table_mutex_lock();
>  
>   ret = register_blkdev(tr->major, tr->name);
>   if (ret < 0) {
>   printk(KERN_WARNING "Unable to register %s block device on 
> major %d: %d\n",
>  tr->name, tr->major, ret);
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>   return ret;
>   }
>  
> @@ -553,7 +546,7 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
>   if (mtd->type != MTD_ABSENT)
>   tr->add_mtd(tr, mtd);
>  
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>   return 0;
>  }
>  
> @@ -561,7 +554,7 @@ int deregister_mtd_blktrans(struct mtd_blktrans_ops *tr)
>  {
>   struct mtd_blktrans_dev *dev, *next;
>  
> - mutex_lock(_table_mutex);
> + mtd_table_mutex_lock();
>  
>   /* Remove it from the list of active majors */
>   list_del(>list);
> @@ -570,7 +563,7 @@ int deregister_mtd_blktrans(struct mtd_blktrans_ops *tr)
>   tr->remove_dev(dev);
>  
>   unregister_blkdev(tr->major, tr->name);
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>  
>   BUG_ON(!list_empty(>devs));
>   return 0;
> diff --git a/drivers/mtd/mtdcore.c 

Re: [PATCH] mtd:avoid blktrans_open/release race and avoid insmod ftl.ko deadlock

[Alexander Sverdlin]

Hello!

On 17/03/17 02:33, Gu Zheng wrote:
> this modification can fix the is issue in commit
> 857814ee65dbc942b18b2dc713124043035e "mtd: fix: avoid race condition

I would propose to revert both above commit and the original offender 008c751e,
which aims for questionable module removal relaxation, but for the costs of 
subtle
race which can only be solved introducing first recursive lock in the kernel.

> when accessing mtd->usecount ",also can solve the issue about it happen
> dealock when ismod ftl.ko. the original process is as follows:
> init_ftl
> register_mtd_blktrans
> mutex_lock(_table_mutex) //mtd_table_mutex locked
> ftl_add_mtd
> add_mtd_blktrans_dev
> device_add_disk
> register_disk
> blkdev_get
> __blkdev_get
> blktrans_open
> mutex_lock(_table_mutex) //dead lock
> 
> this patch can prevent some mtd_table_mutex lock race undiscovered.
> 
> Signed-off-by: Gu Zheng 
> ---
>  drivers/mtd/mtd_blkdevs.c | 31 
>  drivers/mtd/mtdcore.c | 74 
> +++
>  drivers/mtd/mtdcore.h |  4 ++-
>  3 files changed, 71 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index 6b8d5cd..c194208 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -191,7 +191,7 @@ static int blktrans_open(struct block_device *bdev, 
> fmode_t mode)
>   if (!dev)
>   return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/
>  
> - mutex_lock(_table_mutex);
> + mtd_table_mutex_lock();
>   mutex_lock(>lock);
>  
>   if (dev->open)
> @@ -217,7 +217,7 @@ static int blktrans_open(struct block_device *bdev, 
> fmode_t mode)
>  unlock:
>   dev->open++;
>   mutex_unlock(>lock);
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>   blktrans_dev_put(dev);
>   return ret;
>  
> @@ -228,7 +228,7 @@ static int blktrans_open(struct block_device *bdev, 
> fmode_t mode)
>   module_put(dev->tr->owner);
>   kref_put(>ref, blktrans_dev_release);
>   mutex_unlock(>lock);
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>   blktrans_dev_put(dev);
>   return ret;
>  }
> @@ -240,7 +240,7 @@ static void blktrans_release(struct gendisk *disk, 
> fmode_t mode)
>   if (!dev)
>   return;
>  
> - mutex_lock(_table_mutex);
> + mtd_table_mutex_lock();
>   mutex_lock(>lock);
>  
>   if (--dev->open)
> @@ -256,7 +256,7 @@ static void blktrans_release(struct gendisk *disk, 
> fmode_t mode)
>   }
>  unlock:
>   mutex_unlock(>lock);
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>   blktrans_dev_put(dev);
>  }
>  
> @@ -323,10 +323,7 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
>   struct gendisk *gd;
>   int ret;
>  
> - if (mutex_trylock(_table_mutex)) {
> - mutex_unlock(_table_mutex);
> - BUG();
> - }
> + mtd_table_assert_mutex_locked();
>  
>   mutex_lock(_ref_mutex);
>   list_for_each_entry(d, >devs, list) {
> @@ -455,11 +452,7 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)
>  {
>   unsigned long flags;
>  
> - if (mutex_trylock(_table_mutex)) {
> - mutex_unlock(_table_mutex);
> - BUG();
> - }
> -
> + mtd_table_assert_mutex_locked();
>   if (old->disk_attributes)
>   sysfs_remove_group(_to_dev(old->disk)->kobj,
>   old->disk_attributes);
> @@ -531,13 +524,13 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
>   register_mtd_user(_notifier);
>  
>  
> - mutex_lock(_table_mutex);
> + mtd_table_mutex_lock();
>  
>   ret = register_blkdev(tr->major, tr->name);
>   if (ret < 0) {
>   printk(KERN_WARNING "Unable to register %s block device on 
> major %d: %d\n",
>  tr->name, tr->major, ret);
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>   return ret;
>   }
>  
> @@ -553,7 +546,7 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
>   if (mtd->type != MTD_ABSENT)
>   tr->add_mtd(tr, mtd);
>  
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>   return 0;
>  }
>  
> @@ -561,7 +554,7 @@ int deregister_mtd_blktrans(struct mtd_blktrans_ops *tr)
>  {
>   struct mtd_blktrans_dev *dev, *next;
>  
> - mutex_lock(_table_mutex);
> + mtd_table_mutex_lock();
>  
>   /* Remove it from the list of active majors */
>   list_del(>list);
> @@ -570,7 +563,7 @@ int deregister_mtd_blktrans(struct mtd_blktrans_ops *tr)
>   tr->remove_dev(dev);
>  
>   unregister_blkdev(tr->major, tr->name);
> - mutex_unlock(_table_mutex);
> + mtd_table_mutex_unlock();
>  
>   BUG_ON(!list_empty(>devs));
>   return 0;
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
>