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
> 

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

[Gu Zheng]

this modification can fix the is issue in commit
857814ee65dbc942b18b2dc713124043035e "mtd: fix: avoid race condition
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
index 66a9ded..f3d5470 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -84,6 +84,8 @@ static DEFINE_IDR(mtd_idr);
should not use them for _anything_ else */
 DEFINE_MUTEX(mtd_table_mutex);
 EXPORT_SYMBOL_GPL(mtd_table_mutex);
+int mtd_table_mutex_depth;
+struct task_struct *mtd_table_mutex_owner;
 
 struct mtd_info *__mtd_next_device(int i)
 {
@@ -96,6 +98,42 @@ static LIST_HEAD(mtd_notifiers);
 

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

[Gu Zheng]

this modification can fix the is issue in commit
857814ee65dbc942b18b2dc713124043035e "mtd: fix: avoid race condition
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
index 66a9ded..f3d5470 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -84,6 +84,8 @@ static DEFINE_IDR(mtd_idr);
should not use them for _anything_ else */
 DEFINE_MUTEX(mtd_table_mutex);
 EXPORT_SYMBOL_GPL(mtd_table_mutex);
+int mtd_table_mutex_depth;
+struct task_struct *mtd_table_mutex_owner;
 
 struct mtd_info *__mtd_next_device(int i)
 {
@@ -96,6 +98,42 @@ static LIST_HEAD(mtd_notifiers);
 
 #define