[Linux-kernel] [PATCH 1/1] mtd: mtdblock: avoid __might_sleep warnings in mtd_erase

[Ben Dooks]

On 26/04/17 19:18, Ben Hutchings wrote:
> On Wed, 2017-04-26 at 18:46 +0100, Ben Dooks wrote:
>> The mtd_erase() call can hit code that will trigger warnings
>> from __might_sleep(), such as the do_erase_oneblock() function
>> on the cfi_cmdset_0002.c file.
>>
>> This is due to some of the erase functions doing the work in the
>> thread they are called in, which means that the erase_write()
>> should only go into TASK_INTERRUPTIBLE once the mtd_erase call
>> has returned.
> [...]
>> diff --git a/drivers/mtd/mtdblock.c b/drivers/mtd/mtdblock.c
>> index bb4c14f83c75..4b1cd464f919 100644
>> --- a/drivers/mtd/mtdblock.c
>> +++ b/drivers/mtd/mtdblock.c
>> @@ -68,6 +68,7 @@ static int erase_write (struct mtd_info *mtd, unsigned long pos,
>>  	DECLARE_WAITQUEUE(wait, current);
>>  	wait_queue_head_t wait_q;
>>  	size_t retlen;
>> +	long timeout = 1;
>>  	int ret;
>>
>>  	/*
>> @@ -81,12 +82,10 @@ static int erase_write (struct mtd_info *mtd, unsigned long pos,
>>  	erase.len = len;
>>  	erase.priv = (u_long)&wait_q;
>>
>> -	set_current_state(TASK_INTERRUPTIBLE);
>>  	add_wait_queue(&wait_q, &wait);
>>
>>  	ret = mtd_erase(mtd, &erase);
>>  	if (ret) {
>> -		set_current_state(TASK_RUNNING);
>>  		remove_wait_queue(&wait_q, &wait);
>>  		printk (KERN_WARNING "mtdblock: erase of region [0x%lx, 0x%x] "
>>  				     "on \"%s\" failed\n",
>> @@ -94,8 +93,18 @@ static int erase_write (struct mtd_info *mtd, unsigned long pos,
>>  		return ret;
>>  	}
>>
>> -	schedule();  /* Wait for erase to finish. */
>> +	if (erase->state != MTD_ERASE_DONE &&
>> +	    erase->state != MTD_ERASE_FAILED)
>> +		timeout = wait_woken(&wait, TASK_INTERRUPTIBLE,
>> +				     MAX_SCHEDULE_TIMEOUT);
>
> If mtd_erase() returns 0 then the wait queue either has been woken or
> will be woken.  Since we're already on the wait queue, it's safe to wait
> unconditionally.
>
> I think that making the wait conditional results in a race condition
> that could result in returning too early.
>
> Also there seems to be another existing problem here: if this is
> interrupted and we return early then the driver can use-after-free the
> wait queue and erase structure.  mtdchar waits uninterruptibly for
> exactly this reason.
>
> We really ought to have an always-synchronous wrapper for mtd_erase(),
> because this seems to be hard to get right...
>
> Ben.

Ok, so add something like mtd_erase_sync() which uses mtd_erase and then
waits for the completion and then use it for both mtdblock and mtdchar?

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius