Re: [RFC PATCH 2/2] mtd: ubi: wl: avoid erasing a PEB which is empty

[Richard Weinberger <richard@nod.at>]

Am 24.11.2015 um 10:07 schrieb Sebastian Andrzej Siewior:
> On 11/24/2015 10:02 AM, Richard Weinberger wrote:
>> Am 24.11.2015 um 09:42 schrieb Sebastian Andrzej Siewior:
>>> On 11/24/2015 09:39 AM, Richard Weinberger wrote:
>>>>>>> +	} else {
>>>>>>> +		err = do_sync_erase(ubi, e2, vol_id, lnum, torture);
>>>>>>> +		if (err) {
>>>>>>> +			wl_entry_destroy(ubi, e2);
>>>>>>
>>>>>> Why that? The erase_worker will free e2 if it encounters
>>>>>> a fatal error and gives up this PEB. You're introducing a double free.
>>>>>
>>>>> Hmmm. That is real bad error handling you have there. So you invoke
>>>>> do_sync_erase(), the kmalloc() fails and how exactly you free e2 here?
>>>>
>>>> Why do you want to free e2? We free an erase entry only if we give
>>>> it up. wear leveling entries are allocated at init time and destroyed
>>>> when you detach UBI.
>>>
>>> The reference to it in the RB-tree (free) was removed. Is there another
>>> reference to it?
>>
>> UBI supports only single references.
>> Everything else would be a bug.
> 
> So if there is no reference to e2 which was just removed from the
> RB-tree free and do_sync_erase() can't kmalloc() then we leak e2,
> correct?

Yes, you are right. That definitely needs improvement.

A possible solution would be iterating over ubi->lookuptbl upon
detach time and call wl_entry_destroy() on every non-NULL entry.

...or rework do_sync_erase(), currently it calls the erase worker directly.
The erase worker destroys a failed wl entry upon failure. But from the return code
of do_sync_erase() we cannot know whether the erase worker destroyed it already.

Thanks,
//richard