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

[Ben Hutchings <ben.hutchings@codethink.co.uk>]

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.

>  	remove_wait_queue(&wait_q, &wait);
> +	if (timeout == 0) {
> +		printk (KERN_WARNING "mtdblock: erase of region [0x%lx, 0x%x] "
> +				     "on \"%s\" failed\n",
> +			pos, len, mtd->name);
> +		return -ETIMEDOUT;
> +	}
>  
>  	/*
>  	 * Next, write the data to flash.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.