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

From: Alexander Sverdlin
Date: Mon Mar 20 2017 - 05:51:46 EST

Next message: Pablo Neira Ayuso: "Re: [PATCH] netfilter: fix the warning on unused refcount variable"
Previous message: Maxime Ripard: "Re: [PATCH] clk: sunxi: ccu-sun5i needs nkmp"
In reply to: Gu Zheng: "[PATCH] mtd:avoid blktrans_open/release race and avoid insmod ftl.ko deadlock"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello!

On 17/03/17 02:33, Gu Zheng wrote:
> this modification can fix the is issue in commit
> 857814ee65dbc942b18b2dc713124ffff043035e "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(&mtd_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(&mtd_table_mutex) //dead lock
>
> this patch can prevent some mtd_table_mutex lock race undiscovered.
>
> Signed-off-by: Gu Zheng <guzheng1@xxxxxxxxxx>
> ---
> 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(&mtd_table_mutex);
> + mtd_table_mutex_lock();
> mutex_lock(&dev->lock);
>
> if (dev->open)
> @@ -217,7 +217,7 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
> unlock:
> dev->open++;
> mutex_unlock(&dev->lock);
> - mutex_unlock(&mtd_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(&dev->ref, blktrans_dev_release);
> mutex_unlock(&dev->lock);
> - mutex_unlock(&mtd_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(&mtd_table_mutex);
> + mtd_table_mutex_lock();
> mutex_lock(&dev->lock);
>
> if (--dev->open)
> @@ -256,7 +256,7 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode)
> }
> unlock:
> mutex_unlock(&dev->lock);
> - mutex_unlock(&mtd_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(&mtd_table_mutex)) {
> - mutex_unlock(&mtd_table_mutex);
> - BUG();
> - }
> + mtd_table_assert_mutex_locked();
>
> mutex_lock(&blktrans_ref_mutex);
> list_for_each_entry(d, &tr->devs, list) {
> @@ -455,11 +452,7 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)
> {
> unsigned long flags;
>
> - if (mutex_trylock(&mtd_table_mutex)) {
> - mutex_unlock(&mtd_table_mutex);
> - BUG();
> - }
> -
> + mtd_table_assert_mutex_locked();
> if (old->disk_attributes)
> sysfs_remove_group(&disk_to_dev(old->disk)->kobj,
> old->disk_attributes);
> @@ -531,13 +524,13 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
> register_mtd_user(&blktrans_notifier);
>
>
> - mutex_lock(&mtd_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(&mtd_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(&mtd_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(&mtd_table_mutex);
> + mtd_table_mutex_lock();
>
> /* Remove it from the list of active majors */
> list_del(&tr->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(&mtd_table_mutex);
> + mtd_table_mutex_unlock();
>
> BUG_ON(!list_empty(&tr->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 MTD_DEVT(index) MKDEV(MTD_CHAR_MAJOR, (index)*2)
>
> +void mtd_table_mutex_lock(void)
> +{
> + if (mtd_table_mutex_owner != current) {
> + mutex_lock(&mtd_table_mutex);
> + mtd_table_mutex_owner = current;
> + }
> + mtd_table_mutex_depth++;
> +
> +}
> +EXPORT_SYMBOL_GPL(mtd_table_mutex_lock);
> +
> +
> +void mtd_table_mutex_unlock(void)
> +{
> + if (mtd_table_mutex_owner != current) {
> + pr_err("MTD:lock_owner is %s, but current is %s\n",
> + mtd_table_mutex_owner->comm, current->comm);
> + BUG();
> + }
> + if (--mtd_table_mutex_depth == 0) {
> + mtd_table_mutex_owner = NULL;
> + mutex_unlock(&mtd_table_mutex);
> + }
> +
> +}
> +EXPORT_SYMBOL_GPL(mtd_table_mutex_unlock);
> +
> +void mtd_table_assert_mutex_locked(void)
> +{
> + if (mtd_table_mutex_owner != current) {
> + pr_err("MTD:lock_owner is %s, but current is %s\n",
> + mtd_table_mutex_owner->comm, current->comm);
> + BUG();
> + }
> +}
> +EXPORT_SYMBOL_GPL(mtd_table_assert_mutex_locked);
> /* REVISIT once MTD uses the driver model better, whoever allocates
> * the mtd_info will probably want to use the release() hook...
> */
> @@ -502,7 +540,7 @@ int add_mtd_device(struct mtd_info *mtd)
> mtd->backing_dev_info = mtd_bdi;
>
> BUG_ON(mtd->writesize == 0);
> - mutex_lock(&mtd_table_mutex);
> + mtd_table_mutex_lock();
>
> i = idr_alloc(&mtd_idr, mtd, 0, 0, GFP_KERNEL);
> if (i < 0) {
> @@ -563,7 +601,7 @@ int add_mtd_device(struct mtd_info *mtd)
> list_for_each_entry(not, &mtd_notifiers, list)
> not->add(mtd);
>
> - mutex_unlock(&mtd_table_mutex);
> + mtd_table_mutex_unlock();
> /* We _know_ we aren't being removed, because
> our caller is still holding us here. So none
> of this try_ nonsense, and no bitching about it
> @@ -575,7 +613,7 @@ int add_mtd_device(struct mtd_info *mtd)
> of_node_put(mtd_get_of_node(mtd));
> idr_remove(&mtd_idr, i);
> fail_locked:
> - mutex_unlock(&mtd_table_mutex);
> + mtd_table_mutex_unlock();
> return error;
> }
>
> @@ -594,7 +632,7 @@ int del_mtd_device(struct mtd_info *mtd)
> int ret;
> struct mtd_notifier *not;
>
> - mutex_lock(&mtd_table_mutex);
> + mtd_table_mutex_lock();
>
> if (idr_find(&mtd_idr, mtd->index) != mtd) {
> ret = -ENODEV;
> @@ -621,7 +659,7 @@ int del_mtd_device(struct mtd_info *mtd)
> }
>
> out_error:
> - mutex_unlock(&mtd_table_mutex);
> + mtd_table_mutex_unlock();
> return ret;
> }
>
> @@ -782,7 +820,7 @@ void register_mtd_user (struct mtd_notifier *new)
> {
> struct mtd_info *mtd;
>
> - mutex_lock(&mtd_table_mutex);
> + mtd_table_mutex_lock();
>
> list_add(&new->list, &mtd_notifiers);
>
> @@ -791,7 +829,7 @@ void register_mtd_user (struct mtd_notifier *new)
> mtd_for_each_device(mtd)
> new->add(mtd);
>
> - mutex_unlock(&mtd_table_mutex);
> + mtd_table_mutex_unlock();
> }
> EXPORT_SYMBOL_GPL(register_mtd_user);
>
> @@ -808,7 +846,7 @@ int unregister_mtd_user (struct mtd_notifier *old)
> {
> struct mtd_info *mtd;
>
> - mutex_lock(&mtd_table_mutex);
> + mtd_table_mutex_lock();
>
> module_put(THIS_MODULE);
>
> @@ -816,7 +854,7 @@ int unregister_mtd_user (struct mtd_notifier *old)
> old->remove(mtd);
>
> list_del(&old->list);
> - mutex_unlock(&mtd_table_mutex);
> + mtd_table_mutex_unlock();
> return 0;
> }
> EXPORT_SYMBOL_GPL(unregister_mtd_user);
> @@ -837,7 +875,7 @@ struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num)
> struct mtd_info *ret = NULL, *other;
> int err = -ENODEV;
>
> - mutex_lock(&mtd_table_mutex);
> + mtd_table_mutex_lock();
>
> if (num == -1) {
> mtd_for_each_device(other) {
> @@ -861,7 +899,7 @@ struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num)
> if (err)
> ret = ERR_PTR(err);
> out:
> - mutex_unlock(&mtd_table_mutex);
> + mtd_table_mutex_unlock();
> return ret;
> }
> EXPORT_SYMBOL_GPL(get_mtd_device);
> @@ -900,7 +938,7 @@ struct mtd_info *get_mtd_device_nm(const char *name)
> int err = -ENODEV;
> struct mtd_info *mtd = NULL, *other;
>
> - mutex_lock(&mtd_table_mutex);
> + mtd_table_mutex_lock();
>
> mtd_for_each_device(other) {
> if (!strcmp(name, other->name)) {
> @@ -916,20 +954,20 @@ struct mtd_info *get_mtd_device_nm(const char *name)
> if (err)
> goto out_unlock;
>
> - mutex_unlock(&mtd_table_mutex);
> + mtd_table_mutex_unlock();
> return mtd;
>
> out_unlock:
> - mutex_unlock(&mtd_table_mutex);
> + mtd_table_mutex_unlock();
> return ERR_PTR(err);
> }
> EXPORT_SYMBOL_GPL(get_mtd_device_nm);
>
> void put_mtd_device(struct mtd_info *mtd)
> {
> - mutex_lock(&mtd_table_mutex);
> + mtd_table_mutex_lock();
> __put_mtd_device(mtd);
> - mutex_unlock(&mtd_table_mutex);
> + mtd_table_mutex_unlock();
>
> }
> EXPORT_SYMBOL_GPL(put_mtd_device);
> @@ -1744,13 +1782,13 @@ static int mtd_proc_show(struct seq_file *m, void *v)
> struct mtd_info *mtd;
>
> seq_puts(m, "dev: size erasesize name\n");
> - mutex_lock(&mtd_table_mutex);
> + mtd_table_mutex_lock();
> mtd_for_each_device(mtd) {
> seq_printf(m, "mtd%d: %8.8llx %8.8x \"%s\"\n",
> mtd->index, (unsigned long long)mtd->size,
> mtd->erasesize, mtd->name);
> }
> - mutex_unlock(&mtd_table_mutex);
> + mtd_table_mutex_unlock();
> return 0;
> }
>
> diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
> index 55fdb8e..cb1d8fa 100644
> --- a/drivers/mtd/mtdcore.h
> +++ b/drivers/mtd/mtdcore.h
> @@ -3,7 +3,6 @@
> * You should not use them for _anything_ else.
> */
>
> -extern struct mutex mtd_table_mutex;
>
> struct mtd_info *__mtd_next_device(int i);
> int add_mtd_device(struct mtd_info *mtd);
> @@ -21,6 +20,9 @@ void mtd_part_parser_cleanup(struct mtd_partitions *parts);
>
> int __init init_mtdchar(void);
> void __exit cleanup_mtdchar(void);
> +extern void mtd_table_mutex_lock(void);
> +extern void mtd_table_mutex_unlock(void);
> +extern void mtd_table_assert_mutex_locked(void);
>
> #define mtd_for_each_device(mtd) \
> for ((mtd) = __mtd_next_device(0); \
>

--
Best regards,
Alexander Sverdlin.

Next message: Pablo Neira Ayuso: "Re: [PATCH] netfilter: fix the warning on unused refcount variable"
Previous message: Maxime Ripard: "Re: [PATCH] clk: sunxi: ccu-sun5i needs nkmp"
In reply to: Gu Zheng: "[PATCH] mtd:avoid blktrans_open/release race and avoid insmod ftl.ko deadlock"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]