Re: [PATCH] drivers/core: Replace lockdep_set_novalidate_class() with unique class keys

[Alan Stern]

On Wed, Feb 08, 2023 at 07:30:25PM +0900, Tetsuo Handa wrote:
> Commit 1704f47b50b5 ("lockdep: Add novalidate class for dev->mutex
> conversion") made it impossible to find real deadlocks unless timing
> dependent testings manage to trigger hung task like [1] and [2]. And
> lockdep_set_novalidate_class() remained for more than one decade due to
> a fear of false positives [3]. But not sharing mutex_init() could make
> it possible to find real deadlocks without triggering hung task [4].
> Thus, let's assign a unique class key on each "struct device"->mutex.
> 
> Link: https://syzkaller.appspot.com/bug?extid=2d6ac90723742279e101 [1]
> Link: https://syzkaller.appspot.com/bug?extid=2e39bc6569d281acbcfb [2]
> Link: https://lkml.kernel.org/r/Y98FLlr7jkiFlV0k@xxxxxxxxxxxxxxxxxxx [3]
> Link: https://lkml.kernel.org/r/827177aa-bb64-87a9-e1af-dfe070744045@xxxxxxxxxxxxxxxxxxx [4]
> Suggested-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> Co-developed-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>

You must never do this!

I did not put my Signed-off-by: on the patch I sent to you.  I do not 
want it added to that patch or to this one.  You should never put 
somebody else's Signed-off-by: on a patch unless they tell you it's okay 
to do so.

I'm happy to have people test this patch, but I do not want anybody 
think that it is ready to be merged into the kernel.

> Co-developed-by: Hillf Danton <hdanton@xxxxxxxx>
> Signed-off-by: Hillf Danton <hdanton@xxxxxxxx>
> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> ---
> Hello, syzkaller users.
> 
> We made a patch that keeps lockdep validation enabled on "struct dev->mutex".
> Will you try this patch and see if this patch causes boot failures and/or
> too frequent crashes to continue testing.
> 
>  drivers/base/core.c      | 7 ++++++-
>  include/linux/device.h   | 1 +
>  include/linux/lockdep.h  | 6 ++++++
>  kernel/locking/lockdep.c | 7 +++++++
>  4 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index a3e14143ec0c..c30ecbc4d60e 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2322,6 +2322,9 @@ static void device_release(struct kobject *kobj)
>  	devres_release_all(dev);
>  
>  	kfree(dev->dma_range_map);
> +	mutex_destroy(&dev->mutex);
> +	if (!lockdep_static_obj(dev))
> +		lockdep_unregister_key(&dev->mutex_key);
>  
>  	if (dev->release)
>  		dev->release(dev);
> @@ -2941,7 +2944,9 @@ void device_initialize(struct device *dev)
>  	kobject_init(&dev->kobj, &device_ktype);
>  	INIT_LIST_HEAD(&dev->dma_pools);
>  	mutex_init(&dev->mutex);
> -	lockdep_set_novalidate_class(&dev->mutex);
> +	if (!lockdep_static_obj(dev))
> +		lockdep_register_key(&dev->mutex_key);
> +	lockdep_set_class(&dev->mutex, &dev->mutex_key);
>  	spin_lock_init(&dev->devres_lock);
>  	INIT_LIST_HEAD(&dev->devres_head);
>  	device_pm_init(dev);
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 44e3acae7b36..bdaca9f54dc2 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -570,6 +570,7 @@ struct device {
>  	struct mutex		mutex;	/* mutex to synchronize calls to
>  					 * its driver.
>  					 */
> +	struct lock_class_key	mutex_key;	/* Unique key for each device */
>  
>  	struct dev_links_info	links;
>  	struct dev_pm_info	power;
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 1f1099dac3f0..5afc999a7e56 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -172,6 +172,7 @@ do {							\
>  	current->lockdep_recursion -= LOCKDEP_OFF;	\
>  } while (0)
>  
> +extern int lockdep_static_obj(const void *obj);
>  extern void lockdep_register_key(struct lock_class_key *key);
>  extern void lockdep_unregister_key(struct lock_class_key *key);
>  
> @@ -391,6 +392,11 @@ static inline void lockdep_set_selftest_task(struct task_struct *task)
>  # define lockdep_free_key_range(start, size)	do { } while (0)
>  # define lockdep_sys_exit() 			do { } while (0)
>  
> +static inline int lockdep_static_obj(const void *obj)
> +{
> +	return 0;
> +}
> +
>  static inline void lockdep_register_key(struct lock_class_key *key)
>  {
>  }
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index e3375bc40dad..74c0113646f1 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -857,6 +857,13 @@ static int static_obj(const void *obj)
>  	 */
>  	return is_module_address(addr) || is_module_percpu_address(addr);
>  }
> +
> +int lockdep_static_obj(const void *obj)
> +{
> +	return static_obj(obj);
> +}
> +EXPORT_SYMBOL_GPL(lockdep_static_obj);

What's the point of adding a new function that just calls the old 
function?  Why not simply rename the old function?

Alan Stern

> +
>  #endif
>  
>  /*
> -- 
> 2.34.1
>