* [PATCH] UBI: Fix possible deadlock in erase_worker()
@ 2014-09-16 7:48 Richard Weinberger
2014-09-17 8:28 ` Artem Bityutskiy
2014-09-17 9:35 ` Artem Bityutskiy
0 siblings, 2 replies; 8+ messages in thread
From: Richard Weinberger @ 2014-09-16 7:48 UTC (permalink / raw)
To: dedekind1
Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel,
Richard Weinberger, stable
If sync_erase() failes with EINTR, ENOMEM, EAGAIN or
EBUSY erase_worker() re-schedules the failed work.
This will lead to a deadlock because erase_worker() is called
with work_sem held in read mode. And schedule_erase() will take
this lock again.
Fix this issue by adding a locked flag to schedule_erase(),
it indicates whether the caller owns work_sem already.
[ 16.571519] [ INFO: possible recursive locking detected ]
[ 16.571519] 3.17.0-rc4+ #69 Not tainted
[ 16.571519] ---------------------------------------------
[ 16.571519] ubi_bgt0d/2607 is trying to acquire lock:
[ 16.571519] (&ubi->work_sem){.+.+..}, at: [<ffffffff8154eaee>] schedule_ubi_work+0x1e/0x40
[ 16.571519]
[ 16.571519] but task is already holding lock:
[ 16.571519] (&ubi->work_sem){.+.+..}, at: [<ffffffff8154e7ec>] do_work+0x3c/0x110
[ 16.571519]
[ 16.571519] other info that might help us debug this:
[ 16.571519] Possible unsafe locking scenario:
[ 16.571519]
[ 16.571519] CPU0
[ 16.571519] ----
[ 16.571519] lock(&ubi->work_sem);
[ 16.571519] lock(&ubi->work_sem);
[ 16.571519]
[ 16.571519] *** DEADLOCK ***
[ 16.571519]
[ 16.571519] May be due to missing lock nesting notation
[ 16.571519]
[ 16.571519] 1 lock held by ubi_bgt0d/2607:
[ 16.571519] #0: (&ubi->work_sem){.+.+..}, at: [<ffffffff8154e7ec>] do_work+0x3c/0x110
[ 16.571519]
[ 16.571519] stack backtrace:
[ 16.571519] CPU: 1 PID: 2607 Comm: ubi_bgt0d Not tainted 3.17.0-rc4+ #69
[ 16.571519] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.7.5-0-ge51488c-20140816_022509-build35 04/01/2014
[ 16.571519] ffffffff829ee740 ffff88003c32fba8 ffffffff818b4693 ffffffff829ee740
[ 16.571519] ffff88003c32fc70 ffffffff8108bb4b ffff88003cc65400 ffff88003c32c000
[ 16.571519] 000000003e1d37c0 ffffffff810855f1 ffffffff00000000 0000000000a2c516
[ 16.571519] Call Trace:
[ 16.571519] [<ffffffff818b4693>] dump_stack+0x4d/0x66
[ 16.571519] [<ffffffff8108bb4b>] __lock_acquire+0x1b3b/0x1ce0
[ 16.571519] [<ffffffff810855f1>] ? up+0x11/0x50
[ 16.571519] [<ffffffff8109c504>] ? console_unlock+0x1d4/0x4b0
[ 16.571519] [<ffffffff81088f7d>] ? trace_hardirqs_on+0xd/0x10
[ 16.571519] [<ffffffff8108bd98>] lock_acquire+0xa8/0x140
[ 16.571519] [<ffffffff8154eaee>] ? schedule_ubi_work+0x1e/0x40
[ 16.571519] [<ffffffff818bf7bc>] down_read+0x4c/0xa0
[ 16.571519] [<ffffffff8154eaee>] ? schedule_ubi_work+0x1e/0x40
[ 16.571519] [<ffffffff8154eaee>] schedule_ubi_work+0x1e/0x40
[ 16.571519] [<ffffffff8154ebb8>] schedule_erase+0xa8/0x130
[ 16.571519] [<ffffffff8154f42a>] erase_worker+0x40a/0x710
[ 16.571519] [<ffffffff8154e82d>] do_work+0x7d/0x110
[ 16.571519] [<ffffffff81550e78>] ubi_thread+0x128/0x1e0
[ 16.571519] [<ffffffff81550d50>] ? ubi_wl_flush+0x180/0x180
[ 16.571519] [<ffffffff8106871b>] kthread+0xeb/0x110
[ 16.571519] [<ffffffff81068630>] ? kthread_create_on_node+0x210/0x210
[ 16.571519] [<ffffffff818c1e6c>] ret_from_fork+0x7c/0xb0
[ 16.571519] [<ffffffff81068630>] ? kthread_create_on_node+0x210/0x210
Cc: <stable@vger.kernel.org>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
drivers/mtd/ubi/wl.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 20f4917..e985f29 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -884,21 +884,20 @@ int ubi_is_erase_work(struct ubi_work *wrk)
* @vol_id: the volume ID that last used this PEB
* @lnum: the last used logical eraseblock number for the PEB
* @torture: if the physical eraseblock has to be tortured
+ * @locked: non-zero if this function is called with @ubi->work_sem held in
+ * read mode. Never call it with @ubi->work_sem held in write mode!
*
* This function returns zero in case of success and a %-ENOMEM in case of
* failure.
*/
static int schedule_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
- int vol_id, int lnum, int torture)
+ int vol_id, int lnum, int torture, int locked)
{
struct ubi_work *wl_wrk;
ubi_assert(e);
ubi_assert(!ubi_is_fm_block(ubi, e->pnum));
- dbg_wl("schedule erasure of PEB %d, EC %d, torture %d",
- e->pnum, e->ec, torture);
-
wl_wrk = kmalloc(sizeof(struct ubi_work), GFP_NOFS);
if (!wl_wrk)
return -ENOMEM;
@@ -909,7 +908,13 @@ static int schedule_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
wl_wrk->lnum = lnum;
wl_wrk->torture = torture;
- schedule_ubi_work(ubi, wl_wrk);
+ dbg_wl("schedule erasure of PEB %d, EC %d, torture %d",
+ e->pnum, e->ec, torture);
+
+ if (locked)
+ __schedule_ubi_work(ubi, wl_wrk);
+ else
+ schedule_ubi_work(ubi, wl_wrk);
return 0;
}
@@ -982,7 +987,7 @@ int ubi_wl_put_fm_peb(struct ubi_device *ubi, struct ubi_wl_entry *fm_e,
spin_unlock(&ubi->wl_lock);
vol_id = lnum ? UBI_FM_DATA_VOLUME_ID : UBI_FM_SB_VOLUME_ID;
- return schedule_erase(ubi, e, vol_id, lnum, torture);
+ return schedule_erase(ubi, e, vol_id, lnum, torture, 1);
}
#endif
@@ -1464,7 +1469,7 @@ static int erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk,
int err1;
/* Re-schedule the LEB for erasure */
- err1 = schedule_erase(ubi, e, vol_id, lnum, 0);
+ err1 = schedule_erase(ubi, e, vol_id, lnum, 0, 1);
if (err1) {
err = err1;
goto out_ro;
@@ -1620,7 +1625,7 @@ retry:
}
spin_unlock(&ubi->wl_lock);
- err = schedule_erase(ubi, e, vol_id, lnum, torture);
+ err = schedule_erase(ubi, e, vol_id, lnum, torture, 0);
if (err) {
spin_lock(&ubi->wl_lock);
wl_tree_add(e, &ubi->used);
@@ -1909,7 +1914,7 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai)
e->ec = aeb->ec;
ubi_assert(!ubi_is_fm_block(ubi, e->pnum));
ubi->lookuptbl[e->pnum] = e;
- if (schedule_erase(ubi, e, aeb->vol_id, aeb->lnum, 0)) {
+ if (schedule_erase(ubi, e, aeb->vol_id, aeb->lnum, 0, 0)) {
kmem_cache_free(ubi_wl_entry_slab, e);
goto out_free;
}
--
1.8.4.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] UBI: Fix possible deadlock in erase_worker()
2014-09-16 7:48 [PATCH] UBI: Fix possible deadlock in erase_worker() Richard Weinberger
@ 2014-09-17 8:28 ` Artem Bityutskiy
2014-09-17 8:40 ` Richard Weinberger
2014-09-17 9:35 ` Artem Bityutskiy
1 sibling, 1 reply; 8+ messages in thread
From: Artem Bityutskiy @ 2014-09-17 8:28 UTC (permalink / raw)
To: Richard Weinberger
Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel, stable
On Tue, 2014-09-16 at 09:48 +0200, Richard Weinberger wrote:
> If sync_erase() failes with EINTR, ENOMEM, EAGAIN or
> EBUSY erase_worker() re-schedules the failed work.
> This will lead to a deadlock because erase_worker() is called
> with work_sem held in read mode. And schedule_erase() will take
> this lock again.
IIRC, the assumption was that the R/W semaphore may be taken in read
mode many times, so it wouldn't hurt to do:
down_read()
down_read()
up_read()
up_read()
If this is right, then the lockdep warning is incorrect.
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] UBI: Fix possible deadlock in erase_worker()
2014-09-17 8:28 ` Artem Bityutskiy
@ 2014-09-17 8:40 ` Richard Weinberger
2014-09-17 8:43 ` Artem Bityutskiy
0 siblings, 1 reply; 8+ messages in thread
From: Richard Weinberger @ 2014-09-17 8:40 UTC (permalink / raw)
To: dedekind1; +Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel, stable
Am 17.09.2014 10:28, schrieb Artem Bityutskiy:
> On Tue, 2014-09-16 at 09:48 +0200, Richard Weinberger wrote:
>> If sync_erase() failes with EINTR, ENOMEM, EAGAIN or
>> EBUSY erase_worker() re-schedules the failed work.
>> This will lead to a deadlock because erase_worker() is called
>> with work_sem held in read mode. And schedule_erase() will take
>> this lock again.
>
> IIRC, the assumption was that the R/W semaphore may be taken in read
> mode many times, so it wouldn't hurt to do:
>
> down_read()
> down_read()
> up_read()
> up_read()
Hmm, are you sure that this is legal?
Quoting rwsem.h:
/*
* nested locking. NOTE: rwsems are not allowed to recurse
* (which occurs if the same task tries to acquire the same
* lock instance multiple times), but multiple locks of the
* same lock class might be taken, if the order of the locks
* is always the same. This ordering rule can be expressed
* to lockdep via the _nested() APIs, but enumerating the
* subclasses that are used. (If the nesting relationship is
* static then another method for expressing nested locking is
* the explicit definition of lock class keys and the use of
* lockdep_set_class() at lock initialization time.
* See Documentation/lockdep-design.txt for more details.)
*/
In this case the same task is taking the same lock multiple times,
which is not allowed according to rwsem.h.
Thanks,
//richard
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] UBI: Fix possible deadlock in erase_worker()
2014-09-17 8:40 ` Richard Weinberger
@ 2014-09-17 8:43 ` Artem Bityutskiy
0 siblings, 0 replies; 8+ messages in thread
From: Artem Bityutskiy @ 2014-09-17 8:43 UTC (permalink / raw)
To: Richard Weinberger
Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel, stable
On Wed, 2014-09-17 at 10:40 +0200, Richard Weinberger wrote:
> /*
> * nested locking. NOTE: rwsems are not allowed to recurse
> * (which occurs if the same task tries to acquire the same
> * lock instance multiple times), but multiple locks of the
> * same lock class might be taken, if the order of the locks
> * is always the same. This ordering rule can be expressed
> * to lockdep via the _nested() APIs, but enumerating the
> * subclasses that are used. (If the nesting relationship is
> * static then another method for expressing nested locking is
> * the explicit definition of lock class keys and the use of
> * lockdep_set_class() at lock initialization time.
> * See Documentation/lockdep-design.txt for more details.)
> */
>
> In this case the same task is taking the same lock multiple times,
> which is not allowed according to rwsem.h.
Yes, this part was missed, thanks.
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] UBI: Fix possible deadlock in erase_worker()
2014-09-16 7:48 [PATCH] UBI: Fix possible deadlock in erase_worker() Richard Weinberger
2014-09-17 8:28 ` Artem Bityutskiy
@ 2014-09-17 9:35 ` Artem Bityutskiy
2014-09-19 9:46 ` Richard Weinberger
1 sibling, 1 reply; 8+ messages in thread
From: Artem Bityutskiy @ 2014-09-17 9:35 UTC (permalink / raw)
To: Richard Weinberger
Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel, stable
On Tue, 2014-09-16 at 09:48 +0200, Richard Weinberger wrote:
> If sync_erase() failes with EINTR, ENOMEM, EAGAIN or
> EBUSY erase_worker() re-schedules the failed work.
> This will lead to a deadlock because erase_worker() is called
> with work_sem held in read mode. And schedule_erase() will take
> this lock again.
There is this code snippet:
ubi_err("failed to erase PEB %d, error %d", pnum, err);
kfree(wl_wrk);
if (err == -EINTR || err == -ENOMEM || err == -EAGAIN ||
err == -EBUSY) {
int err1;
/* Re-schedule the LEB for erasure */
err1 = schedule_erase(ubi, e, vol_id, lnum, 0);
if (err1) {
err = err1;
goto out_ro;
}
return err;
}
How about move 'kfree(wl_wrk)' down, and execute
__schedule_ubi_work(ubi, wl_wrk)
inside the 'if' clause instead? The fix would seem to be more elegant
then.
Hmm?
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] UBI: Fix possible deadlock in erase_worker()
2014-09-17 9:35 ` Artem Bityutskiy
@ 2014-09-19 9:46 ` Richard Weinberger
2014-09-19 10:47 ` Artem Bityutskiy
0 siblings, 1 reply; 8+ messages in thread
From: Richard Weinberger @ 2014-09-19 9:46 UTC (permalink / raw)
To: dedekind1; +Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel, stable
Am 17.09.2014 11:35, schrieb Artem Bityutskiy:
> On Tue, 2014-09-16 at 09:48 +0200, Richard Weinberger wrote:
>> If sync_erase() failes with EINTR, ENOMEM, EAGAIN or
>> EBUSY erase_worker() re-schedules the failed work.
>> This will lead to a deadlock because erase_worker() is called
>> with work_sem held in read mode. And schedule_erase() will take
>> this lock again.
>
> There is this code snippet:
>
> ubi_err("failed to erase PEB %d, error %d", pnum, err);
> kfree(wl_wrk);
>
> if (err == -EINTR || err == -ENOMEM || err == -EAGAIN ||
> err == -EBUSY) {
> int err1;
>
> /* Re-schedule the LEB for erasure */
> err1 = schedule_erase(ubi, e, vol_id, lnum, 0);
> if (err1) {
> err = err1;
> goto out_ro;
> }
> return err;
> }
>
> How about move 'kfree(wl_wrk)' down, and execute
>
> __schedule_ubi_work(ubi, wl_wrk)
>
> inside the 'if' clause instead? The fix would seem to be more elegant
> then.
>
> Hmm?
Yes, that would work too.
Or we apply "[PATCH 1/2] UBI: Call worker functions without work_sem held". :)
Thanks,
//richard
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] UBI: Fix possible deadlock in erase_worker()
2014-09-19 9:46 ` Richard Weinberger
@ 2014-09-19 10:47 ` Artem Bityutskiy
2014-09-19 11:01 ` Richard Weinberger
0 siblings, 1 reply; 8+ messages in thread
From: Artem Bityutskiy @ 2014-09-19 10:47 UTC (permalink / raw)
To: Richard Weinberger
Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel, stable
On Fri, 2014-09-19 at 11:46 +0200, Richard Weinberg
> Or we apply "[PATCH 1/2] UBI: Call worker functions without work_sem held". :)
But I explained in the other e-mail that the semaphore is needed, and
why.
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] UBI: Fix possible deadlock in erase_worker()
2014-09-19 10:47 ` Artem Bityutskiy
@ 2014-09-19 11:01 ` Richard Weinberger
0 siblings, 0 replies; 8+ messages in thread
From: Richard Weinberger @ 2014-09-19 11:01 UTC (permalink / raw)
To: dedekind1; +Cc: dwmw2, computersforpeace, linux-mtd, linux-kernel, stable
Am 19.09.2014 12:47, schrieb Artem Bityutskiy:
> On Fri, 2014-09-19 at 11:46 +0200, Richard Weinberg
>> Or we apply "[PATCH 1/2] UBI: Call worker functions without work_sem held". :)
>
> But I explained in the other e-mail that the semaphore is needed, and
> why.
Sorry, I completely misunderstood your mail.
Rereading it now.
Thanks,
//richard
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-09-19 11:01 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-16 7:48 [PATCH] UBI: Fix possible deadlock in erase_worker() Richard Weinberger
2014-09-17 8:28 ` Artem Bityutskiy
2014-09-17 8:40 ` Richard Weinberger
2014-09-17 8:43 ` Artem Bityutskiy
2014-09-17 9:35 ` Artem Bityutskiy
2014-09-19 9:46 ` Richard Weinberger
2014-09-19 10:47 ` Artem Bityutskiy
2014-09-19 11:01 ` Richard Weinberger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).