Re: [PATCH 25/27] locking/lockdep: Add support for dynamic keys

[Bart Van Assche]

On Thu, 2018-11-29 at 11:10 +-0100, Peter Zijlstra wrote:
+AD4 On Wed, Nov 28, 2018 at 03:43:23PM -0800, Bart Van Assche wrote:
+AD4 +AD4 +-/+ACo hash+AF8-entry is used to keep track of dynamically allocated keys. +ACo-/
+AD4 +AD4  struct lock+AF8-class+AF8-key +AHs
+AD4 +AD4 +-	struct hlist+AF8-node		hash+AF8-entry+ADs
+AD4 +AD4  	struct lockdep+AF8-subclass+AF8-key	subkeys+AFs-MAX+AF8-LOCKDEP+AF8-SUBCLASSES+AF0AOw
+AD4 +AD4  +AH0AOw
+AD4 
+AD4 One consideration+ADs and maybe we should have a BUILD+AF8-BUG for that, is
+AD4 that this object should be no larger than the smallest lock primitive.
+AD4 
+AD4 That typically is raw+AF8-spinlock+AF8-t, which normally is 4 bytes, but with
+AD4 lockdep on that at least also includes struct lockdep+AF8-map.
+AD4 
+AD4 So what we want is:
+AD4 
+AD4 	sizeof(lock+AF8-class+AF8-key) +ADwAPQ sizeof(raw+AF8-spinlock+AF8-t)
+AD4 
+AD4 Otherwise, two consecutive spinlocks could end up with key overlap in
+AD4 their subclass range.
+AD4 
+AD4 Now, I think that is still valid after this patch, but it is something
+AD4 that gave me pause.

How about adding this as an additional patch before patch 25/27?

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 9a7cca6dc3d4..ce05b9b419f4 100644
--- a/kernel/locking/lockdep.c
+-+-+- b/kernel/locking/lockdep.c
+AEAAQA -725,6 +-725,15 +AEAAQA static bool assign+AF8-lock+AF8-key(struct lockdep+AF8-map +ACo-lock)
 +AHs
 	unsigned long can+AF8-addr, addr +AD0 (unsigned long)lock+ADs
 
+-	/+ACo
+-	 +ACo lockdep+AF8-free+AF8-key+AF8-range() assumes that struct lock+AF8-class+AF8-key
+-	 +ACo objects do not overlap. Since we use the address of lock
+-	 +ACo objects as class key for static objects, check whether the
+-	 +ACo size of lock+AF8-class+AF8-key objects does not exceed the size of
+-	 +ACo the smallest lock object.
+-	 +ACo-/
+-	BUILD+AF8-BUG+AF8-ON(sizeof(struct lock+AF8-class+AF8-key) +AD4 sizeof(raw+AF8-spinlock+AF8-t))+ADs
+-
 	if (+AF8AXw-is+AF8-kernel+AF8-percpu+AF8-address(addr, +ACY-can+AF8-addr))
 		lock-+AD4-key +AD0 (void +ACo)can+AF8-addr+ADs
 	else if (+AF8AXw-is+AF8-module+AF8-percpu+AF8-address(addr, +ACY-can+AF8-addr))

Thanks,

Bart.