[U-Boot] [PATCH v2] cmd: usb_mass_storage: add protection for block_dev

[Marek Vasut]

On 10/02/2018 09:54 AM, Patrick DELAUNAY wrote:
> Hi Marek,

Hi,

>> From: Marek Vasut <marex at denx.de>
>> Sent: dimanche 30 septembre 2018 10:09
>>
>> On 09/28/2018 11:30 AM, Patrick DELAUNAY wrote:
>>> Hi,
>>
>> Hi,
>>
>>>> From: Marek Vasut <marex at denx.de>
>>>>
>>>> On 09/26/2018 01:04 PM, Patrick Delaunay wrote:
>>>>> solve data abort for the command "ums 0 ubi 0"
>>>>> because result of case blk_get_device_part_str() result is OK but
>>>>> with block_dev = 0 when CONFIG_CMD_UBIFS is activate and ubi volume
>>>>> is mounted
>>>>
>>>> I don't quite understand the commit message, can you reword it?
>>>
>>> Ok, I prepare a V3
>>
>> Sending a V3 while discussion is ongoing is pointless.
> 
> Sorry, I already sent it Friday only with more clear commit message.
> Because I really ashamed by of my this fist comment... 
> Even me,  I can't understand that I wrote.

Happens to everyone.

>>>> Also, when is this ever called with block_dev == NULL ? What's the
>>>> condition to trigger this and why ?
>>>
>>> To reproduce the issue you need to have a ubifs  already mounted, for
>>> example with the command:
>>> 	ubi part UBI
>>>  	ubifsmount ubi0:boot
>>>
>>> I investigate the point, the call stack before the crash is  caused by
>>> the ubi support in
>>> ./disk/part.c:425 =  blk_get_device_part_str()
>>>
>>> Some part of code here are under CONFIG_CMD_UBIFS with the comment :
>>> 	"ubi goes through a mtd, rathen then through a regular block device"
>>>
>>> So the the function return a pointer to disk_partition_t :
>>> 		info->type = BOOT_PART_TYPE
>>> 		info->name =  "UBI"
>>>  but without block device information !
>>> 		*dev_desc = NULL
>>
>> Would it rather make sense to fix it here , so it has a valid dev_desc ?
> 
> I am not  confident that can be done easily,  if I correctly understood the code,  because
> UBI is not really a regular block device but only directly connected to MTD to allow 
> simple access to generic fs support for ubifs volume.
> 
> So NULL block devicedescriptor  is "normal" for UBIFS.
> 
> To solve the issue at this level, a fake block device should be implemented for the UBI case....
> And I don't sure that I can propose something here without breaking all the other commands
> using blk_get_device_part_str () function.
>  
> See  in ./fs/fs.c, the null dev desc is allowed for UBI
> 
> struct fstype_info fstypes[] 
> 
> #ifdef CONFIG_CMD_UBIFS
> 	{
> 		.fstype = FS_TYPE_UBIFS,
> 		.name = "ubifs",
> 		.null_dev_desc_ok = true,
> 
>>> So the issue occurs because, when the ubifs volume is mounted, The
>>> code in  cmd/usb_mass_storage.c
>> [...]
Thanks for clarifying.

Simon, thoughts ?

-- 
Best regards,
Marek Vasut