linux-kernel - Re: [PATCH driver-core-next] sysfs: rename sysfs_assoc

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <8761se1jfx.fsf@xmission.com>
Date:	Wed, 30 Oct 2013 15:29:38 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Tejun Heo <tj@...nel.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH driver-core-next] sysfs: rename sysfs_assoc_lock and explain what it's about

Tejun Heo <tj@...nel.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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/