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

[Richard Weinberger <richard@xxxxxx>]

----- Ursprüngliche Mail -----
> Von: "Hou Tao" <houtao1@xxxxxxxxxx>
> An: "Richard Weinberger" <richard.weinberger@xxxxxxxxx>
> CC: "linux-mtd" <linux-mtd@xxxxxxxxxxxxxxxxxxx>, "Miquel Raynal" <miquel.raynal@xxxxxxxxxxx>, "Vignesh Raghavendra"
> <vigneshr@xxxxxx>, "richard" <richard@xxxxxx>, "Maxim Levitsky" <maximlevitsky@xxxxxxxxx>, "Marek Vasut"
> <marek.vasut@xxxxxxxxx>, "Brian Norris" <computersforpeace@xxxxxxxxx>, "David Woodhouse" <dwmw2@xxxxxxxxxxxxx>
> Gesendet: Mittwoch, 16. Oktober 2019 12:55:12
> Betreff: Re: [PATCH] mtd: blkdevs: protect tr->devs list by mtd_table_mutex

> Hi,
> 
> On 2019/10/16 4:10, Richard Weinberger wrote:
>> On Sun, Sep 29, 2019 at 4:05 PM Hou Tao <houtao1@xxxxxxxxxx> 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.
>> 
>> I'm confused. Can the problem only happen if you remove a mtd while
>> it is open?
>> 
> No. The problem may happen when closing a mtd block device (instead of
> the mtd char device) for which its mtd device had already been removed.
> 
> The reason why I can not confirm the problem is that I am trying to
> confirm the problem by the following steps:
> (1) insmod block2mtd.ko to create a mtd device (e.g., /dev/mtd0)
> (2) open /dev/mtdblock0
> (3) remove /dev/mtd0 by removing block2mtd.ko
> 
> step (3) always fails because the opening of /dev/mtdblock0 has already
> increased the reference count of block2mtd.ko, so /dev/mtd0 can not be removed.

Ok. But yeah, the problem is real and I'm sure with ubi+gluebi it can be triggered.

Thanks,
//richard

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