linux-kernel - Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate stack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 19 Jan 2017 11:47:55 +0900
From:   Byungchul Park <byungchul.park@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     mingo@...nel.org, tglx@...utronix.de, walken@...gle.com,
        boqun.feng@...il.com, kirill@...temov.name,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        iamjoonsoo.kim@....com, akpm@...ux-foundation.org,
        npiggin@...il.com
Subject: Re: [PATCH v4 05/15] lockdep: Make check_prev_add can use a separate
 stack_trace

On Wed, Jan 18, 2017 at 04:10:53PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 18, 2017 at 11:04:32AM +0900, Byungchul Park wrote:
> > On Tue, Jan 17, 2017 at 04:54:31PM +0100, Peter Zijlstra wrote:
> > > On Fri, Jan 13, 2017 at 07:11:43PM +0900, Byungchul Park wrote:
> > > > What do you think about the following patches doing it?
> > > 
> > > I was more thinking about something like so...
> > > 
> > > Also, I think I want to muck with struct stack_trace; the members:
> > > max_nr_entries and skip are input arguments to save_stack_trace() and
> > > bloat the structure for no reason.
> > 
> > With your approach, save_trace() must be called whenever check_prevs_add()
> > is called, which might be unnecessary.
> 
> True.. but since we hold the graph_lock this is a slow path anyway, so I
> didn't care much.

If we don't need to care it, the problem becomes easy to solve. But IMHO,
it'd be better to care it as original lockdep code did, because
save_trace() might have bigger overhead than we expect and
check_prevs_add() can be called frequently, so it'd be better to avoid it
when possible.

> Then again, I forgot to clean up in a bunch of paths.
> 
> > Frankly speaking, I think what I proposed resolved it neatly. Don't you
> > think so?
> 
> My initial reaction was to your patches being radically different to
> what I had proposed. But after fixing mine I don't particularly like
> either one of them.
> 
> Also, I think yours has a hole in, you check nr_stack_trace_entries
> against an older copy to check we did save_stack(), this is not accurate
> as check_prev_add() can drop graph_lock in the verbose case and then
> someone else could have done save_stack().

Right. My mistake..

Then.. The following patch on top of my patch 2/2 can solve it. Right?

---

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 49b9386..0f5bded 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -1892,7 +1892,7 @@ static inline void inc_chains(void)
 		if (entry->class == hlock_class(next)) {
 			if (distance == 1)
 				entry->distance = 1;
-			return 2;
+			return 1;
 		}
 	}
 
@@ -1927,9 +1927,10 @@ static inline void inc_chains(void)
 		print_lock_name(hlock_class(next));
 		printk(KERN_CONT "\n");
 		dump_stack();
-		return graph_lock();
+		if (!graph_lock())
+			return 0;
 	}
-	return 1;
+	return 2;
 }
 
 /*
@@ -1975,15 +1976,16 @@ static inline void inc_chains(void)
 			 * added:
 			 */
 			if (hlock->read != 2 && hlock->check) {
-				if (!check_prev_add(curr, hlock, next,
-							distance, &trace, save))
+				int ret = check_prev_add(curr, hlock, next,
+							distance, &trace, save);
+				if (!ret)
 					return 0;
 
 				/*
 				 * Stop saving stack_trace if save_trace() was
 				 * called at least once:
 				 */
-				if (save && start_nr != nr_stack_trace_entries)
+				if (save && ret == 2)
 					save = NULL;
 
 				/*