Re: [PATCH driver-core-next] sysfs: rename sysfs_assoc_lock and explain what it's about

[ebiederm@xmission.com (Eric W. Biederman)]

Tejun Heo <tj@kernel.org> writes:

> Hey, again.
>
> How about something like the following?
>
> -	spin_lock(&sysfs_assoc_lock);
> +	/*
> +	 * In general, kboject owner is responsible for ensuring removal
> +	 * doesn't race with other operations and sysfs doesn't provide any
> +	 * protection; however, when @kobj is used as a symlink target, the
> +	 * symlinking entity usually doesn't own @kobj and thus has no
> +	 * control over removal.  @kobj->sd may be removed anytime and
> +	 * symlink code may end up dereferencing an already freed sd.

Except every time sysfs exports a restriction like that and doesn't
verify people have held up their end of it someone in the kernel
inevitably gets the code wrong.  So I don't see how a big fat comment
buried deep in the underlying abstractions that people use is going to
make the code easier to understand or maintain.  It certainly won't
prevent people from goofing up and with no warning.

> +	 *
> +	 * sysfs_symlink_target_lock synchronizes @kobj->sd disassociation
> +	 * against symlink operations so that symlink code can safely
> +	 * dereference @kobj->sd.
> +	 */
> +	spin_lock(&sysfs_symlink_target_lock);
>  	kobj->sd = NULL;
> -	spin_unlock(&sysfs_assoc_lock);
> +	spin_unlock(&sysfs_symlink_target_lock);
>  
>  	if (sd) {
>  		WARN_ON_ONCE(sysfs_type(sd) != SYSFS_DIR);
> diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
> index 22ea2f5..1a23681 100644
> --- a/fs/sysfs/symlink.c
> +++ b/fs/sysfs/symlink.c
> @@ -32,13 +32,15 @@ static int sysfs_do_create_link_sd(struct sysfs_dirent *parent_sd,
>  
>  	BUG_ON(!name || !parent_sd);
>  
> -	/* target->sd can go away beneath us but is protected with
> -	 * sysfs_assoc_lock.  Fetch target_sd from it.
> +	/*
> +	 * We don't own @target and it may be removed at any time.
> +	 * Synchronize using sysfs_symlink_target_lock.  See
> +	 * sysfs_remove_dir() for details.
>  	 */
> -	spin_lock(&sysfs_assoc_lock);
> +	spin_lock(&sysfs_symlink_target_lock);
>  	if (target->sd)
>  		target_sd = sysfs_get(target->sd);
> -	spin_unlock(&sysfs_assoc_lock);
> +	spin_unlock(&sysfs_symlink_target_lock);
>  
>  	error = -ENOENT;
>  	if (!target_sd)
> @@ -140,10 +142,16 @@ void sysfs_delete_link(struct kobject *kobj, struct kobject *targ,
>  			const char *name)
>  {
>  	const void *ns = NULL;
> -	spin_lock(&sysfs_assoc_lock);
> +
> +	/*
> +	 * We don't own @target and it may be removed at any time.
> +	 * Synchronize using sysfs_symlink_target_lock.  See
> +	 * sysfs_remove_dir() for details.
> +	 */
> +	spin_lock(&sysfs_symlink_target_lock);
>  	if (targ->sd)
>  		ns = targ->sd->s_ns;
> -	spin_unlock(&sysfs_assoc_lock);
> +	spin_unlock(&sysfs_symlink_target_lock);
>  	sysfs_hash_and_remove(kobj->sd, name, ns);
>  }
>  
> diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
> index 05d063f..e3aea92 100644
> --- a/fs/sysfs/sysfs.h
> +++ b/fs/sysfs/sysfs.h
> @@ -159,7 +159,7 @@ extern struct kmem_cache *sysfs_dir_cachep;
>   * dir.c
>   */
>  extern struct mutex sysfs_mutex;
> -extern spinlock_t sysfs_assoc_lock;
> +extern spinlock_t sysfs_symlink_target_lock;
>  extern const struct dentry_operations sysfs_dentry_ops;
>  
>  extern const struct file_operations sysfs_dir_operations;