From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from imap0.codethink.co.uk ([185.43.218.159]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1d3RXK-0006Aj-Da for linux-mtd@lists.infradead.org; Wed, 26 Apr 2017 18:19:36 +0000 Message-ID: <1493230734.10415.140.camel@codethink.co.uk> Subject: Re: [Linux-kernel] [PATCH 1/1] mtd: mtdblock: avoid __might_sleep warnings in mtd_erase From: Ben Hutchings To: Ben Dooks Cc: linux-kernel@lists.codethink.co.uk, David Woodhouse , Brian Norris , linux-mtd@lists.infradead.org Date: Wed, 26 Apr 2017 19:18:54 +0100 In-Reply-To: <20170426174609.29433-2-ben.dooks@codethink.co.uk> References: <20170426174609.29433-1-ben.dooks@codethink.co.uk> <20170426174609.29433-2-ben.dooks@codethink.co.uk> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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.