* Re: 2.6.24-rc2-mm1: kcryptd vs lockdep
[not found] <20071120234605.GG23667@elte.hu>
@ 2007-11-21 15:58 ` Oleg Nesterov
2007-11-21 16:06 ` Johannes Berg
2007-11-24 10:53 ` [PATCH] debug_check_no_locks_freed: fix in_range() checks Oleg Nesterov
0 siblings, 2 replies; 7+ messages in thread
From: Oleg Nesterov @ 2007-11-21 15:58 UTC (permalink / raw)
To: Alasdair G Kergon, Milan Broz
Cc: Ingo Molnar, Johannes Berg, Torsten Kaiser, Andrew Morton, linux-kernel
Alasdair G Kergon wrote:
>
> - But what happens if kcryptd_crypt_write_convert_loop() calls
> INIT_WORK/queue_work twice?
Can't find this function. But "INIT_WORK + queue_work" twice is very
wrong of course.
Milan Broz wrote:
>
> Ok, then I have question: Is the following pseudocode correct
> (and problem is in lock validation which checks something
> already initialized for another queue) or reusing work_struct
> is not permitted from inside called work function ?
>
> (Note comment in code "It is permissible to free the struct
> work_struct from inside the function that is called from it".)
>
> struct work_struct work;
> struct workqueue_struct *a, *b;
>
> do_b(*work)
> {
> /* do something else */
> }
>
> do_a(*work)
> {
> /* do something */
> INIT_WORK(&work, do_b);
> queue_work(b, &work);
> }
>
>
> INIT_WORK(&work, do_a);
> queue_work(a, &work);
(just in case, in that particular case PREPARE_WORK() should be used)
INIT_WORK(w) can be used if we know that "w" is not pending, and nobody
else can write to this work (say, queue_work(w) or cancel_work_sync(w)).
So currently the code above should work correctly.
However, I'd say it is not correct, INIT_WORK() can throw out some debug
info for example, or the implementation could be changed.
I'm not sure about CONFIG_LOCKDEP (Johannes cc'ed). INIT_WORK() does
lockdep_init_map(->lockdep_map) but run_workqueue() has a local copy,
looks ok.
Oleg.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 2.6.24-rc2-mm1: kcryptd vs lockdep
2007-11-21 15:58 ` 2.6.24-rc2-mm1: kcryptd vs lockdep Oleg Nesterov
@ 2007-11-21 16:06 ` Johannes Berg
2007-11-24 10:53 ` [PATCH] debug_check_no_locks_freed: fix in_range() checks Oleg Nesterov
1 sibling, 0 replies; 7+ messages in thread
From: Johannes Berg @ 2007-11-21 16:06 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Alasdair G Kergon, Milan Broz, Ingo Molnar, Torsten Kaiser,
Andrew Morton, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]
Hi,
> > Ok, then I have question: Is the following pseudocode correct
> > (and problem is in lock validation which checks something
> > already initialized for another queue) or reusing work_struct
> > is not permitted from inside called work function ?
> >
> > (Note comment in code "It is permissible to free the struct
> > work_struct from inside the function that is called from it".)
> >
> > struct work_struct work;
> > struct workqueue_struct *a, *b;
> >
> > do_b(*work)
> > {
> > /* do something else */
> > }
> >
> > do_a(*work)
> > {
> > /* do something */
> > INIT_WORK(&work, do_b);
> > queue_work(b, &work);
> > }
> >
> >
> > INIT_WORK(&work, do_a);
> > queue_work(a, &work);
>
> (just in case, in that particular case PREPARE_WORK() should be used)
>
> INIT_WORK(w) can be used if we know that "w" is not pending, and nobody
> else can write to this work (say, queue_work(w) or cancel_work_sync(w)).
> So currently the code above should work correctly.
>
> However, I'd say it is not correct, INIT_WORK() can throw out some debug
> info for example, or the implementation could be changed.
>
> I'm not sure about CONFIG_LOCKDEP (Johannes cc'ed). INIT_WORK() does
> lockdep_init_map(->lockdep_map) but run_workqueue() has a local copy,
> looks ok.
We explicitly need to use a copy of the lockdep_map for "locking" the
work struct as per the quoted comment. So as far as I can tell, what
INIT_WORK() is doing here is changing an at that point unused copy of
the lockdep map so I think it should be fine. Not sure about the other
fine points nor why you'd want this though :)
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] debug_check_no_locks_freed: fix in_range() checks
2007-11-21 15:58 ` 2.6.24-rc2-mm1: kcryptd vs lockdep Oleg Nesterov
2007-11-21 16:06 ` Johannes Berg
@ 2007-11-24 10:53 ` Oleg Nesterov
2007-11-24 12:18 ` Torsten Kaiser
2007-11-24 12:22 ` Ingo Molnar
1 sibling, 2 replies; 7+ messages in thread
From: Oleg Nesterov @ 2007-11-24 10:53 UTC (permalink / raw)
To: Ingo Molnar, Alasdair G Kergon, Milan Broz
Cc: Johannes Berg, Torsten Kaiser, Andrew Morton, linux-kernel
Torsten, could you ack/nack this patch?
Torsten Kaiser wrote:
>
> static inline int in_range(const void *start, const void *addr, const void *end)
> {
> return addr >= start && addr <= end;
> }
> This will return true, if addr is in the range of start (including)
> to end (including).
>
> But debug_check_no_locks_freed() seems does:
> const void *mem_to = mem_from + mem_len
> -> mem_to is the last byte of the freed range, that fits in_range
> lock_from = (void *)hlock->instance;
> -> first byte of the lock
> lock_to = (void *)(hlock->instance + 1);
> -> first byte of the next lock, not last byte of the lock that is being checked!
>
> The test is:
> if (!in_range(mem_from, lock_from, mem_to) &&
> !in_range(mem_from, lock_to, mem_to))
> continue;
> So it tests, if the first byte of the lock is in the range that is freed ->OK
> And if the first byte of the *next* lock is in the range that is freed
> -> Not OK.
We can also simplify in_range checks, we need only 2 comparisons, not 4.
If the lock is not in memory range, it should be either at the left of range
or at the right.
Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
--- 24/kernel/lockdep.c~ 2007-11-09 12:57:31.000000000 +0300
+++ 24/kernel/lockdep.c 2007-11-24 13:32:52.000000000 +0300
@@ -3054,11 +3054,6 @@ void __init lockdep_info(void)
#endif
}
-static inline int in_range(const void *start, const void *addr, const void *end)
-{
- return addr >= start && addr <= end;
-}
-
static void
print_freed_lock_bug(struct task_struct *curr, const void *mem_from,
const void *mem_to, struct held_lock *hlock)
@@ -3080,6 +3075,13 @@ print_freed_lock_bug(struct task_struct
dump_stack();
}
+static inline int not_in_range(const void* mem_from, unsigned long mem_len,
+ const void* lock_from, unsigned long lock_len)
+{
+ return lock_from + lock_len <= mem_from ||
+ mem_from + mem_len <= lock_from;
+}
+
/*
* Called when kernel memory is freed (or unmapped), or if a lock
* is destroyed or reinitialized - this code checks whether there is
@@ -3087,7 +3089,6 @@ print_freed_lock_bug(struct task_struct
*/
void debug_check_no_locks_freed(const void *mem_from, unsigned long mem_len)
{
- const void *mem_to = mem_from + mem_len, *lock_from, *lock_to;
struct task_struct *curr = current;
struct held_lock *hlock;
unsigned long flags;
@@ -3100,14 +3101,11 @@ void debug_check_no_locks_freed(const vo
for (i = 0; i < curr->lockdep_depth; i++) {
hlock = curr->held_locks + i;
- lock_from = (void *)hlock->instance;
- lock_to = (void *)(hlock->instance + 1);
-
- if (!in_range(mem_from, lock_from, mem_to) &&
- !in_range(mem_from, lock_to, mem_to))
+ if (not_in_range(mem_from, mem_len, hlock->instance,
+ sizeof(*hlock->instance)))
continue;
- print_freed_lock_bug(curr, mem_from, mem_to, hlock);
+ print_freed_lock_bug(curr, mem_from, mem_from + mem_len, hlock);
break;
}
local_irq_restore(flags);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] debug_check_no_locks_freed: fix in_range() checks
2007-11-24 10:53 ` [PATCH] debug_check_no_locks_freed: fix in_range() checks Oleg Nesterov
@ 2007-11-24 12:18 ` Torsten Kaiser
2007-11-24 12:25 ` Oleg Nesterov
2007-11-24 12:35 ` Alasdair G Kergon
2007-11-24 12:22 ` Ingo Molnar
1 sibling, 2 replies; 7+ messages in thread
From: Torsten Kaiser @ 2007-11-24 12:18 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Alasdair G Kergon, Milan Broz, Johannes Berg,
Andrew Morton, linux-kernel
On Nov 24, 2007 11:53 AM, Oleg Nesterov <oleg@tv-sign.ru> wrote:
> Torsten, could you ack/nack this patch?
>From looking at the code I would ack it.
I will reapply agk-dm-dm-crypt-move-bio-submission-to-thread.patch and
this patch and boot several times, but as the message was not
triggered on every boot, this can't prove anything.
But if it happens again, I will notify you.
Torsten
> Torsten Kaiser wrote:
> >
> > static inline int in_range(const void *start, const void *addr, const void *end)
> > {
> > return addr >= start && addr <= end;
> > }
> > This will return true, if addr is in the range of start (including)
> > to end (including).
> >
> > But debug_check_no_locks_freed() seems does:
> > const void *mem_to = mem_from + mem_len
> > -> mem_to is the last byte of the freed range, that fits in_range
> > lock_from = (void *)hlock->instance;
> > -> first byte of the lock
> > lock_to = (void *)(hlock->instance + 1);
> > -> first byte of the next lock, not last byte of the lock that is being checked!
> >
> > The test is:
> > if (!in_range(mem_from, lock_from, mem_to) &&
> > !in_range(mem_from, lock_to, mem_to))
> > continue;
> > So it tests, if the first byte of the lock is in the range that is freed ->OK
> > And if the first byte of the *next* lock is in the range that is freed
> > -> Not OK.
>
> We can also simplify in_range checks, we need only 2 comparisons, not 4.
> If the lock is not in memory range, it should be either at the left of range
> or at the right.
>
> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
>
> --- 24/kernel/lockdep.c~ 2007-11-09 12:57:31.000000000 +0300
> +++ 24/kernel/lockdep.c 2007-11-24 13:32:52.000000000 +0300
> @@ -3054,11 +3054,6 @@ void __init lockdep_info(void)
> #endif
> }
>
> -static inline int in_range(const void *start, const void *addr, const void *end)
> -{
> - return addr >= start && addr <= end;
> -}
> -
> static void
> print_freed_lock_bug(struct task_struct *curr, const void *mem_from,
> const void *mem_to, struct held_lock *hlock)
> @@ -3080,6 +3075,13 @@ print_freed_lock_bug(struct task_struct
> dump_stack();
> }
>
> +static inline int not_in_range(const void* mem_from, unsigned long mem_len,
> + const void* lock_from, unsigned long lock_len)
> +{
> + return lock_from + lock_len <= mem_from ||
> + mem_from + mem_len <= lock_from;
> +}
> +
> /*
> * Called when kernel memory is freed (or unmapped), or if a lock
> * is destroyed or reinitialized - this code checks whether there is
> @@ -3087,7 +3089,6 @@ print_freed_lock_bug(struct task_struct
> */
> void debug_check_no_locks_freed(const void *mem_from, unsigned long mem_len)
> {
> - const void *mem_to = mem_from + mem_len, *lock_from, *lock_to;
> struct task_struct *curr = current;
> struct held_lock *hlock;
> unsigned long flags;
> @@ -3100,14 +3101,11 @@ void debug_check_no_locks_freed(const vo
> for (i = 0; i < curr->lockdep_depth; i++) {
> hlock = curr->held_locks + i;
>
> - lock_from = (void *)hlock->instance;
> - lock_to = (void *)(hlock->instance + 1);
> -
> - if (!in_range(mem_from, lock_from, mem_to) &&
> - !in_range(mem_from, lock_to, mem_to))
> + if (not_in_range(mem_from, mem_len, hlock->instance,
> + sizeof(*hlock->instance)))
> continue;
>
> - print_freed_lock_bug(curr, mem_from, mem_to, hlock);
> + print_freed_lock_bug(curr, mem_from, mem_from + mem_len, hlock);
> break;
> }
> local_irq_restore(flags);
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] debug_check_no_locks_freed: fix in_range() checks
2007-11-24 10:53 ` [PATCH] debug_check_no_locks_freed: fix in_range() checks Oleg Nesterov
2007-11-24 12:18 ` Torsten Kaiser
@ 2007-11-24 12:22 ` Ingo Molnar
1 sibling, 0 replies; 7+ messages in thread
From: Ingo Molnar @ 2007-11-24 12:22 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Alasdair G Kergon, Milan Broz, Johannes Berg, Torsten Kaiser,
Andrew Morton, linux-kernel
* Oleg Nesterov <oleg@tv-sign.ru> wrote:
> > But debug_check_no_locks_freed() seems does:
> > const void *mem_to = mem_from + mem_len
> > -> mem_to is the last byte of the freed range, that fits in_range
> > lock_from = (void *)hlock->instance;
> > -> first byte of the lock
> > lock_to = (void *)(hlock->instance + 1);
> > -> first byte of the next lock, not last byte of the lock that is being checked!
> >
> > The test is:
> > if (!in_range(mem_from, lock_from, mem_to) &&
> > !in_range(mem_from, lock_to, mem_to))
> > continue;
> > So it tests, if the first byte of the lock is in the range that is freed ->OK
> > And if the first byte of the *next* lock is in the range that is freed
> > -> Not OK.
thanks, applied.
Ingo
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] debug_check_no_locks_freed: fix in_range() checks
2007-11-24 12:18 ` Torsten Kaiser
@ 2007-11-24 12:25 ` Oleg Nesterov
2007-11-24 12:35 ` Alasdair G Kergon
1 sibling, 0 replies; 7+ messages in thread
From: Oleg Nesterov @ 2007-11-24 12:25 UTC (permalink / raw)
To: Torsten Kaiser
Cc: Ingo Molnar, Alasdair G Kergon, Milan Broz, Johannes Berg,
Andrew Morton, linux-kernel
On 11/24, Torsten Kaiser wrote:
>
> On Nov 24, 2007 11:53 AM, Oleg Nesterov <oleg@tv-sign.ru> wrote:
> > Torsten, could you ack/nack this patch?
>
> From looking at the code I would ack it.
Great.
> I will reapply agk-dm-dm-crypt-move-bio-submission-to-thread.patch and
> this patch and boot several times, but as the message was not
> triggered on every boot, this can't prove anything.
Regardless of any other possible problems, I think you found a real bug
in debug_check_no_locks_freed() which should be fixed.
Oleg.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] debug_check_no_locks_freed: fix in_range() checks
2007-11-24 12:18 ` Torsten Kaiser
2007-11-24 12:25 ` Oleg Nesterov
@ 2007-11-24 12:35 ` Alasdair G Kergon
1 sibling, 0 replies; 7+ messages in thread
From: Alasdair G Kergon @ 2007-11-24 12:35 UTC (permalink / raw)
To: Torsten Kaiser
Cc: Oleg Nesterov, Ingo Molnar, Alasdair G Kergon, Milan Broz,
Johannes Berg, Andrew Morton, linux-kernel
On Sat, Nov 24, 2007 at 01:18:35PM +0100, Torsten Kaiser wrote:
> I will reapply agk-dm-dm-crypt-move-bio-submission-to-thread.patch and
> this patch and boot several times
OK for a test system, but until the write loop problem is addressed I believe
there's a risk of data corruption under low memory conditions. (I dropped it
from the export to -mm a few days ago for this reason.)
Alasdair
--
agk@redhat.com
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2007-11-24 12:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20071120234605.GG23667@elte.hu>
2007-11-21 15:58 ` 2.6.24-rc2-mm1: kcryptd vs lockdep Oleg Nesterov
2007-11-21 16:06 ` Johannes Berg
2007-11-24 10:53 ` [PATCH] debug_check_no_locks_freed: fix in_range() checks Oleg Nesterov
2007-11-24 12:18 ` Torsten Kaiser
2007-11-24 12:25 ` Oleg Nesterov
2007-11-24 12:35 ` Alasdair G Kergon
2007-11-24 12:22 ` Ingo Molnar
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).