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

[Hou Tao <houtao1@xxxxxxxxxx>]

Hi,

On 2019/10/16 19:06, Richard Weinberger wrote:
> ----- 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 for your suggestions. I will try to use ubi+gluebi to confirm the problem.

Regards,
Tao
> Thanks,
> //richard
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 


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