[PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

[PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

[Greg KH]

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Linus noticed that the assignment of sd isn't protected by the lock in
sysfs_remove_dir(), so move the assignment of the variable under the
lock to be safe.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---

Tejun, any objection to this patch?  You consolidated the locks back in
2007 on this function, and nothing has changed there since then, so odds
are it's not a problem, but nice to be safe, right?

 fs/sysfs/dir.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index eab59de4..2609f934 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -856,9 +856,10 @@ void sysfs_remove(struct sysfs_dirent *sd)
  */
 void sysfs_remove_dir(struct kobject *kobj)
 {
-	struct sysfs_dirent *sd = kobj->sd;
+	struct sysfs_dirent *sd;
 
 	spin_lock(&sysfs_assoc_lock);
+	sd = kobj->sd;
 	kobj->sd = NULL;
 	spin_unlock(&sysfs_assoc_lock);
 

Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

[Eric W. Biederman]

Greg KH <gregkh@linuxfoundation.org> writes:

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Linus noticed that the assignment of sd isn't protected by the lock in
> sysfs_remove_dir(), so move the assignment of the variable under the
> lock to be safe.

I don't have a strong feeling either way but how would that matter?
There is only ever one sd associated with a kobj.

And we better be under the sysfs_mutex when the assignment and and
sysfs_remove_dir are called.

> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>
> Tejun, any objection to this patch?  You consolidated the locks back in
> 2007 on this function, and nothing has changed there since then, so odds
> are it's not a problem, but nice to be safe, right?
>
>  fs/sysfs/dir.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index eab59de4..2609f934 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -856,9 +856,10 @@ void sysfs_remove(struct sysfs_dirent *sd)
>   */
>  void sysfs_remove_dir(struct kobject *kobj)
>  {
> -	struct sysfs_dirent *sd = kobj->sd;
> +	struct sysfs_dirent *sd;
>  
>  	spin_lock(&sysfs_assoc_lock);
> +	sd = kobj->sd;
>  	kobj->sd = NULL;
>  	spin_unlock(&sysfs_assoc_lock);
>  

Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

[Linus Torvalds]

On Tue, Oct 29, 2013 at 5:39 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> I don't have a strong feeling either way but how would that matter?
> There is only ever one sd associated with a kobj.

What does that matter? If you have multiple callers, they might try to
free that one sd twice, since they could both see a non-NULL case.

> And we better be under the sysfs_mutex when the assignment and and
> sysfs_remove_dir are called.

Not as far as I can tell. kobject_del() calls sysfs_remove_dir(), and
I'm not seeing why that would be under the mutex.  The only locking I
see is that sysfs_assoc_lock, which _isn't_ held for the reading of
kobj->sd.

Now, there may be other reasons for this all working (like the fact
that only one user ever calls kobject_del() on any particular object,
but it sure as hell isn't obvious. The fact that you seem to be
confused about this only proves my point.

Besides, the "design pattern" of having a lock for the assignment, but
then reading the value without that lock seems to be all kinds of
f*cking stupid, wouldn't you agree? I'm really not seeing how that
could _ever_ be something you make excuses for in the first place.
Even if there is some external locking (which, as far as I can tell,
there is not), that would just raise the question as to what reason
that spinlock has to exist at all.

The code doesn't make any sense with the locking the way it is now. It
might _work_, of course, but it sure as hell doesn't make sense.


              Linus

Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

[Eric W. Biederman]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, Oct 29, 2013 at 5:39 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> I don't have a strong feeling either way but how would that matter?
>> There is only ever one sd associated with a kobj.
>
> What does that matter? If you have multiple callers, they might try to
> free that one sd twice, since they could both see a non-NULL case.

>> And we better be under the sysfs_mutex when the assignment and and
>> sysfs_remove_dir are called.
>
> Not as far as I can tell. kobject_del() calls sysfs_remove_dir(), and
> I'm not seeing why that would be under the mutex.  The only locking I
> see is that sysfs_assoc_lock, which _isn't_ held for the reading of
> kobj->sd.
>
> Now, there may be other reasons for this all working (like the fact
> that only one user ever calls kobject_del() on any particular object,
> but it sure as hell isn't obvious. The fact that you seem to be
> confused about this only proves my point.

I never actually looked deeply into it, and I was working from several
year old memory and a quick skim of the patch when I asked the question.

The protection we have previous to this patch is that syfs_remove_dir is
only sane to call once.

Which makes the code that does:
	if (!dir_sd)
        	return;
in __sysfs_remove_dir very suspicious.   I expect we want a
WARN_ON(!dir_sd);

But the entire directory removal process and working on sysfs stopped
being fun before I managed to get that cleaned up.  And unless I missed
something go by Tejun is going to go generalize this thing before this
bit gets cleaned up.  Sigh.

> Besides, the "design pattern" of having a lock for the assignment, but
> then reading the value without that lock seems to be all kinds of
> f*cking stupid, wouldn't you agree? I'm really not seeing how that
> could _ever_ be something you make excuses for in the first place.
> Even if there is some external locking (which, as far as I can tell,
> there is not), that would just raise the question as to what reason
> that spinlock has to exist at all.

I wasn't making excuses I was just trying to understand the reasoning
for this little patch flying through my inbox.

On an equally bizarre note.  I don't understand why we have a separate
spinlock there.  Looks...  Sigh.  We use a different lock from
everything as a premature optimization so that sysfs_remove_dir could be
modified to just take a sysfs_dirent, and all of the kobject handling
could be removed.

Sigh. It was never in my way and while I was working on the code that
there was a good locking reason for doing that silly thing.

> The code doesn't make any sense with the locking the way it is now. It
> might _work_, of course, but it sure as hell doesn't make sense.

In net I agree.

Eric

Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

[Tejun Heo]

Hey, guys.

On Tue, Oct 29, 2013 at 03:09:39PM -0700, Greg KH wrote:
> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Linus noticed that the assignment of sd isn't protected by the lock in
> sysfs_remove_dir(), so move the assignment of the variable under the
> lock to be safe.
> 
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> 
> Tejun, any objection to this patch?  You consolidated the locks back in
> 2007 on this function, and nothing has changed there since then, so odds
> are it's not a problem, but nice to be safe, right?

IIRC, it was a lock added to specifically address race between kobject
removal and someone else symlinking to it.  In all other cases, the
rule is that the kobject owner is responsible for ensuring that
removal doesn't race against other sysfs operations on the kobject.
However, someone else linking to it didn't have any way to ensure that
kobj->sd won't be removed underneath it - it holds the target kobject
but that doesn't do anything to prevent the target's owner from
disassociating the kobject from its sysfs_dirent.

So, sysfs_assoc_lock is protecting against that specific scenario - it
guarantees that either kobj->sd is disassociated and symlinking sees
that or symlinking grabs sysfs_dirent's reference before
disassociation happens.  It addresses the one exception case of the
general "kobject owner handles exclusion among operations" rule.

Moving "sd = kobj->sd" inside the spinlock doesn't make any difference
but what we really need is a comment explaining what that odd piece of
locking is about.

Thanks.

-- 
tejun

Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

[Tejun Heo]

Hello,

On Tue, Oct 29, 2013 at 10:29:43PM -0700, Eric W. Biederman wrote:
> I never actually looked deeply into it, and I was working from several
> year old memory and a quick skim of the patch when I asked the question.
> 
> The protection we have previous to this patch is that syfs_remove_dir is
> only sane to call once.
> 
> Which makes the code that does:
> 	if (!dir_sd)
>         	return;
> in __sysfs_remove_dir very suspicious.   I expect we want a
> WARN_ON(!dir_sd);

It was always like that, probably in the same spirit as kfree() taking
NULL so that it can be easier, for example, in init failure paths.

> But the entire directory removal process and working on sysfs stopped
> being fun before I managed to get that cleaned up.  And unless I missed
> something go by Tejun is going to go generalize this thing before this
> bit gets cleaned up.  Sigh.

I kept the same behavior for kernfs_remove().  I don't think it's
something we explicitly want to clean up tho?  It's an acceptable
behavior.

> On an equally bizarre note.  I don't understand why we have a separate
> spinlock there.  Looks...  Sigh.  We use a different lock from
> everything as a premature optimization so that sysfs_remove_dir could be
> modified to just take a sysfs_dirent, and all of the kobject handling
> could be removed.
> 
> Sigh. It was never in my way and while I was working on the code that
> there was a good locking reason for doing that silly thing.

Umm... you got it completely wrong.  It's there to address a race
condition between removal and symlinking and has nothing to do with
optimization.

The current odd looking locking on removal side serves a purpose in
making it clear that it isn't synchronizing concurrent removal calls.
Maybe we should rename the lock to sysfs_symlink_target_lock and add
fat comments on both sides?  Or we can make it a mutex and exclude the
entire removal and symlinking, which would probably easier to follow.

Thanks.

-- 
tejun

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

[Tejun Heo]

Hey, again.

How about something like the following?

Thanks.
----- >8 -----
sysfs_assoc_lock is an odd piece of locking.  In general, whoever owns
a kobject is responsible for synchronizing sysfs operations and sysfs
proper assumes that, for example, removal won't race with any other
operation; however, this doesn't work for symlinking because an entity
performing symlink doesn't usually own the target kobject and thus has
no control over its removal.

sysfs_assoc_lock synchronizes symlink operations against kobj->sd
disassociation so that symlink code doesn't end up dereferencing
already freed sysfs_dirent by racing with removal of the target
kobject.

This is quite obscure and the generic name of the lock and lack of
comments make it difficult to understand its role.  Let's rename it to
sysfs_symlink_target_lock and add comments explaining what's going on.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 fs/sysfs/dir.c     |   18 +++++++++++++++---
 fs/sysfs/symlink.c |   20 ++++++++++++++------
 fs/sysfs/sysfs.h   |    2 +-
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index de47ed3..08c6696 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -26,7 +26,7 @@
 #include "sysfs.h"
 
 DEFINE_MUTEX(sysfs_mutex);
-DEFINE_SPINLOCK(sysfs_assoc_lock);
+DEFINE_SPINLOCK(sysfs_symlink_target_lock);
 
 #define to_sysfs_dirent(X) rb_entry((X), struct sysfs_dirent, s_rb)
 
@@ -902,9 +902,21 @@ void sysfs_remove_dir(struct kobject *kobj)
 {
 	struct sysfs_dirent *sd = kobj->sd;
 
-	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.
+	 *
+	 * 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;

Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

[Eric W. Biederman]

Tejun Heo <tj@kernel.org> writes:

> Hello,
>
> On Tue, Oct 29, 2013 at 10:29:43PM -0700, Eric W. Biederman wrote:
>> I never actually looked deeply into it, and I was working from several
>> year old memory and a quick skim of the patch when I asked the question.
>> 
>> The protection we have previous to this patch is that syfs_remove_dir is
>> only sane to call once.
>> 
>> Which makes the code that does:
>> 	if (!dir_sd)
>>         	return;
>> in __sysfs_remove_dir very suspicious.   I expect we want a
>> WARN_ON(!dir_sd);
>
> It was always like that, probably in the same spirit as kfree() taking
> NULL so that it can be easier, for example, in init failure paths.

Which given that we are talking about sysfs either means that no one is
taking advantage of this case or there is some totally broken usage
created by accident and that we aren't warning about.

>> But the entire directory removal process and working on sysfs stopped
>> being fun before I managed to get that cleaned up.  And unless I missed
>> something go by Tejun is going to go generalize this thing before this
>> bit gets cleaned up.  Sigh.
>
> I kept the same behavior for kernfs_remove().  I don't think it's
> something we explicitly want to clean up tho?  It's an acceptable
> behavior.

It has been a long time since I looked but there are or at least were
problematic cases last time I looked.

The pci hotplug layer removes or removed kobjects from sysfs in the
wrong order with parent directories disappearing before their children.

There were leaks in this area.

I fixed the worst of it and everything I had energy to fix when I was
working on sysfs.  But it is a real problem.

One of the particularly problematic things that can happen with sysfs is
that we can get a hotplug event in userspace and then examine sysfs and
not find the attributes of the device because the kernel has not added
them yet.

Which is a particularly good reason to have a campaign against
independent usage of device_create_file and device_remove_file in the
device users.

At which point really the right thing to do when we delete a directory
is to WARN and be very grumpy if there are any attributes in the
directory we were removing.

Being nice here has resulted in buggy semantics exported to userspace,
and buggy kernel code being written.  In a generalized version of sysfs
we don't want to be nice to avoid the existing mess that sysfs sees.

>> On an equally bizarre note.  I don't understand why we have a separate
>> spinlock there.  Looks...  Sigh.  We use a different lock from
>> everything as a premature optimization so that sysfs_remove_dir could be
>> modified to just take a sysfs_dirent, and all of the kobject handling
>> could be removed.
>> 
>> Sigh. It was never in my way and while I was working on the code that
>> there was a good locking reason for doing that silly thing.
>
> Umm... you got it completely wrong.  It's there to address a race
> condition between removal and symlinking and has nothing to do with
> optimization.

No.  It totally is a premature optimization.  There is no reason to add
another lock to sysfs and make understanding of what locks are needed
sto protect data structures something that needs to be thought about.
sysfs_mutex can easily be expanded to cover this role, and in fact all
cases where we take sysfs_assoc_lock today just a few lines later we
take sysfs_mutex.

Furthermore explicitly you state in your original commit when
sysfs_assoc_lock was added that it was to aid in the separation of
kobjects and sysfs dirents.

commit aecdcedaab49ca40620dc7dd70f67ee7269a66c9
Author: Tejun Heo <htejun@gmail.com>
Date:   Thu Jun 14 03:45:15 2007 +0900

    sysfs: implement kobj_sysfs_assoc_lock
    
    kobj->dentry can go away anytime unless the user controls when the
    associated sysfs node is deleted.  This patch implements
    kobj_sysfs_assoc_lock which protects kobj->dentry.  This will be used
    to maintain kobj based API when converting sysfs to use sysfs_dirent
    tree instead of dentry/kobject.
    
    Note that this lock belongs to kobject/driver-model not sysfs.  Once
    sysfs is converted to not use kobject in its interface, this can be
    removed from sysfs.
    
    This is in preparation of object reference simplification.
    
    Signed-off-by: Tejun Heo <htejun@gmail.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

> The current odd looking locking on removal side serves a purpose in
> making it clear that it isn't synchronizing concurrent removal calls.

At the moment the only reason concurrent calls are not properly
synchronized is because the read is outside of the lock.

If you want to avoid concurrent calls WARN_ON(!kobject->sd) is a much
better approach.

But syfs_mutex a few lines later does synchronize concurrent removal
calls, and sysfs_assoc_lock

Which given how sysfs is used is really insane. If we don't support
something we need to warn about it, not 

> Maybe we should rename the lock to sysfs_symlink_target_lock and add
> fat comments on both sides?  Or we can make it a mutex and exclude the
> entire removal and symlinking, which would probably easier to follow.

Agreed.  A mutex to excluse the entire removal and symlinking does sound
easier to follow.  Why don't we call that mutex sysfs_mutex?

Eric

Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

[Eric W. Biederman]

Greg KH <gregkh@linuxfoundation.org> writes:

> From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Linus noticed that the assignment of sd isn't protected by the lock in
> sysfs_remove_dir(), so move the assignment of the variable under the
> lock to be safe.
>
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>

> ---
>
> Tejun, any objection to this patch?  You consolidated the locks back in
> 2007 on this function, and nothing has changed there since then, so odds
> are it's not a problem, but nice to be safe, right?
>
>  fs/sysfs/dir.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index eab59de4..2609f934 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -856,9 +856,10 @@ void sysfs_remove(struct sysfs_dirent *sd)
>   */
>  void sysfs_remove_dir(struct kobject *kobj)
>  {
> -	struct sysfs_dirent *sd = kobj->sd;
> +	struct sysfs_dirent *sd;
>  
>  	spin_lock(&sysfs_assoc_lock);
> +	sd = kobj->sd;
>  	kobj->sd = NULL;
>  	spin_unlock(&sysfs_assoc_lock);
>  

Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

[Tejun Heo]

Hey, Eric.

On Wed, Oct 30, 2013 at 02:41:16PM -0700, Eric W. Biederman wrote:
> Being nice here has resulted in buggy semantics exported to userspace,
> and buggy kernel code being written.  In a generalized version of sysfs
> we don't want to be nice to avoid the existing mess that sysfs sees.

Hmm... given that the generalized version always does recursive
deletion, I don't think the problem you described is too relevant.  It
really is something which can be either way.  Given that other
interfaces including sysfs_put() allow for NULL input, I think
flipping the behavior is likely more churn than worthwhile.

> Which given how sysfs is used is really insane. If we don't support
> something we need to warn about it, not 

Add the warning then?  Given that sysfs_remove() ends up putting the
base reference, it's something ultimately pointless tho.

> > Maybe we should rename the lock to sysfs_symlink_target_lock and add
> > fat comments on both sides?  Or we can make it a mutex and exclude the
> > entire removal and symlinking, which would probably easier to follow.
> 
> Agreed.  A mutex to excluse the entire removal and symlinking does sound
> easier to follow.  Why don't we call that mutex sysfs_mutex?

Well, we can merge any locking but that's likely to lead to some ugly
code.  sysfs core has always been somewhat isolated from kobject
integration and what you're suggesting would violate such isolation.
With the pending sysfs core separation, I don't think it's gonna work
at all.  kobj->sd association is something which is specific to kobj
<-> sysfs interaction.  If you wanna make behavior in that layer more
strictly synchronized, please go ahead but let's please not mix the
two layers.

Thanks.

-- 
tejun

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

[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;

Re: [PATCH] sysfs: move assignment to be under lock in sysfs_remove_dir()

[Greg KH]

On Wed, Oct 30, 2013 at 02:41:16PM -0700, Eric W. Biederman wrote:
> One of the particularly problematic things that can happen with sysfs is
> that we can get a hotplug event in userspace and then examine sysfs and
> not find the attributes of the device because the kernel has not added
> them yet.
> 
> Which is a particularly good reason to have a campaign against
> independent usage of device_create_file and device_remove_file in the
> device users.

I have been working on that over the past few months, there are now
default attribute groups for all things, and those are used to create
the files _before_ the hotplug event goes to userspace.  I still have
more work to do to clean this up, but my goal is to remove
device_create_file() entirely, although we still have some work to go
for the platform driver case, which will take some time.

But we will get there eventually, it's a problem that has to be fixed as
more and more people hit this with multi-core, fast systems these days.

> At which point really the right thing to do when we delete a directory
> is to WARN and be very grumpy if there are any attributes in the
> directory we were removing.

We tried this once, and it was a mess.  But that was a long time ago,
maybe it's better now, I'll look and see...

thanks,

greg k-h

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

[Tejun Heo]

Hello, Eric.

On Wed, Oct 30, 2013 at 03:29:38PM -0700, Eric W. Biederman wrote:
> 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.

The big fat comment is not meant for kobject users.  They have always
assumed the responsibility of not issuing conflicting operations
concurrently.  The comment explains the role of the lock so that a
person reading the code can understand what's going on as it is
confusing why the locking is necessary there when the caller is
supposed to be responsible.

Now, it's true that we *can* make kobject interface to allow multiple
concurrent operations and synchronize internally by generalizing the
locking.  Given how kobject is used, I'm not sure what that'd buy tho,
especially for removal.  Because removal puts the base ref as I wrote
before, it's an operation intrinsically reserved to the owner of the
object.  I'm pretty skeptical about its usefulness but if you think
it's worthwhile, please feel free to give it a shot.

Thanks.

-- 
tejun