Re: [PATCH driver-core-linus] sysfs: give different locking key to regular and bin files

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Dec 04, 2013 at 09:20:40AM -0500, Tejun Heo wrote:
> Dave, can you please test this one too?  Greg, once this turns out to
> be okay, I'll send you a merged branch which pulls in
> driver-core-linus + this patch into driver-core-next which will surely
> generate conflict.

Dave, did this patch fix the issue?

thanks,

greg k-h

> ----- 8< ------
> 027a485d12e0 ("sysfs: use a separate locking class for open files
> depending on mmap") assigned different lockdep key to
> sysfs_open_file->mutex depending on whether the file implements mmap
> or not in an attempt to avoid spurious lockdep warning caused by
> merging of regular and bin file paths.
> 
> While this restored some of the original behavior of using different
> locks (at least lockdep is concerned) for the different clases of
> files.  The restoration wasn't full because now the lockdep key
> assignment depends on whether the file has mmap or not instead of
> whether it's a regular file or not.
> 
> This means that bin files which don't implement mmap will get assigned
> the same lockdep class as regular files.  This is problematic because
> file_operations for bin files still implements the mmap file operation
> and checking whether the sysfs file actually implements mmap happens
> in the file operation after grabbing @sysfs_open_file->mutex.  We
> still end up adding locking dependency from mmap locking to
> sysfs_open_file->mutex to the regular file mutex which triggers
> spurious circular locking warning.
> 
> Fix it by restoring the original behavior fully by differentiating
> lockdep key by whether the file is regular or bin, instead of the
> existence of mmap.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Dave Jones <davej@redhat.com>
> Link: http://lkml.kernel.org/g/20131203184324.GA11320@redhat.com
> ---
>  fs/sysfs/file.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
> index b94f936..35e7d08 100644
> --- a/fs/sysfs/file.c
> +++ b/fs/sysfs/file.c
> @@ -609,7 +609,7 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
>  	struct sysfs_dirent *attr_sd = file->f_path.dentry->d_fsdata;
>  	struct kobject *kobj = attr_sd->s_parent->s_dir.kobj;
>  	struct sysfs_open_file *of;
> -	bool has_read, has_write, has_mmap;
> +	bool has_read, has_write;
>  	int error = -EACCES;
>  
>  	/* need attr_sd for attr and ops, its parent for kobj */
> @@ -621,7 +621,6 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
>  
>  		has_read = battr->read || battr->mmap;
>  		has_write = battr->write || battr->mmap;
> -		has_mmap = battr->mmap;
>  	} else {
>  		const struct sysfs_ops *ops = sysfs_file_ops(attr_sd);
>  
> @@ -633,7 +632,6 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
>  
>  		has_read = ops->show;
>  		has_write = ops->store;
> -		has_mmap = false;
>  	}
>  
>  	/* check perms and supported operations */
> @@ -661,9 +659,9 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
>  	 * open file has a separate mutex, it's okay as long as those don't
>  	 * happen on the same file.  At this point, we can't easily give
>  	 * each file a separate locking class.  Let's differentiate on
> -	 * whether the file has mmap or not for now.
> +	 * whether the file is bin or not for now.
>  	 */
> -	if (has_mmap)
> +	if (sysfs_is_bin(attr_sd))
>  		mutex_init(&of->mutex);
>  	else
>  		mutex_init(&of->mutex);