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

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

On Thu, 2017-04-27 at 09:27 +0100, Ben Dooks wrote:
> 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?

...and any other callers that want synchronous behaviour, especially if
they're doing it wrong now.

Ben.

-- 
Ben Hutchings
Software Developer, Codethink Ltd.