Re: [PATCH] mtd: blkdevs: protect tr->devs list by mtd_table_mutex

[Hou Tao <houtao1@xxxxxxxxxx>]

ping ?

On 2019/9/29 22:11, Hou Tao wrote:
> There may be list corruption if there are concurrent list traversal
> and list deletion on tr->devs as showed in the following case:
> 
> CPU 0                               CPU 1
> 
> open /dev/mtdblock1
> 
> // remove mtd1
> blktrans_notify_remove()
>     del_mtd_blktrans_dev()
> 
> close /dev/mtdblock1
>   blktrans_release
>     blktrans_dev_put
>       acquire blktrans_ref_mutex     // remove mtd0
>       // the final release           acquire mtd_table_mutex
>       blktrans_dev_release()         blktrans_notify_remove()
>         // remove mtdblock1            // next is mtdblock1
>         list_del(&dev->list)           list_for_each_entry_safe()
> 
> We could fix the problem by acquiring blktrans_ref_mutex during
> the traversal of tr->devs, but blktrans_ref_mutex needs to be released
> before invoking tr->remote_dev(), so we also need to increase the kref
> of current device else the device may be freed and decrease the kref
> after the removal.
> 
> Or we could move the list deletion to del_mtd_blktrans_dev(), and protect
> the operations on tr->devs by mtd_table_mutex which has already be taken.
> 
> The latter fix is simpler. We also can remove the unnecessary acquisitions
> of blktrans_ref_mutex in add_mtd_blktrans_dev() because operations on
> tr->devs have already been protected by mtd_table_mutex.
> 
> Fixes: 048d87199566 ("mtd: blktrans: Hotplug fixes")
> Signed-off-by: Hou Tao <houtao1@xxxxxxxxxx>
> ---
> I found the problem by code review, and could not find a way to
> ensure the problem, because the removal of mtd devices almost
> comes from the removal of modules, and the open of /dev/mtdblockX
> will prevent the module from removing.
> ---
>  drivers/mtd/mtd_blkdevs.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index 0c05f77f9b21..e04260237a25 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -35,7 +35,6 @@ static void blktrans_dev_release(struct kref *kref)
>  	blk_mq_free_tag_set(dev->tag_set);
>  	kfree(dev->tag_set);
>  	put_disk(dev->disk);
> -	list_del(&dev->list);
>  	kfree(dev);
>  }
>  
> @@ -350,7 +349,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
>  		BUG();
>  	}
>  
> -	mutex_lock(&blktrans_ref_mutex);
>  	list_for_each_entry(d, &tr->devs, list) {
>  		if (new->devnum == -1) {
>  			/* Use first free number */
> @@ -362,7 +360,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
>  			}
>  		} else if (d->devnum == new->devnum) {
>  			/* Required number taken */
> -			mutex_unlock(&blktrans_ref_mutex);
>  			return -EBUSY;
>  		} else if (d->devnum > new->devnum) {
>  			/* Required number was free */
> @@ -381,14 +378,12 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
>  	 * with this number. */
>  	if (new->devnum > (MINORMASK >> tr->part_bits) ||
>  	    (tr->part_bits && new->devnum >= 27 * 26)) {
> -		mutex_unlock(&blktrans_ref_mutex);
>  		goto error1;
>  	}
>  
>  	list_add_tail(&new->list, &tr->devs);
> - added:
> -	mutex_unlock(&blktrans_ref_mutex);
>  
> + added:
>  	mutex_init(&new->lock);
>  	kref_init(&new->ref);
>  	if (!tr->writesect)
> @@ -484,6 +479,8 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old)
>  		BUG();
>  	}
>  
> +	list_del(&old->list);
> +
>  	if (old->disk_attributes)
>  		sysfs_remove_group(&disk_to_dev(old->disk)->kobj,
>  						old->disk_attributes);
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/