[PATCH] sysfs: differentiate between locking links and non-links

[PATCH] sysfs: differentiate between locking links and non-links

[Neil Brown]

Hi,
 I've just spent a while sorting out some lockdep complaints triggered
 by the recent addition of the "s_active" lockdep annotation in sysfs
  (commit 846f99749ab68bbc7f75c74fec305de675b1a1bf)

 Some of them are genuine and I have submitted a fix for those.
 Some are, I think, debatable and I get to that is a minute.  I've
 submitted a fix for them anyway.
 But some are to my mind clearly bogus and I'm hoping that can be
 fixed by the change below (or similar).
 The 'bogus' ones are triggered by writing to a sysfs attribute file
 for which the handler tries to delete a symlink from sysfs.
 This appears to be a recursion on s_active as s_active is held while
 the handler runs and is again needed to effect the delete.  However
 as the thing being deleted is a symlink, it is very clearly a
 different object to the thing triggering the delete, so there is no
 real loop.

 The following patch splits the lockdep context in two - one for
 symlink and one for everything else.  This removes the apparent loop.
 (An example report can be seen in
     http://bugzilla.kernel.org/show_bug.cgi?id=15142).

 The "debatable" dependency loops happen when writing to one attribute
 causes a different attribute to be deleted.  In my (md) case this can
 actually cause a deadlock as both the attributes take the same lock
 while the handler is running.  This is because deleting the attribute
 will block until the all accesses of that attribute have completed (I
 think).
 However it should be possible to delete a name from sysfs while there
 are still accesses pending (it works for normal files!!).  So if
 sysfs could be changed to simply unlink the file and leave deletion to
 happen when the refcount become zero it would certainly make my life
 a lot easier, and allow the removal of some ugly code from md.c.
 I don't know sysfs well enough to suggest a patch though.

Thanks,
NeilBrown



commit 2e502cfe444b68f6ef6b8b2abe83b6112564095b
Author: NeilBrown <neilb@suse.de>
Date:   Wed Feb 10 09:43:45 2010 +1100

    sysfs: differentiate  between locking links and non-links for sysfs
    
    symlinks and non-symlink is sysfs are very different.
    A symlink can never be locked (active) while an attribute
    modification routine is running.  So removing symlink from an
    attribute 'store' routine should be permitted without any lockdep
    warnings.
    
    So split the lockdep context for 's_active' in two, one for symlinks
    and other for everything else.
    
    Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 699f371..e6eeaf6 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -354,7 +354,10 @@ struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type)
 
 	atomic_set(&sd->s_count, 1);
 	atomic_set(&sd->s_active, 0);
-	sysfs_dirent_init_lockdep(sd);
+	if (type & SYSFS_KOBJ_LINK)
+		sysfs_dirent_init_lockdep(sd, "link");
+	else
+		sysfs_dirent_init_lockdep(sd, "non_link");
 
 	sd->s_name = name;
 	sd->s_mode = mode;
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index cdd9377..a83a02b 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -89,11 +89,11 @@ static inline unsigned int sysfs_type(struct sysfs_dirent *sd)
 }
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
-#define sysfs_dirent_init_lockdep(sd)				\
+#define sysfs_dirent_init_lockdep(sd, type)			\
 do {								\
 	static struct lock_class_key __key;			\
 								\
-	lockdep_init_map(&sd->dep_map, "s_active", &__key, 0);	\
+	lockdep_init_map(&sd->dep_map, "s_active_" type, &__key, 0);	\
 } while(0)
 #else
 #define sysfs_dirent_init_lockdep(sd) do {} while(0)

Re: [PATCH] sysfs: differentiate between locking links and non-links

[David Rientjes]

On Wed, 10 Feb 2010, Neil Brown wrote:

> commit 2e502cfe444b68f6ef6b8b2abe83b6112564095b
> Author: NeilBrown <neilb@suse.de>
> Date:   Wed Feb 10 09:43:45 2010 +1100
> 
>     sysfs: differentiate  between locking links and non-links for sysfs
>     
>     symlinks and non-symlink is sysfs are very different.
>     A symlink can never be locked (active) while an attribute
>     modification routine is running.  So removing symlink from an
>     attribute 'store' routine should be permitted without any lockdep
>     warnings.
>     
>     So split the lockdep context for 's_active' in two, one for symlinks
>     and other for everything else.
>     

What happens for hard links such as writing to 
/sys/devices/block/xxx/queue/scheduler to change an I/O scheduler which 
requires sd->dep_map and sd->parent->dep_map in sysfs_get_active_two() to 
pin both?  The call to kobject_del() invokes the destruction that also 
requires sd->dep_map in sysfs_deactivate() because of the s_active lockdep 
annotation.

This is the scenario that is presented in
http://bugzilla.kernel.org/show_bug.cgi?id=15202.

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Américo Wang]

On Wed, Feb 10, 2010 at 9:21 AM, David Rientjes <rientjes@google.com> wrote:
> On Wed, 10 Feb 2010, Neil Brown wrote:
>
>> commit 2e502cfe444b68f6ef6b8b2abe83b6112564095b
>> Author: NeilBrown <neilb@suse.de>
>> Date:   Wed Feb 10 09:43:45 2010 +1100
>>
>>     sysfs: differentiate  between locking links and non-links for sysfs
>>
>>     symlinks and non-symlink is sysfs are very different.
>>     A symlink can never be locked (active) while an attribute
>>     modification routine is running.  So removing symlink from an
>>     attribute 'store' routine should be permitted without any lockdep
>>     warnings.
>>
>>     So split the lockdep context for 's_active' in two, one for symlinks
>>     and other for everything else.
>>
>
> What happens for hard links such as writing to
> /sys/devices/block/xxx/queue/scheduler to change an I/O scheduler which
> requires sd->dep_map and sd->parent->dep_map in sysfs_get_active_two() to
> pin both?  The call to kobject_del() invokes the destruction that also
> requires sd->dep_map in sysfs_deactivate() because of the s_active lockdep
> annotation.
>

This is not related with Neil's case at all.

The I/O scheduler switch case should be a bogus, I am working on
it. We have more similar cases of cpu hotplug. Trust me, I am working
on a fix to all of them, this is not as easy as you may think about.

Thanks.

Re: [PATCH] sysfs: differentiate between locking links and non-links

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 974 bytes --]

On Wed, 10 Feb 2010, Américo Wang wrote:

> > What happens for hard links such as writing to
> > /sys/devices/block/xxx/queue/scheduler to change an I/O scheduler which
> > requires sd->dep_map and sd->parent->dep_map in sysfs_get_active_two() to
> > pin both?  The call to kobject_del() invokes the destruction that also
> > requires sd->dep_map in sysfs_deactivate() because of the s_active lockdep
> > annotation.
> >
> 
> This is not related with Neil's case at all.
> 
> The I/O scheduler switch case should be a bogus, I am working on
> it. We have more similar cases of cpu hotplug. Trust me, I am working
> on a fix to all of them, this is not as easy as you may think about.
> 

You should be able to reuse Neil's sysfs_dirent_init_lockdep(sd, type) to 
seperate the lock classes for the sd getting pinned in 
sysfs_get_active_two() from sysfs_deactivate(), although using subclasses 
would probably be optimal since there is a clear parent -> child relationship.

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Américo Wang]

On Wed, Feb 10, 2010 at 11:05 AM, David Rientjes <rientjes@google.com> wrote:
> On Wed, 10 Feb 2010, Américo Wang wrote:
>
>> > What happens for hard links such as writing to
>> > /sys/devices/block/xxx/queue/scheduler to change an I/O scheduler which
>> > requires sd->dep_map and sd->parent->dep_map in sysfs_get_active_two() to
>> > pin both?  The call to kobject_del() invokes the destruction that also
>> > requires sd->dep_map in sysfs_deactivate() because of the s_active lockdep
>> > annotation.
>> >
>>
>> This is not related with Neil's case at all.
>>
>> The I/O scheduler switch case should be a bogus, I am working on
>> it. We have more similar cases of cpu hotplug. Trust me, I am working
>> on a fix to all of them, this is not as easy as you may think about.
>>
>
> You should be able to reuse Neil's sysfs_dirent_init_lockdep(sd, type) to
> seperate the lock classes for the sd getting pinned in
> sysfs_get_active_two() from sysfs_deactivate(), although using subclasses
> would probably be optimal since there is a clear parent -> child relationship.

Yeah, basically, my fix is also adding a separate lockdep class, but
at a different
level. I will send the fix as soon as I finish it.

Thanks.

Re: [PATCH] sysfs: differentiate between locking links and non-links

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 702 bytes --]

On Wed, 10 Feb 2010, Américo Wang wrote:

> > You should be able to reuse Neil's sysfs_dirent_init_lockdep(sd, type) to
> > seperate the lock classes for the sd getting pinned in
> > sysfs_get_active_two() from sysfs_deactivate(), although using subclasses
> > would probably be optimal since there is a clear parent -> child relationship.
> 
> Yeah, basically, my fix is also adding a separate lockdep class, but
> at a different
> level. I will send the fix as soon as I finish it.
> 

They shouldn't be entirely seperate classes for your "mutable" cases since 
there will always be a parent -> child relationship, they should be 
subclasses under the same lockclass at a SINGLE_DEPTH_NESTING level.

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Américo Wang]

On Wed, Feb 10, 2010 at 11:19 AM, David Rientjes <rientjes@google.com> wrote:
> On Wed, 10 Feb 2010, Américo Wang wrote:
>
>> > You should be able to reuse Neil's sysfs_dirent_init_lockdep(sd, type) to
>> > seperate the lock classes for the sd getting pinned in
>> > sysfs_get_active_two() from sysfs_deactivate(), although using subclasses
>> > would probably be optimal since there is a clear parent -> child relationship.
>>
>> Yeah, basically, my fix is also adding a separate lockdep class, but
>> at a different
>> level. I will send the fix as soon as I finish it.
>>
>
> They shouldn't be entirely seperate classes for your "mutable" cases since
> there will always be a parent -> child relationship, they should be
> subclasses under the same lockclass at a SINGLE_DEPTH_NESTING level.

The relationship is not always clear, take the cpu hotplug case as an example,
there are plenty of sysfs files will be removed if we take one cpu
offline, like mce
files, which are not even under the same directory with cpu sysfs
device directory.

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Américo Wang]

On Wed, Feb 10, 2010 at 9:09 AM, Neil Brown <neilb@suse.de> wrote:
>
> Hi,
>  I've just spent a while sorting out some lockdep complaints triggered
>  by the recent addition of the "s_active" lockdep annotation in sysfs
>  (commit 846f99749ab68bbc7f75c74fec305de675b1a1bf)
>
>  Some of them are genuine and I have submitted a fix for those.
>  Some are, I think, debatable and I get to that is a minute.  I've
>  submitted a fix for them anyway.
>  But some are to my mind clearly bogus and I'm hoping that can be
>  fixed by the change below (or similar).
>  The 'bogus' ones are triggered by writing to a sysfs attribute file
>  for which the handler tries to delete a symlink from sysfs.
>  This appears to be a recursion on s_active as s_active is held while
>  the handler runs and is again needed to effect the delete.  However
>  as the thing being deleted is a symlink, it is very clearly a
>  different object to the thing triggering the delete, so there is no
>  real loop.
>
>  The following patch splits the lockdep context in two - one for
>  symlink and one for everything else.  This removes the apparent loop.
>  (An example report can be seen in
>     http://bugzilla.kernel.org/show_bug.cgi?id=15142).
>
>  The "debatable" dependency loops happen when writing to one attribute
>  causes a different attribute to be deleted.  In my (md) case this can
>  actually cause a deadlock as both the attributes take the same lock
>  while the handler is running.  This is because deleting the attribute
>  will block until the all accesses of that attribute have completed (I
>  think).
>  However it should be possible to delete a name from sysfs while there
>  are still accesses pending (it works for normal files!!).  So if
>  sysfs could be changed to simply unlink the file and leave deletion to
>  happen when the refcount become zero it would certainly make my life
>  a lot easier, and allow the removal of some ugly code from md.c.
>  I don't know sysfs well enough to suggest a patch though.
>

Hi, Neil,

Thanks for your patch.

This bug report is new for me. Recently we received lots of sysfs lockdep
warnings, I am working on a patch to fix all the bogus ones.

However, this one is _not_ similar to the other cases, as you decribed.
This patch could fix the problem, but not a good fix, IMO. We need more
work in sysfs layer to fix this kind of things. I will take care of this.

Thanks.

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Tejun Heo]

Hello,

On 02/10/2010 11:08 AM, Américo Wang wrote:
> This bug report is new for me. Recently we received lots of sysfs lockdep
> warnings, I am working on a patch to fix all the bogus ones.
> 
> However, this one is _not_ similar to the other cases, as you decribed.
> This patch could fix the problem, but not a good fix, IMO. We need more
> work in sysfs layer to fix this kind of things. I will take care of this.

Can't we just give each s_active lock a separate class?  Would that be
too costly?

Thanks.

-- 
tejun

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Américo Wang]

On Wed, Feb 10, 2010 at 10:19 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On 02/10/2010 11:08 AM, Américo Wang wrote:
>> This bug report is new for me. Recently we received lots of sysfs lockdep
>> warnings, I am working on a patch to fix all the bogus ones.
>>
>> However, this one is _not_ similar to the other cases, as you decribed.
>> This patch could fix the problem, but not a good fix, IMO. We need more
>> work in sysfs layer to fix this kind of things. I will take care of this.
>
> Can't we just give each s_active lock a separate class?  Would that be
> too costly?
>

Not because it is expensive or not, it is because whether it hits the real
problem.

What I am doing is trying to add a "mutable" flag to sysfs files, those files
could be removed from kernel during some changes, e.g. cpu hotplug, I/O
scheduler switch. I add a new lockdep class for all of them, so that will be
safe for all cases like this.

If I understand this case correctly, it is not that different with the
rest cases
that I met, thus should be included into my fix.

Thanks.

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Eric W. Biederman]

Tejun Heo <tj@kernel.org> writes:

> Hello,
>
> On 02/10/2010 11:08 AM, Américo Wang wrote:
>> This bug report is new for me. Recently we received lots of sysfs lockdep
>> warnings, I am working on a patch to fix all the bogus ones.
>> 
>> However, this one is _not_ similar to the other cases, as you decribed.
>> This patch could fix the problem, but not a good fix, IMO. We need more
>> work in sysfs layer to fix this kind of things. I will take care of this.
>
> Can't we just give each s_active lock a separate class?  Would that be
> too costly?

When I asked the question earlier I was told that that locking classes
require static storage.  Where would that static storage come from?

Eric

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Tejun Heo]

On 02/10/2010 05:03 PM, Eric W. Biederman wrote:
> Tejun Heo <tj@kernel.org> writes:
> 
>> Hello,
>>
>> On 02/10/2010 11:08 AM, Américo Wang wrote:
>>> This bug report is new for me. Recently we received lots of sysfs lockdep
>>> warnings, I am working on a patch to fix all the bogus ones.
>>>
>>> However, this one is _not_ similar to the other cases, as you decribed.
>>> This patch could fix the problem, but not a good fix, IMO. We need more
>>> work in sysfs layer to fix this kind of things. I will take care of this.
>>
>> Can't we just give each s_active lock a separate class?  Would that be
>> too costly?
> 
> When I asked the question earlier I was told that that locking classes
> require static storage.  Where would that static storage come from?

Maybe I'm glossly misunderstanding it but wouldn't embedding struct
lockdep_map into sysfs_node as in work_struct do the trick?

Thanks.

-- 
tejun

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Eric W. Biederman]

Tejun Heo <tj@kernel.org> writes:

> On 02/10/2010 05:03 PM, Eric W. Biederman wrote:
>> Tejun Heo <tj@kernel.org> writes:
>> 
>>> Hello,
>>>
>>> On 02/10/2010 11:08 AM, Américo Wang wrote:
>>>> This bug report is new for me. Recently we received lots of sysfs lockdep
>>>> warnings, I am working on a patch to fix all the bogus ones.
>>>>
>>>> However, this one is _not_ similar to the other cases, as you decribed.
>>>> This patch could fix the problem, but not a good fix, IMO. We need more
>>>> work in sysfs layer to fix this kind of things. I will take care of this.
>>>
>>> Can't we just give each s_active lock a separate class?  Would that be
>>> too costly?
>> 
>> When I asked the question earlier I was told that that locking classes
>> require static storage.  Where would that static storage come from?
>
> Maybe I'm glossly misunderstanding it but wouldn't embedding struct
> lockdep_map into sysfs_node as in work_struct do the trick?

In lockdep_init_map there is the following check:

	/*
	 * Sanity check, the lock-class key must be persistent:
	 */
	if (!static_obj(key)) {
		printk("BUG: key %p not in .data!\n", key);
		DEBUG_LOCKS_WARN_ON(1);
		return;
	}

It needs playing with but I think we can embed something in struct
attribute, and simply disallow dynamically allocated instances of
struct attribute.

Eric

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Greg KH]

On Wed, Feb 10, 2010 at 10:25:21AM -0800, Eric W. Biederman wrote:
> Tejun Heo <tj@kernel.org> writes:
> 
> > On 02/10/2010 05:03 PM, Eric W. Biederman wrote:
> >> Tejun Heo <tj@kernel.org> writes:
> >> 
> >>> Hello,
> >>>
> >>> On 02/10/2010 11:08 AM, Américo Wang wrote:
> >>>> This bug report is new for me. Recently we received lots of sysfs lockdep
> >>>> warnings, I am working on a patch to fix all the bogus ones.
> >>>>
> >>>> However, this one is _not_ similar to the other cases, as you decribed.
> >>>> This patch could fix the problem, but not a good fix, IMO. We need more
> >>>> work in sysfs layer to fix this kind of things. I will take care of this.
> >>>
> >>> Can't we just give each s_active lock a separate class?  Would that be
> >>> too costly?
> >> 
> >> When I asked the question earlier I was told that that locking classes
> >> require static storage.  Where would that static storage come from?
> >
> > Maybe I'm glossly misunderstanding it but wouldn't embedding struct
> > lockdep_map into sysfs_node as in work_struct do the trick?
> 
> In lockdep_init_map there is the following check:
> 
> 	/*
> 	 * Sanity check, the lock-class key must be persistent:
> 	 */
> 	if (!static_obj(key)) {
> 		printk("BUG: key %p not in .data!\n", key);
> 		DEBUG_LOCKS_WARN_ON(1);
> 		return;
> 	}
> 
> It needs playing with but I think we can embed something in struct
> attribute, and simply disallow dynamically allocated instances of
> struct attribute.

I think some code dynamically creates attributes today, as this has
never been a restriction.

So I don't know if this is going to work :(

thanks,

greg k-h

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Wed, Feb 10, 2010 at 10:25:21AM -0800, Eric W. Biederman wrote:
>> Tejun Heo <tj@kernel.org> writes:
>> 
>> > On 02/10/2010 05:03 PM, Eric W. Biederman wrote:
>> >> Tejun Heo <tj@kernel.org> writes:
>> >> 
>> >>> Hello,
>> >>>
>> >>> On 02/10/2010 11:08 AM, Américo Wang wrote:
>> >>>> This bug report is new for me. Recently we received lots of sysfs lockdep
>> >>>> warnings, I am working on a patch to fix all the bogus ones.
>> >>>>
>> >>>> However, this one is _not_ similar to the other cases, as you decribed.
>> >>>> This patch could fix the problem, but not a good fix, IMO. We need more
>> >>>> work in sysfs layer to fix this kind of things. I will take care of this.
>> >>>
>> >>> Can't we just give each s_active lock a separate class?  Would that be
>> >>> too costly?
>> >> 
>> >> When I asked the question earlier I was told that that locking classes
>> >> require static storage.  Where would that static storage come from?
>> >
>> > Maybe I'm glossly misunderstanding it but wouldn't embedding struct
>> > lockdep_map into sysfs_node as in work_struct do the trick?
>> 
>> In lockdep_init_map there is the following check:
>> 
>> 	/*
>> 	 * Sanity check, the lock-class key must be persistent:
>> 	 */
>> 	if (!static_obj(key)) {
>> 		printk("BUG: key %p not in .data!\n", key);
>> 		DEBUG_LOCKS_WARN_ON(1);
>> 		return;
>> 	}
>> 
>> It needs playing with but I think we can embed something in struct
>> attribute, and simply disallow dynamically allocated instances of
>> struct attribute.
>
> I think some code dynamically creates attributes today, as this has
> never been a restriction.
>
> So I don't know if this is going to work :(

I need to see how locks do this, but they face the same problem.  For
normal locks this is resolved by having a requirement to call a special
initializer in the dynamic case.  Adding that requirement for the
rare dynamically allocated sysfs attributes seems reasonable.

Additionally sysfs attributes are exactly the right granularity for
lock classes because that is where the behavior is the same or
changes.

We have 845 instances of static struct attributes which certainly makes
that the dominant case and worth aiming at.


Eric

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Tejun Heo]

Hello,

On 02/11/2010 10:31 AM, Eric W. Biederman wrote:
>> I think some code dynamically creates attributes today, as this has
>> never been a restriction.
>>
>> So I don't know if this is going to work :(
> 
> I need to see how locks do this, but they face the same problem.  For
> normal locks this is resolved by having a requirement to call a special
> initializer in the dynamic case.  Adding that requirement for the
> rare dynamically allocated sysfs attributes seems reasonable.

Yeah, this is the same problem all other constructus face too and the
reason why different variants of initializers are introduced for
lockdep support.  For dynamic ones, making the initializer declare
static key and using it to initialize lockdep_map should work.

> Additionally sysfs attributes are exactly the right granularity for
> lock classes because that is where the behavior is the same or
> changes.

Yeap.

> We have 845 instances of static struct attributes which certainly makes
> that the dominant case and worth aiming at.

Yeah, and sysfs will be following the usual convention of dealing with
these issues, which is the right thing to do.

Thanks.

-- 
tejun

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Eric W. Biederman]

Tejun Heo <tj@kernel.org> writes:

> Hello,
>
> On 02/11/2010 10:31 AM, Eric W. Biederman wrote:
>>> I think some code dynamically creates attributes today, as this has
>>> never been a restriction.
>>>
>>> So I don't know if this is going to work :(
>> 
>> I need to see how locks do this, but they face the same problem.  For
>> normal locks this is resolved by having a requirement to call a special
>> initializer in the dynamic case.  Adding that requirement for the
>> rare dynamically allocated sysfs attributes seems reasonable.
>
> Yeah, this is the same problem all other constructus face too and the
> reason why different variants of initializers are introduced for
> lockdep support.  For dynamic ones, making the initializer declare
> static key and using it to initialize lockdep_map should work.
>
>> Additionally sysfs attributes are exactly the right granularity for
>> lock classes because that is where the behavior is the same or
>> changes.
>
> Yeap.
>
>> We have 845 instances of static struct attributes which certainly makes
>> that the dominant case and worth aiming at.
>
> Yeah, and sysfs will be following the usual convention of dealing with
> these issues, which is the right thing to do.

I have been playing with it and so far the code doesn't seem too bad. I have
however come across another misfeature of sysfs.  sysfs_get_active_two appears
to be unnecessary overkill.

The purpose of the active references are to allows us to block when
removing sysfs entries that have custom methods so we don't remove
modules or those custom methods don't remove access data structures
after the files have been removed.  Further sysfs_remove_dir remove
all elements in the directory before removing the directory itself, so
there is no chance we will remove a directory with active children.

Tejun do you know of any other reason we want sysfs_get_active_two?

If not I think we can make active references apply exclusively to attributes.

Eric

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Tejun Heo]

Hello, Eric.

On 02/12/2010 03:08 AM, Eric W. Biederman wrote:
> I have been playing with it and so far the code doesn't seem too bad. I have
> however come across another misfeature of sysfs.  sysfs_get_active_two appears
> to be unnecessary overkill.
>
> The purpose of the active references are to allows us to block when
> removing sysfs entries that have custom methods so we don't remove
> modules or those custom methods don't remove access data structures
> after the files have been removed.  Further sysfs_remove_dir remove
> all elements in the directory before removing the directory itself, so
> there is no chance we will remove a directory with active children.
> 
> Tejun do you know of any other reason we want sysfs_get_active_two?
> 
> If not I think we can make active references apply exclusively to
> attributes.

Yeah, it's necessary for something which I can't remember from the top
of my head ATM.  It maybe has something to do with attributes not
holding reference to the owning module while the parent kobj does.
I'll dig in but IIRC it's not there just for fun.  Is it difficult to
do the lockdep annotation without removing get_active_two?

Thanks.

-- 
tejun

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Eric W. Biederman]

Tejun Heo <tj@kernel.org> writes:

> Hello, Eric.
>
> On 02/12/2010 03:08 AM, Eric W. Biederman wrote:
>> I have been playing with it and so far the code doesn't seem too bad. I have
>> however come across another misfeature of sysfs.  sysfs_get_active_two appears
>> to be unnecessary overkill.
>>
>> The purpose of the active references are to allows us to block when
>> removing sysfs entries that have custom methods so we don't remove
>> modules or those custom methods don't remove access data structures
>> after the files have been removed.  Further sysfs_remove_dir remove
>> all elements in the directory before removing the directory itself, so
>> there is no chance we will remove a directory with active children.
>> 
>> Tejun do you know of any other reason we want sysfs_get_active_two?
>> 
>> If not I think we can make active references apply exclusively to
>> attributes.
>
> Yeah, it's necessary for something which I can't remember from the top
> of my head ATM.  It maybe has something to do with attributes not
> holding reference to the owning module while the parent kobj does.
> I'll dig in but IIRC it's not there just for fun.  Is it difficult to
> do the lockdep annotation without removing get_active_two?

Probably double the code and testing, plus.  I could not figure out
where to stash a lock_class_key for directories.  So it certainly would
be a pain to keep get_active_two just because.

If you can figure out what the value of an active reference on a directory
is we can keep them.

My reasoning is that we effectively get an active reference on directories
for free because we have to delete the children before we can delete a
directory.

I do know we have to go up to the directory to get the kobject.  So
from that perspective it makes sense.

It would not surprise me if there was a reason that is now gone
that required the active references on directories.  sysfs has gone through
a lot of cleanups since that code was added.

Eric

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Eric W. Biederman]

Tejun Heo <tj@kernel.org> writes:

> Hello, Eric.
>
> On 02/12/2010 03:08 AM, Eric W. Biederman wrote:
>> I have been playing with it and so far the code doesn't seem too bad. I have
>> however come across another misfeature of sysfs.  sysfs_get_active_two appears
>> to be unnecessary overkill.
>>
>> The purpose of the active references are to allows us to block when
>> removing sysfs entries that have custom methods so we don't remove
>> modules or those custom methods don't remove access data structures
>> after the files have been removed.  Further sysfs_remove_dir remove
>> all elements in the directory before removing the directory itself, so
>> there is no chance we will remove a directory with active children.
>> 
>> Tejun do you know of any other reason we want sysfs_get_active_two?
>> 
>> If not I think we can make active references apply exclusively to
>> attributes.
>
> Yeah, it's necessary for something which I can't remember from the top
> of my head ATM.  It maybe has something to do with attributes not
> holding reference to the owning module while the parent kobj does.
> I'll dig in but IIRC it's not there just for fun.  Is it difficult to
> do the lockdep annotation without removing get_active_two?

Probably double the code and testing, plus.  I could not figure out
where to stash a lock_class_key for directories.  So it certainly would
be a pain to keep get_active_two just because.

If you can figure out what the value of an active reference on a directory
is we can keep them.

My reasoning is that we effectively get an active reference on directories
for free because we have to delete the children before we can delete a
directory.

I do know we have to go up to the directory to get the kobject.  So
from that perspective it makes sense.

It would not surprise me if there was a reason that is now gone
that required the active references on directories.  sysfs has gone through
a lot of cleanups since that code was added.

Eric

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Tejun Heo]

Hello,

On 02/12/2010 10:20 AM, Eric W. Biederman wrote:
> My reasoning is that we effectively get an active reference on directories
> for free because we have to delete the children before we can delete a
> directory.

Yeah, as long as we guarantee that all children are removed before the
parent, I think we're in the clear.  The group handling seems
problematic (w/ or w/o the patch) but yeap Ack on removing
get_active_two.

Thanks.

-- 
tejun

[PATCH 0/4] Better sysfs lockdep

[Eric W. Biederman]

It turns out that it is pretty simple to rework the lockdep support
to give each sysfs attribute it's own lockdep class, which should
kill the annoying false positives we have been seeing lately.

The one caveat is that it requires dynamically allocated sysfs
attributes to be explicitly initialized.  Luckily it is brainless
trivial whack-a-mole and took maybe 15 minutes to find all of the
dynamically allocates sysfs attributes on my test machine, because
the errors show up when the attributes are registered with sysfs,
and all of the fixes are one-liners.

Hopefully with this we can head back to fixing the real issues.

Eric

[PATCH 1/4] sysfs: Remove sysfs_get/put_active_two

[Eric W. Biederman]

It turns out that holding an active reference on a directory is
pointless.  The purpose of the active references are to allows us to
block when removing sysfs entries that have custom methods so we don't
remove modules while running modular code and to keep those custom
methods from accessing data structures after the files have been
removed.  Further sysfs_remove_dir remove all elements in the
directory before removing the directory itself, so there is no chance
we will remove a directory with active children.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/bin.c   |   50 +++++++++++++++++++++++++-------------------------
 fs/sysfs/dir.c   |   43 ++-----------------------------------------
 fs/sysfs/file.c  |   18 +++++++++---------
 fs/sysfs/sysfs.h |    4 ++--
 4 files changed, 38 insertions(+), 77 deletions(-)

diff --git a/fs/sysfs/bin.c b/fs/sysfs/bin.c
index a0a500a..e9d2935 100644
--- a/fs/sysfs/bin.c
+++ b/fs/sysfs/bin.c
@@ -54,14 +54,14 @@ fill_read(struct dentry *dentry, char *buffer, loff_t off, size_t count)
 	int rc;
 
 	/* need attr_sd for attr, its parent for kobj */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	rc = -EIO;
 	if (attr->read)
 		rc = attr->read(kobj, attr, buffer, off, count);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 
 	return rc;
 }
@@ -125,14 +125,14 @@ flush_write(struct dentry *dentry, char *buffer, loff_t offset, size_t count)
 	int rc;
 
 	/* need attr_sd for attr, its parent for kobj */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	rc = -EIO;
 	if (attr->write)
 		rc = attr->write(kobj, attr, buffer, offset, count);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 
 	return rc;
 }
@@ -184,12 +184,12 @@ static void bin_vma_open(struct vm_area_struct *vma)
 	if (!bb->vm_ops || !bb->vm_ops->open)
 		return;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return;
 
 	bb->vm_ops->open(vma);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 }
 
 static void bin_vma_close(struct vm_area_struct *vma)
@@ -201,12 +201,12 @@ static void bin_vma_close(struct vm_area_struct *vma)
 	if (!bb->vm_ops || !bb->vm_ops->close)
 		return;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return;
 
 	bb->vm_ops->close(vma);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 }
 
 static int bin_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
@@ -219,12 +219,12 @@ static int bin_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	if (!bb->vm_ops || !bb->vm_ops->fault)
 		return VM_FAULT_SIGBUS;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return VM_FAULT_SIGBUS;
 
 	ret = bb->vm_ops->fault(vma, vmf);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return ret;
 }
 
@@ -241,12 +241,12 @@ static int bin_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 	if (!bb->vm_ops->page_mkwrite)
 		return 0;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return VM_FAULT_SIGBUS;
 
 	ret = bb->vm_ops->page_mkwrite(vma, vmf);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return ret;
 }
 
@@ -261,12 +261,12 @@ static int bin_access(struct vm_area_struct *vma, unsigned long addr,
 	if (!bb->vm_ops || !bb->vm_ops->access)
 		return -EINVAL;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -EINVAL;
 
 	ret = bb->vm_ops->access(vma, addr, buf, len, write);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return ret;
 }
 
@@ -281,12 +281,12 @@ static int bin_set_policy(struct vm_area_struct *vma, struct mempolicy *new)
 	if (!bb->vm_ops || !bb->vm_ops->set_policy)
 		return 0;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -EINVAL;
 
 	ret = bb->vm_ops->set_policy(vma, new);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return ret;
 }
 
@@ -301,12 +301,12 @@ static struct mempolicy *bin_get_policy(struct vm_area_struct *vma,
 	if (!bb->vm_ops || !bb->vm_ops->get_policy)
 		return vma->vm_policy;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return vma->vm_policy;
 
 	pol = bb->vm_ops->get_policy(vma, addr);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return pol;
 }
 
@@ -321,12 +321,12 @@ static int bin_migrate(struct vm_area_struct *vma, const nodemask_t *from,
 	if (!bb->vm_ops || !bb->vm_ops->migrate)
 		return 0;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return 0;
 
 	ret = bb->vm_ops->migrate(vma, from, to, flags);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return ret;
 }
 #endif
@@ -356,7 +356,7 @@ static int mmap(struct file *file, struct vm_area_struct *vma)
 
 	/* need attr_sd for attr, its parent for kobj */
 	rc = -ENODEV;
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		goto out_unlock;
 
 	rc = -EINVAL;
@@ -384,7 +384,7 @@ static int mmap(struct file *file, struct vm_area_struct *vma)
 	bb->vm_ops = vma->vm_ops;
 	vma->vm_ops = &bin_vm_ops;
 out_put:
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 out_unlock:
 	mutex_unlock(&bb->mutex);
 
@@ -399,7 +399,7 @@ static int open(struct inode * inode, struct file * file)
 	int error;
 
 	/* binary file operations requires both @sd and its parent */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	error = -EACCES;
@@ -426,11 +426,11 @@ static int open(struct inode * inode, struct file * file)
 	mutex_unlock(&sysfs_bin_lock);
 
 	/* open succeeded, put active references */
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return 0;
 
  err_out:
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	kfree(bb);
 	return error;
 }
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 5c4703d..1bdc42f 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -93,7 +93,7 @@ static void sysfs_unlink_sibling(struct sysfs_dirent *sd)
  *	RETURNS:
  *	Pointer to @sd on success, NULL on failure.
  */
-static struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
+struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
 {
 	if (unlikely(!sd))
 		return NULL;
@@ -124,7 +124,7 @@ static struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
  *	Put an active reference to @sd.  This function is noop if @sd
  *	is NULL.
  */
-static void sysfs_put_active(struct sysfs_dirent *sd)
+void sysfs_put_active(struct sysfs_dirent *sd)
 {
 	struct completion *cmpl;
 	int v;
@@ -145,45 +145,6 @@ static void sysfs_put_active(struct sysfs_dirent *sd)
 }
 
 /**
- *	sysfs_get_active_two - get active references to sysfs_dirent and parent
- *	@sd: sysfs_dirent of interest
- *
- *	Get active reference to @sd and its parent.  Parent's active
- *	reference is grabbed first.  This function is noop if @sd is
- *	NULL.
- *
- *	RETURNS:
- *	Pointer to @sd on success, NULL on failure.
- */
-struct sysfs_dirent *sysfs_get_active_two(struct sysfs_dirent *sd)
-{
-	if (sd) {
-		if (sd->s_parent && unlikely(!sysfs_get_active(sd->s_parent)))
-			return NULL;
-		if (unlikely(!sysfs_get_active(sd))) {
-			sysfs_put_active(sd->s_parent);
-			return NULL;
-		}
-	}
-	return sd;
-}
-
-/**
- *	sysfs_put_active_two - put active references to sysfs_dirent and parent
- *	@sd: sysfs_dirent of interest
- *
- *	Put active references to @sd and its parent.  This function is
- *	noop if @sd is NULL.
- */
-void sysfs_put_active_two(struct sysfs_dirent *sd)
-{
-	if (sd) {
-		sysfs_put_active(sd);
-		sysfs_put_active(sd->s_parent);
-	}
-}
-
-/**
  *	sysfs_deactivate - deactivate sysfs_dirent
  *	@sd: sysfs_dirent to deactivate
  *
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index dc30d9e..8d6bd65 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -85,13 +85,13 @@ static int fill_read_buffer(struct dentry * dentry, struct sysfs_buffer * buffer
 		return -ENOMEM;
 
 	/* need attr_sd for attr and ops, its parent for kobj */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	buffer->event = atomic_read(&attr_sd->s_attr.open->event);
 	count = ops->show(kobj, attr_sd->s_attr.attr, buffer->page);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 
 	/*
 	 * The code works fine with PAGE_SIZE return but it's likely to
@@ -203,12 +203,12 @@ flush_write_buffer(struct dentry * dentry, struct sysfs_buffer * buffer, size_t
 	int rc;
 
 	/* need attr_sd for attr and ops, its parent for kobj */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	rc = ops->store(kobj, attr_sd->s_attr.attr, buffer->page, count);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 
 	return rc;
 }
@@ -344,7 +344,7 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 		memmove(last_sysfs_file, p, strlen(p) + 1);
 
 	/* need attr_sd for attr and ops, its parent for kobj */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	/* every kobject with an attribute needs a ktype assigned */
@@ -393,13 +393,13 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 		goto err_free;
 
 	/* open succeeded, put active references */
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return 0;
 
  err_free:
 	kfree(buffer);
  err_out:
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return error;
 }
 
@@ -437,12 +437,12 @@ static unsigned int sysfs_poll(struct file *filp, poll_table *wait)
 	struct sysfs_open_dirent *od = attr_sd->s_attr.open;
 
 	/* need parent for the kobj, grab both */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		goto trigger;
 
 	poll_wait(filp, &od->poll, wait);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 
 	if (buffer->event != atomic_read(&od->event))
 		goto trigger;
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index cdd9377..bb7723c 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -124,8 +124,8 @@ extern const struct file_operations sysfs_dir_operations;
 extern const struct inode_operations sysfs_dir_inode_operations;
 
 struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd);
-struct sysfs_dirent *sysfs_get_active_two(struct sysfs_dirent *sd);
-void sysfs_put_active_two(struct sysfs_dirent *sd);
+struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd);
+void sysfs_put_active(struct sysfs_dirent *sd);
 void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
 		       struct sysfs_dirent *parent_sd);
 int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
-- 
1.6.5.2.143.g8cc62

[PATCH 2/4] sysfs: Only take active references on attributes.

[Eric W. Biederman]

If we exclude directories and symlinks from the set of sysfs
dirents where we need active references we are left with
sysfs attributes (binary or not).

- Tweak sysfs_deactivate to only do something on attributes
- Move lockdep initialization into sysfs_file_add_mode to
  limit it to just attributes.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/dir.c   |    5 ++++-
 fs/sysfs/file.c  |    1 +
 fs/sysfs/sysfs.h |    1 +
 3 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 1bdc42f..481fdec 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -156,6 +156,10 @@ static void sysfs_deactivate(struct sysfs_dirent *sd)
 	int v;
 
 	BUG_ON(sd->s_sibling || !(sd->s_flags & SYSFS_FLAG_REMOVED));
+
+	if (!(sysfs_type(sd) & SYSFS_ACTIVE_REF))
+		return;
+
 	sd->s_sibling = (void *)&wait;
 
 	rwsem_acquire(&sd->dep_map, 0, 0, _RET_IP_);
@@ -315,7 +319,6 @@ struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type)
 
 	atomic_set(&sd->s_count, 1);
 	atomic_set(&sd->s_active, 0);
-	sysfs_dirent_init_lockdep(sd);
 
 	sd->s_name = name;
 	sd->s_mode = mode;
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 8d6bd65..8eaae5d 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -509,6 +509,7 @@ int sysfs_add_file_mode(struct sysfs_dirent *dir_sd,
 	if (!sd)
 		return -ENOMEM;
 	sd->s_attr.attr = (void *)attr;
+	sysfs_dirent_init_lockdep(sd);
 
 	sysfs_addrm_start(&acxt, dir_sd);
 	rc = sysfs_add_one(&acxt, sd);
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index bb7723c..7db6884 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -79,6 +79,7 @@ struct sysfs_dirent {
 #define SYSFS_KOBJ_BIN_ATTR		0x0004
 #define SYSFS_KOBJ_LINK			0x0008
 #define SYSFS_COPY_NAME			(SYSFS_DIR | SYSFS_KOBJ_LINK)
+#define SYSFS_ACTIVE_REF		(SYSFS_KOBJ_ATTR | SYSFS_KOBJ_BIN_ATTR)
 
 #define SYSFS_FLAG_MASK			~SYSFS_TYPE_MASK
 #define SYSFS_FLAG_REMOVED		0x0200
-- 
1.6.5.2.143.g8cc62

[PATCH 3/4] sysfs: Use one lockdep class per sysfs attribute.

[Eric W. Biederman]

Acknowledge that the logical sysfs rwsem has one instance per
sysfs attribute with different locking depencencies for different
attributes.

There is a sysfs idiom where writing to one sysfs file causes the
addition or removal of other sysfs files.   Lumping all of the
sysfs attributes together in one lock class causes lockdep to
generate lots of false positives.

This introduces the requirement that non-static sysfs attributes
need to be initialized with sysfs_attr_init or sysfs_bin_attr_init.
Strictly speaking this requirement only exists when lockdep is
enabled, and when lockdep is enabled we get a bit fat warning
if this requirement is not met.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/sysfs.h      |    7 +++++--
 include/linux/sysfs.h |   18 ++++++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 7db6884..37e0e08 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -92,9 +92,12 @@ static inline unsigned int sysfs_type(struct sysfs_dirent *sd)
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 #define sysfs_dirent_init_lockdep(sd)				\
 do {								\
-	static struct lock_class_key __key;			\
+	struct attribute *attr = sd->s_attr.attr;		\
+	struct lock_class_key *key = attr->key;			\
+	if (!key)						\
+		key = &attr->skey;				\
 								\
-	lockdep_init_map(&sd->dep_map, "s_active", &__key, 0);	\
+	lockdep_init_map(&sd->dep_map, "s_active", key, 0);	\
 } while(0)
 #else
 #define sysfs_dirent_init_lockdep(sd) do {} while(0)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index cfa8308..c3048de 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -15,6 +15,7 @@
 #include <linux/compiler.h>
 #include <linux/errno.h>
 #include <linux/list.h>
+#include <linux/lockdep.h>
 #include <asm/atomic.h>
 
 struct kobject;
@@ -29,8 +30,23 @@ struct attribute {
 	const char		*name;
 	struct module		*owner;
 	mode_t			mode;
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	struct lock_class_key	*key;
+	struct lock_class_key	skey;
+#endif
 };
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+#define sysfs_attr_init(attr)				\
+do {							\
+	static struct lock_class_key __key;		\
+							\
+	(attr)->key = &__key;				\
+} while(0)
+#else
+#define sysfs_attr_init(attr) do {} while(0)
+#endif
+
 struct attribute_group {
 	const char		*name;
 	mode_t			(*is_visible)(struct kobject *,
@@ -74,6 +90,8 @@ struct bin_attribute {
 		    struct vm_area_struct *vma);
 };
 
+#define sysfs_bin_attr_init(bin_attr) sysfs_attr_init(&bin_attr->attr)
+
 struct sysfs_ops {
 	ssize_t	(*show)(struct kobject *, struct attribute *,char *);
 	ssize_t	(*store)(struct kobject *,struct attribute *,const char *, size_t);
-- 
1.6.5.2.143.g8cc62

[PATCH 4/4] sysfs: Use sysfs_attr_init and sysfs_bin_attr_init on dynamic attributes

[Eric W. Biederman]

These are the non-static sysfs attributes that exist on
my test machine.  Fix them to use sysfs_attr_init or
sysfs_bin_attr_init as appropriate.   It simply requires
making a sysfs attribute present to see this.  So this
is a little bit tedious but otherwise not too bad.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c |    1 +
 drivers/acpi/system.c            |    2 ++
 drivers/pci/pci-sysfs.c          |    5 +++++
 kernel/params.c                  |    1 +
 4 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index a8aacd4..28cba46 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -2044,6 +2044,7 @@ static __init void mce_init_banks(void)
 		struct mce_bank *b = &mce_banks[i];
 		struct sysdev_attribute *a = &b->attr;
 
+		sysfs_attr_init(&a->attr);
 		a->attr.name	= b->attrname;
 		snprintf(b->attrname, ATTR_LEN, "bank%d", i);
 
diff --git a/drivers/acpi/system.c b/drivers/acpi/system.c
index d112829..a50fd08 100644
--- a/drivers/acpi/system.c
+++ b/drivers/acpi/system.c
@@ -101,6 +101,7 @@ static void acpi_table_attr_init(struct acpi_table_attr *table_attr,
 	struct acpi_table_header *header = NULL;
 	struct acpi_table_attr *attr = NULL;
 
+	sysfs_attr_init(&table_attr->attr.attr);
 	if (table_header->signature[0] != '\0')
 		memcpy(table_attr->name, table_header->signature,
 			ACPI_NAME_SIZE);
@@ -475,6 +476,7 @@ void acpi_irq_stats_init(void)
 			goto fail;
 		strncpy(name, buffer, strlen(buffer) + 1);
 
+		sysfs_attr_init(&counter_attrs[i].attr);
 		counter_attrs[i].attr.name = name;
 		counter_attrs[i].attr.mode = 0644;
 		counter_attrs[i].show = counter_show;
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index c5df94e..6fbcbea 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -640,6 +640,7 @@ void pci_create_legacy_files(struct pci_bus *b)
 	if (!b->legacy_io)
 		goto kzalloc_err;
 
+	sysfs_bin_attr_init(&b->legacy_io);
 	b->legacy_io->attr.name = "legacy_io";
 	b->legacy_io->size = 0xffff;
 	b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
@@ -652,6 +653,7 @@ void pci_create_legacy_files(struct pci_bus *b)
 		goto legacy_io_err;
 
 	/* Allocated above after the legacy_io struct */
+	sysfs_bin_attr_init(&b->legacy_mem);
 	b->legacy_mem = b->legacy_io + 1;
 	b->legacy_mem->attr.name = "legacy_mem";
 	b->legacy_mem->size = 1024*1024;
@@ -798,6 +800,7 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
 	if (res_attr) {
 		char *res_attr_name = (char *)(res_attr + 1);
 
+		sysfs_bin_attr_init(res_attr);
 		if (write_combine) {
 			pdev->res_attr_wc[num] = res_attr;
 			sprintf(res_attr_name, "resource%d_wc", num);
@@ -970,6 +973,7 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
 		if (!attr)
 			return -ENOMEM;
 
+		sysfs_bin_attr_init(attr);
 		attr->size = dev->vpd->len;
 		attr->attr.name = "vpd";
 		attr->attr.mode = S_IRUSR | S_IWUSR;
@@ -1036,6 +1040,7 @@ int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
 			retval = -ENOMEM;
 			goto err_resource_files;
 		}
+		sysfs_bin_attr_init(attr);
 		attr->size = rom_size;
 		attr->attr.name = "rom";
 		attr->attr.mode = S_IRUSR;
diff --git a/kernel/params.c b/kernel/params.c
index cf1b691..5bf6deb 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -517,6 +517,7 @@ static __modinit int add_sysfs_param(struct module_kobject *mk,
 	new->grp.attrs = attrs;
 
 	/* Tack new one on the end. */
+	sysfs_attr_init(&new->attrs[num].mattr.attr);
 	new->attrs[num].param = kp;
 	new->attrs[num].mattr.show = param_attr_show;
 	new->attrs[num].mattr.store = param_attr_store;
-- 
1.6.5.2.143.g8cc62

Re: [PATCH 4/4] sysfs: Use sysfs_attr_init and sysfs_bin_attr_init on dynamic attributes

[Greg KH]

On Thu, Feb 11, 2010 at 03:23:05PM -0800, Eric W. Biederman wrote:
> 
> These are the non-static sysfs attributes that exist on
> my test machine.  Fix them to use sysfs_attr_init or
> sysfs_bin_attr_init as appropriate.   It simply requires
> making a sysfs attribute present to see this.  So this
> is a little bit tedious but otherwise not too bad.

Can we document this in the sysfs_attr_init kerneldoc documentation that
you add for this new function?

thanks,

greg k-h

[PATCH] sysfs: Document sysfs_attr_init and sysfs_bin_attr_init

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Thu, Feb 11, 2010 at 03:23:05PM -0800, Eric W. Biederman wrote:
>> 
>> These are the non-static sysfs attributes that exist on
>> my test machine.  Fix them to use sysfs_attr_init or
>> sysfs_bin_attr_init as appropriate.   It simply requires
>> making a sysfs attribute present to see this.  So this
>> is a little bit tedious but otherwise not too bad.
>
> Can we document this in the sysfs_attr_init kerneldoc documentation that
> you add for this new function?

Here you go.

>From 646c76c0b20b30923a652bbf8cefec5e335359a9 Mon Sep 17 00:00:00 2001
From: Eric W. Biederman <ebiederm@xmission.com>
Date: Fri, 12 Feb 2010 04:35:32 -0800
Subject: [PATCH] sysfs: Document sysfs_attr_init and sysfs_bin_attr_init

I have added a new requirement to the external sysfs interface
that dynamically allocated sysfs attributes must call sysfs_attr_init
if lockdep is enabled.  For the time being callying sysfs_attr_init
is only mandatory if lockdep is enabled, so we can live with a few
unconverted instances until we find them all.  As this is part of
the public interface of sysfs it is a good idea to document these
pseudo functions so someone inspeciting the code can find out
what has happened.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/sysfs.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index c3048de..2b24ae5 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -36,6 +36,16 @@ struct attribute {
 #endif
 };
 
+/**
+ *	sysfs_attr_init - initialize a dynamically allocated sysfs attribute
+ *	@attr: struct attribute to initialize
+ *
+ *	Initialize a dynamically allocated struct attribute so we can
+ *	make lockdep happy.  This is a new requirement for attributes
+ *	and initially this is only needed when lockdep is enabled.
+ *	Lockdep gives a nice error when your attribute is added to
+ *	sysfs if you don't have this.
+ */
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 #define sysfs_attr_init(attr)				\
 do {							\
@@ -90,6 +100,16 @@ struct bin_attribute {
 		    struct vm_area_struct *vma);
 };
 
+/**
+ *	sysfs_bin_attr_init - initialize a dynamically allocated bin_attribute
+ *	@attr: struct bin_attribute to initialize
+ *
+ *	Initialize a dynamically allocated struct bin_attribute so we
+ *	can make lockdep happy.  This is a new requirement for
+ *	attributes and initially this is only needed when lockdep is
+ *	enabled.  Lockdep gives a nice error when your attribute is
+ *	added to sysfs if you don't have this.
+ */
 #define sysfs_bin_attr_init(bin_attr) sysfs_attr_init(&bin_attr->attr)
 
 struct sysfs_ops {
-- 
1.6.5.2.143.g8cc62

[PATCH] sysfs: Use sysfs_attr_init and sysfs_bin_attr_init on module dynamic attributes

[Eric W. Biederman]

A little more whack-a-mole annotating the dynamic sysfs attributes.  I
had everything built into my earlier test kernel, and so I missed
these.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 kernel/module.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index e96b8ed..e7e94f7 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1080,6 +1080,7 @@ static void add_sect_attrs(struct module *mod, unsigned int nsect,
 		if (sattr->name == NULL)
 			goto out;
 		sect_attrs->nsections++;
+		sysfs_attr_init(&sattr->mattr.attr);
 		sattr->mattr.show = module_sect_show;
 		sattr->mattr.store = NULL;
 		sattr->mattr.attr.name = sattr->name;
@@ -1175,6 +1176,7 @@ static void add_notes_attrs(struct module *mod, unsigned int nsect,
 		if (!(sechdrs[i].sh_flags & SHF_ALLOC))
 			continue;
 		if (sechdrs[i].sh_type == SHT_NOTE) {
+			sysfs_bin_attr_init(nattr);
 			nattr->attr.name = mod->sect_attrs->attrs[loaded].name;
 			nattr->attr.mode = S_IRUGO;
 			nattr->size = sechdrs[i].sh_size;
@@ -1247,6 +1249,7 @@ int module_add_modinfo_attrs(struct module *mod)
 		if (!attr->test ||
 		    (attr->test && attr->test(mod))) {
 			memcpy(temp_attr, attr, sizeof(*temp_attr));
+			sysfs_attr_init(&temp_attr->attr);
 			error = sysfs_create_file(&mod->mkobj.kobj,&temp_attr->attr);
 			++temp_attr;
 		}
-- 
1.6.5.2.143.g8cc62

Re: [PATCH 4/4] sysfs: Use sysfs_attr_init and sysfs_bin_attr_init on dynamic attributes

[Américo Wang]

On Thu, Feb 11, 2010 at 03:23:05PM -0800, Eric W. Biederman wrote:
>
>These are the non-static sysfs attributes that exist on
>my test machine.  Fix them to use sysfs_attr_init or
>sysfs_bin_attr_init as appropriate.   It simply requires
>making a sysfs attribute present to see this.  So this
>is a little bit tedious but otherwise not too bad.
>
>Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


Based on previous patches,

Acked-by: WANG Cong <xiyou.wangcong@gmail.com>

But it seems there are still some cases missed, e.g. the one
reported by John. We can fix them when they appear.

Thanks!

>---
> arch/x86/kernel/cpu/mcheck/mce.c |    1 +
> drivers/acpi/system.c            |    2 ++
> drivers/pci/pci-sysfs.c          |    5 +++++
> kernel/params.c                  |    1 +
> 4 files changed, 9 insertions(+), 0 deletions(-)
>
>diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
>index a8aacd4..28cba46 100644
>--- a/arch/x86/kernel/cpu/mcheck/mce.c
>+++ b/arch/x86/kernel/cpu/mcheck/mce.c
>@@ -2044,6 +2044,7 @@ static __init void mce_init_banks(void)
> 		struct mce_bank *b = &mce_banks[i];
> 		struct sysdev_attribute *a = &b->attr;
> 
>+		sysfs_attr_init(&a->attr);
> 		a->attr.name	= b->attrname;
> 		snprintf(b->attrname, ATTR_LEN, "bank%d", i);
> 
>diff --git a/drivers/acpi/system.c b/drivers/acpi/system.c
>index d112829..a50fd08 100644
>--- a/drivers/acpi/system.c
>+++ b/drivers/acpi/system.c
>@@ -101,6 +101,7 @@ static void acpi_table_attr_init(struct acpi_table_attr *table_attr,
> 	struct acpi_table_header *header = NULL;
> 	struct acpi_table_attr *attr = NULL;
> 
>+	sysfs_attr_init(&table_attr->attr.attr);
> 	if (table_header->signature[0] != '\0')
> 		memcpy(table_attr->name, table_header->signature,
> 			ACPI_NAME_SIZE);
>@@ -475,6 +476,7 @@ void acpi_irq_stats_init(void)
> 			goto fail;
> 		strncpy(name, buffer, strlen(buffer) + 1);
> 
>+		sysfs_attr_init(&counter_attrs[i].attr);
> 		counter_attrs[i].attr.name = name;
> 		counter_attrs[i].attr.mode = 0644;
> 		counter_attrs[i].show = counter_show;
>diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>index c5df94e..6fbcbea 100644
>--- a/drivers/pci/pci-sysfs.c
>+++ b/drivers/pci/pci-sysfs.c
>@@ -640,6 +640,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> 	if (!b->legacy_io)
> 		goto kzalloc_err;
> 
>+	sysfs_bin_attr_init(&b->legacy_io);
> 	b->legacy_io->attr.name = "legacy_io";
> 	b->legacy_io->size = 0xffff;
> 	b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
>@@ -652,6 +653,7 @@ void pci_create_legacy_files(struct pci_bus *b)
> 		goto legacy_io_err;
> 
> 	/* Allocated above after the legacy_io struct */
>+	sysfs_bin_attr_init(&b->legacy_mem);
> 	b->legacy_mem = b->legacy_io + 1;
> 	b->legacy_mem->attr.name = "legacy_mem";
> 	b->legacy_mem->size = 1024*1024;
>@@ -798,6 +800,7 @@ static int pci_create_attr(struct pci_dev *pdev, int num, int write_combine)
> 	if (res_attr) {
> 		char *res_attr_name = (char *)(res_attr + 1);
> 
>+		sysfs_bin_attr_init(res_attr);
> 		if (write_combine) {
> 			pdev->res_attr_wc[num] = res_attr;
> 			sprintf(res_attr_name, "resource%d_wc", num);
>@@ -970,6 +973,7 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
> 		if (!attr)
> 			return -ENOMEM;
> 
>+		sysfs_bin_attr_init(attr);
> 		attr->size = dev->vpd->len;
> 		attr->attr.name = "vpd";
> 		attr->attr.mode = S_IRUSR | S_IWUSR;
>@@ -1036,6 +1040,7 @@ int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
> 			retval = -ENOMEM;
> 			goto err_resource_files;
> 		}
>+		sysfs_bin_attr_init(attr);
> 		attr->size = rom_size;
> 		attr->attr.name = "rom";
> 		attr->attr.mode = S_IRUSR;
>diff --git a/kernel/params.c b/kernel/params.c
>index cf1b691..5bf6deb 100644
>--- a/kernel/params.c
>+++ b/kernel/params.c
>@@ -517,6 +517,7 @@ static __modinit int add_sysfs_param(struct module_kobject *mk,
> 	new->grp.attrs = attrs;
> 
> 	/* Tack new one on the end. */
>+	sysfs_attr_init(&new->attrs[num].mattr.attr);
> 	new->attrs[num].param = kp;
> 	new->attrs[num].mattr.show = param_attr_show;
> 	new->attrs[num].mattr.store = param_attr_store;
>-- 
>1.6.5.2.143.g8cc62
>

-- 
Live like a child, think like the god.
 

Re: [PATCH 4/4] sysfs: Use sysfs_attr_init and sysfs_bin_attr_init on dynamic attributes

[Eric W. Biederman]

Américo Wang <xiyou.wangcong@gmail.com> writes:

> On Thu, Feb 11, 2010 at 03:23:05PM -0800, Eric W. Biederman wrote:
>>
>>These are the non-static sysfs attributes that exist on
>>my test machine.  Fix them to use sysfs_attr_init or
>>sysfs_bin_attr_init as appropriate.   It simply requires
>>making a sysfs attribute present to see this.  So this
>>is a little bit tedious but otherwise not too bad.
>>
>>Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>
>
> Based on previous patches,
>
> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
>
> But it seems there are still some cases missed, e.g. the one
> reported by John. We can fix them when they appear.

Yes.  There is at least the module case that I sent a follow up
patch for.

Whacking these things when the appear isn't my favorite policy
but at least it is easy.

Eric

Re: [PATCH 3/4] sysfs: Use one lockdep class per sysfs attribute.

[Américo Wang]

On Thu, Feb 11, 2010 at 03:21:53PM -0800, Eric W. Biederman wrote:
>
>Acknowledge that the logical sysfs rwsem has one instance per
>sysfs attribute with different locking depencencies for different
>attributes.
>
>There is a sysfs idiom where writing to one sysfs file causes the
>addition or removal of other sysfs files.   Lumping all of the
>sysfs attributes together in one lock class causes lockdep to
>generate lots of false positives.
>
>This introduces the requirement that non-static sysfs attributes
>need to be initialized with sysfs_attr_init or sysfs_bin_attr_init.
>Strictly speaking this requirement only exists when lockdep is
>enabled, and when lockdep is enabled we get a bit fat warning
>if this requirement is not met.

Since there are fewer non-static attributes, this will not
waste much space.

>
>Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


Acked-by: WANG Cong <xiyou.wangcong@gmail.com>

>---
> fs/sysfs/sysfs.h      |    7 +++++--
> include/linux/sysfs.h |   18 ++++++++++++++++++
> 2 files changed, 23 insertions(+), 2 deletions(-)
>
>diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
>index 7db6884..37e0e08 100644
>--- a/fs/sysfs/sysfs.h
>+++ b/fs/sysfs/sysfs.h
>@@ -92,9 +92,12 @@ static inline unsigned int sysfs_type(struct sysfs_dirent *sd)
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> #define sysfs_dirent_init_lockdep(sd)				\
> do {								\
>-	static struct lock_class_key __key;			\
>+	struct attribute *attr = sd->s_attr.attr;		\
>+	struct lock_class_key *key = attr->key;			\
>+	if (!key)						\
>+		key = &attr->skey;				\
> 								\
>-	lockdep_init_map(&sd->dep_map, "s_active", &__key, 0);	\
>+	lockdep_init_map(&sd->dep_map, "s_active", key, 0);	\
> } while(0)
> #else
> #define sysfs_dirent_init_lockdep(sd) do {} while(0)
>diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>index cfa8308..c3048de 100644
>--- a/include/linux/sysfs.h
>+++ b/include/linux/sysfs.h
>@@ -15,6 +15,7 @@
> #include <linux/compiler.h>
> #include <linux/errno.h>
> #include <linux/list.h>
>+#include <linux/lockdep.h>
> #include <asm/atomic.h>
> 
> struct kobject;
>@@ -29,8 +30,23 @@ struct attribute {
> 	const char		*name;
> 	struct module		*owner;
> 	mode_t			mode;
>+#ifdef CONFIG_DEBUG_LOCK_ALLOC
>+	struct lock_class_key	*key;
>+	struct lock_class_key	skey;
>+#endif
> };
> 
>+#ifdef CONFIG_DEBUG_LOCK_ALLOC
>+#define sysfs_attr_init(attr)				\
>+do {							\
>+	static struct lock_class_key __key;		\
>+							\
>+	(attr)->key = &__key;				\
>+} while(0)
>+#else
>+#define sysfs_attr_init(attr) do {} while(0)
>+#endif
>+
> struct attribute_group {
> 	const char		*name;
> 	mode_t			(*is_visible)(struct kobject *,
>@@ -74,6 +90,8 @@ struct bin_attribute {
> 		    struct vm_area_struct *vma);
> };
> 
>+#define sysfs_bin_attr_init(bin_attr) sysfs_attr_init(&bin_attr->attr)
>+
> struct sysfs_ops {
> 	ssize_t	(*show)(struct kobject *, struct attribute *,char *);
> 	ssize_t	(*store)(struct kobject *,struct attribute *,const char *, size_t);
>-- 
>1.6.5.2.143.g8cc62
>

-- 
Live like a child, think like the god.
 

Re: [PATCH 2/4] sysfs: Only take active references on attributes.

[Américo Wang]

On Thu, Feb 11, 2010 at 03:20:00PM -0800, Eric W. Biederman wrote:
>
>If we exclude directories and symlinks from the set of sysfs
>dirents where we need active references we are left with
>sysfs attributes (binary or not).
>
>- Tweak sysfs_deactivate to only do something on attributes
>- Move lockdep initialization into sysfs_file_add_mode to
>  limit it to just attributes.

Why?

If I read your patch correctly, s_active will be useless
for non-attributes sysfs entries? For sysfs dir, maybe,
since it can only be removed by sysfs_remove_dir(),
but not sure about sysfs symlinks...


>
>Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>---
> fs/sysfs/dir.c   |    5 ++++-
> fs/sysfs/file.c  |    1 +
> fs/sysfs/sysfs.h |    1 +
> 3 files changed, 6 insertions(+), 1 deletions(-)
>
>diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
>index 1bdc42f..481fdec 100644
>--- a/fs/sysfs/dir.c
>+++ b/fs/sysfs/dir.c
>@@ -156,6 +156,10 @@ static void sysfs_deactivate(struct sysfs_dirent *sd)
> 	int v;
> 
> 	BUG_ON(sd->s_sibling || !(sd->s_flags & SYSFS_FLAG_REMOVED));
>+
>+	if (!(sysfs_type(sd) & SYSFS_ACTIVE_REF))
>+		return;
>+
> 	sd->s_sibling = (void *)&wait;
> 
> 	rwsem_acquire(&sd->dep_map, 0, 0, _RET_IP_);
>@@ -315,7 +319,6 @@ struct sysfs_dirent *sysfs_new_dirent(const char *name, umode_t mode, int type)
> 
> 	atomic_set(&sd->s_count, 1);
> 	atomic_set(&sd->s_active, 0);
>-	sysfs_dirent_init_lockdep(sd);
> 
> 	sd->s_name = name;
> 	sd->s_mode = mode;
>diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
>index 8d6bd65..8eaae5d 100644
>--- a/fs/sysfs/file.c
>+++ b/fs/sysfs/file.c
>@@ -509,6 +509,7 @@ int sysfs_add_file_mode(struct sysfs_dirent *dir_sd,
> 	if (!sd)
> 		return -ENOMEM;
> 	sd->s_attr.attr = (void *)attr;
>+	sysfs_dirent_init_lockdep(sd);
> 
> 	sysfs_addrm_start(&acxt, dir_sd);
> 	rc = sysfs_add_one(&acxt, sd);
>diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
>index bb7723c..7db6884 100644
>--- a/fs/sysfs/sysfs.h
>+++ b/fs/sysfs/sysfs.h
>@@ -79,6 +79,7 @@ struct sysfs_dirent {
> #define SYSFS_KOBJ_BIN_ATTR		0x0004
> #define SYSFS_KOBJ_LINK			0x0008
> #define SYSFS_COPY_NAME			(SYSFS_DIR | SYSFS_KOBJ_LINK)
>+#define SYSFS_ACTIVE_REF		(SYSFS_KOBJ_ATTR | SYSFS_KOBJ_BIN_ATTR)
> 
> #define SYSFS_FLAG_MASK			~SYSFS_TYPE_MASK
> #define SYSFS_FLAG_REMOVED		0x0200
>-- 
>1.6.5.2.143.g8cc62
>

-- 
Live like a child, think like the god.
 

Re: [PATCH 2/4] sysfs: Only take active references on attributes.

[Américo Wang]

On Mon, Feb 15, 2010 at 03:27:45PM +0800, Américo Wang wrote:
>On Thu, Feb 11, 2010 at 03:20:00PM -0800, Eric W. Biederman wrote:
>>
>>If we exclude directories and symlinks from the set of sysfs
>>dirents where we need active references we are left with
>>sysfs attributes (binary or not).
>>
>>- Tweak sysfs_deactivate to only do something on attributes
>>- Move lockdep initialization into sysfs_file_add_mode to
>>  limit it to just attributes.
>
>Why?
>
>If I read your patch correctly, s_active will be useless
>for non-attributes sysfs entries? For sysfs dir, maybe,
>since it can only be removed by sysfs_remove_dir(),
>but not sure about sysfs symlinks...
>

For sysfs dir's, opening it will not get s_active,
since it doesn't have .open member. But it does
put s_active when removing it. This seems buggy?

For symlinks, it seems sysfs totally ignores s_active,
thus is safe for this patch.

Re: [PATCH 2/4] sysfs: Only take active references on attributes.

[Américo Wang]

On Mon, Feb 15, 2010 at 04:15:27PM +0800, Américo Wang wrote:
>On Mon, Feb 15, 2010 at 03:27:45PM +0800, Américo Wang wrote:
>>On Thu, Feb 11, 2010 at 03:20:00PM -0800, Eric W. Biederman wrote:
>>>
>>>If we exclude directories and symlinks from the set of sysfs
>>>dirents where we need active references we are left with
>>>sysfs attributes (binary or not).
>>>
>>>- Tweak sysfs_deactivate to only do something on attributes
>>>- Move lockdep initialization into sysfs_file_add_mode to
>>>  limit it to just attributes.
>>
>>Why?
>>
>>If I read your patch correctly, s_active will be useless
>>for non-attributes sysfs entries? For sysfs dir, maybe,
>>since it can only be removed by sysfs_remove_dir(),
>>but not sure about sysfs symlinks...
>>
>
>For sysfs dir's, opening it will not get s_active,
>since it doesn't have .open member. But it does
>put s_active when removing it. This seems buggy?

Oh, definitely no...

s_active of sysfs dir is only for kernel space,
after your previous patch.

>
>For symlinks, it seems sysfs totally ignores s_active,
>thus is safe for this patch.
>

Thus, your patch seems correct.

I think we could merge them for more testing.

Acked-by: WANG Cong <xiyou.wangcong@gmail.com>

Thanks!

Re: [PATCH 2/4] sysfs: Only take active references on attributes.

[Eric W. Biederman]

Américo Wang <xiyou.wangcong@gmail.com> writes:

> On Mon, Feb 15, 2010 at 03:27:45PM +0800, Américo Wang wrote:
>>On Thu, Feb 11, 2010 at 03:20:00PM -0800, Eric W. Biederman wrote:
>>>
>>>If we exclude directories and symlinks from the set of sysfs
>>>dirents where we need active references we are left with
>>>sysfs attributes (binary or not).
>>>
>>>- Tweak sysfs_deactivate to only do something on attributes
>>>- Move lockdep initialization into sysfs_file_add_mode to
>>>  limit it to just attributes.
>>
>>Why?
>>
>>If I read your patch correctly, s_active will be useless
>>for non-attributes sysfs entries? For sysfs dir, maybe,
>>since it can only be removed by sysfs_remove_dir(),
>>but not sure about sysfs symlinks...

Yes. s_active is effectively useless for non-attribute sysfs entries.

> For sysfs dir's, opening it will not get s_active,
> since it doesn't have .open member. But it does
> put s_active when removing it. This seems buggy?

s_active needs to be taken over the lifetime of every method.  For
directories a file descriptor will pin the dirent which does have a
reference to the sysfs dirent, and that ensures the sysfs entry does
not disappear.

There is one common path for removing sysfs dirents and they all call
sysfs_deactivate.  The job of sysfs_deactivate is to wait until there
are no methods actively running so their dirents can have any
non-generic state cleaned up.

Neither symlinks nor directories call sysfs_get/put_active, thus
sysfs_deactivate is effectively a noop for those dirents before my
patch.  After my patch sysfs_deactivate is also a noop from a lockdep
perspective.

Eric

Re: [PATCH 1/4] sysfs: Remove sysfs_get/put_active_two

[Américo Wang]

On Thu, Feb 11, 2010 at 03:14:47PM -0800, Eric W. Biederman wrote:
>
>It turns out that holding an active reference on a directory is
>pointless.  The purpose of the active references are to allows us to
>block when removing sysfs entries that have custom methods so we don't
>remove modules while running modular code and to keep those custom
>methods from accessing data structures after the files have been
>removed.  Further sysfs_remove_dir remove all elements in the
>directory before removing the directory itself, so there is no chance
>we will remove a directory with active children.
>

Oh, I see... You are trying to change the meaning of s_active,
thus killing the lockdep warning that it triggers.

Hmm, I agree with your analysis above. As long as the order is that,
this change should be safe, since sysfs_remove_dir() should be
the only way to remove an sysfs dir.

It is a really nice patch!

Acked-by: WANG Cong <xiyou.wangcong@gmail.com>

Thanks!

>Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>---
> fs/sysfs/bin.c   |   50 +++++++++++++++++++++++++-------------------------
> fs/sysfs/dir.c   |   43 ++-----------------------------------------
> fs/sysfs/file.c  |   18 +++++++++---------
> fs/sysfs/sysfs.h |    4 ++--
> 4 files changed, 38 insertions(+), 77 deletions(-)
>
>diff --git a/fs/sysfs/bin.c b/fs/sysfs/bin.c
>index a0a500a..e9d2935 100644
>--- a/fs/sysfs/bin.c
>+++ b/fs/sysfs/bin.c
>@@ -54,14 +54,14 @@ fill_read(struct dentry *dentry, char *buffer, loff_t off, size_t count)
> 	int rc;
> 
> 	/* need attr_sd for attr, its parent for kobj */
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return -ENODEV;
> 
> 	rc = -EIO;
> 	if (attr->read)
> 		rc = attr->read(kobj, attr, buffer, off, count);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 
> 	return rc;
> }
>@@ -125,14 +125,14 @@ flush_write(struct dentry *dentry, char *buffer, loff_t offset, size_t count)
> 	int rc;
> 
> 	/* need attr_sd for attr, its parent for kobj */
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return -ENODEV;
> 
> 	rc = -EIO;
> 	if (attr->write)
> 		rc = attr->write(kobj, attr, buffer, offset, count);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 
> 	return rc;
> }
>@@ -184,12 +184,12 @@ static void bin_vma_open(struct vm_area_struct *vma)
> 	if (!bb->vm_ops || !bb->vm_ops->open)
> 		return;
> 
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return;
> 
> 	bb->vm_ops->open(vma);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> }
> 
> static void bin_vma_close(struct vm_area_struct *vma)
>@@ -201,12 +201,12 @@ static void bin_vma_close(struct vm_area_struct *vma)
> 	if (!bb->vm_ops || !bb->vm_ops->close)
> 		return;
> 
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return;
> 
> 	bb->vm_ops->close(vma);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> }
> 
> static int bin_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>@@ -219,12 +219,12 @@ static int bin_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> 	if (!bb->vm_ops || !bb->vm_ops->fault)
> 		return VM_FAULT_SIGBUS;
> 
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return VM_FAULT_SIGBUS;
> 
> 	ret = bb->vm_ops->fault(vma, vmf);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 	return ret;
> }
> 
>@@ -241,12 +241,12 @@ static int bin_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
> 	if (!bb->vm_ops->page_mkwrite)
> 		return 0;
> 
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return VM_FAULT_SIGBUS;
> 
> 	ret = bb->vm_ops->page_mkwrite(vma, vmf);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 	return ret;
> }
> 
>@@ -261,12 +261,12 @@ static int bin_access(struct vm_area_struct *vma, unsigned long addr,
> 	if (!bb->vm_ops || !bb->vm_ops->access)
> 		return -EINVAL;
> 
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return -EINVAL;
> 
> 	ret = bb->vm_ops->access(vma, addr, buf, len, write);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 	return ret;
> }
> 
>@@ -281,12 +281,12 @@ static int bin_set_policy(struct vm_area_struct *vma, struct mempolicy *new)
> 	if (!bb->vm_ops || !bb->vm_ops->set_policy)
> 		return 0;
> 
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return -EINVAL;
> 
> 	ret = bb->vm_ops->set_policy(vma, new);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 	return ret;
> }
> 
>@@ -301,12 +301,12 @@ static struct mempolicy *bin_get_policy(struct vm_area_struct *vma,
> 	if (!bb->vm_ops || !bb->vm_ops->get_policy)
> 		return vma->vm_policy;
> 
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return vma->vm_policy;
> 
> 	pol = bb->vm_ops->get_policy(vma, addr);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 	return pol;
> }
> 
>@@ -321,12 +321,12 @@ static int bin_migrate(struct vm_area_struct *vma, const nodemask_t *from,
> 	if (!bb->vm_ops || !bb->vm_ops->migrate)
> 		return 0;
> 
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return 0;
> 
> 	ret = bb->vm_ops->migrate(vma, from, to, flags);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 	return ret;
> }
> #endif
>@@ -356,7 +356,7 @@ static int mmap(struct file *file, struct vm_area_struct *vma)
> 
> 	/* need attr_sd for attr, its parent for kobj */
> 	rc = -ENODEV;
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		goto out_unlock;
> 
> 	rc = -EINVAL;
>@@ -384,7 +384,7 @@ static int mmap(struct file *file, struct vm_area_struct *vma)
> 	bb->vm_ops = vma->vm_ops;
> 	vma->vm_ops = &bin_vm_ops;
> out_put:
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> out_unlock:
> 	mutex_unlock(&bb->mutex);
> 
>@@ -399,7 +399,7 @@ static int open(struct inode * inode, struct file * file)
> 	int error;
> 
> 	/* binary file operations requires both @sd and its parent */
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return -ENODEV;
> 
> 	error = -EACCES;
>@@ -426,11 +426,11 @@ static int open(struct inode * inode, struct file * file)
> 	mutex_unlock(&sysfs_bin_lock);
> 
> 	/* open succeeded, put active references */
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 	return 0;
> 
>  err_out:
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 	kfree(bb);
> 	return error;
> }
>diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
>index 5c4703d..1bdc42f 100644
>--- a/fs/sysfs/dir.c
>+++ b/fs/sysfs/dir.c
>@@ -93,7 +93,7 @@ static void sysfs_unlink_sibling(struct sysfs_dirent *sd)
>  *	RETURNS:
>  *	Pointer to @sd on success, NULL on failure.
>  */
>-static struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
>+struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
> {
> 	if (unlikely(!sd))
> 		return NULL;
>@@ -124,7 +124,7 @@ static struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
>  *	Put an active reference to @sd.  This function is noop if @sd
>  *	is NULL.
>  */
>-static void sysfs_put_active(struct sysfs_dirent *sd)
>+void sysfs_put_active(struct sysfs_dirent *sd)
> {
> 	struct completion *cmpl;
> 	int v;
>@@ -145,45 +145,6 @@ static void sysfs_put_active(struct sysfs_dirent *sd)
> }
> 
> /**
>- *	sysfs_get_active_two - get active references to sysfs_dirent and parent
>- *	@sd: sysfs_dirent of interest
>- *
>- *	Get active reference to @sd and its parent.  Parent's active
>- *	reference is grabbed first.  This function is noop if @sd is
>- *	NULL.
>- *
>- *	RETURNS:
>- *	Pointer to @sd on success, NULL on failure.
>- */
>-struct sysfs_dirent *sysfs_get_active_two(struct sysfs_dirent *sd)
>-{
>-	if (sd) {
>-		if (sd->s_parent && unlikely(!sysfs_get_active(sd->s_parent)))
>-			return NULL;
>-		if (unlikely(!sysfs_get_active(sd))) {
>-			sysfs_put_active(sd->s_parent);
>-			return NULL;
>-		}
>-	}
>-	return sd;
>-}
>-
>-/**
>- *	sysfs_put_active_two - put active references to sysfs_dirent and parent
>- *	@sd: sysfs_dirent of interest
>- *
>- *	Put active references to @sd and its parent.  This function is
>- *	noop if @sd is NULL.
>- */
>-void sysfs_put_active_two(struct sysfs_dirent *sd)
>-{
>-	if (sd) {
>-		sysfs_put_active(sd);
>-		sysfs_put_active(sd->s_parent);
>-	}
>-}
>-
>-/**
>  *	sysfs_deactivate - deactivate sysfs_dirent
>  *	@sd: sysfs_dirent to deactivate
>  *
>diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
>index dc30d9e..8d6bd65 100644
>--- a/fs/sysfs/file.c
>+++ b/fs/sysfs/file.c
>@@ -85,13 +85,13 @@ static int fill_read_buffer(struct dentry * dentry, struct sysfs_buffer * buffer
> 		return -ENOMEM;
> 
> 	/* need attr_sd for attr and ops, its parent for kobj */
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return -ENODEV;
> 
> 	buffer->event = atomic_read(&attr_sd->s_attr.open->event);
> 	count = ops->show(kobj, attr_sd->s_attr.attr, buffer->page);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 
> 	/*
> 	 * The code works fine with PAGE_SIZE return but it's likely to
>@@ -203,12 +203,12 @@ flush_write_buffer(struct dentry * dentry, struct sysfs_buffer * buffer, size_t
> 	int rc;
> 
> 	/* need attr_sd for attr and ops, its parent for kobj */
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return -ENODEV;
> 
> 	rc = ops->store(kobj, attr_sd->s_attr.attr, buffer->page, count);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 
> 	return rc;
> }
>@@ -344,7 +344,7 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
> 		memmove(last_sysfs_file, p, strlen(p) + 1);
> 
> 	/* need attr_sd for attr and ops, its parent for kobj */
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		return -ENODEV;
> 
> 	/* every kobject with an attribute needs a ktype assigned */
>@@ -393,13 +393,13 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
> 		goto err_free;
> 
> 	/* open succeeded, put active references */
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 	return 0;
> 
>  err_free:
> 	kfree(buffer);
>  err_out:
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 	return error;
> }
> 
>@@ -437,12 +437,12 @@ static unsigned int sysfs_poll(struct file *filp, poll_table *wait)
> 	struct sysfs_open_dirent *od = attr_sd->s_attr.open;
> 
> 	/* need parent for the kobj, grab both */
>-	if (!sysfs_get_active_two(attr_sd))
>+	if (!sysfs_get_active(attr_sd))
> 		goto trigger;
> 
> 	poll_wait(filp, &od->poll, wait);
> 
>-	sysfs_put_active_two(attr_sd);
>+	sysfs_put_active(attr_sd);
> 
> 	if (buffer->event != atomic_read(&od->event))
> 		goto trigger;
>diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
>index cdd9377..bb7723c 100644
>--- a/fs/sysfs/sysfs.h
>+++ b/fs/sysfs/sysfs.h
>@@ -124,8 +124,8 @@ extern const struct file_operations sysfs_dir_operations;
> extern const struct inode_operations sysfs_dir_inode_operations;
> 
> struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd);
>-struct sysfs_dirent *sysfs_get_active_two(struct sysfs_dirent *sd);
>-void sysfs_put_active_two(struct sysfs_dirent *sd);
>+struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd);
>+void sysfs_put_active(struct sysfs_dirent *sd);
> void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
> 		       struct sysfs_dirent *parent_sd);
> int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
>-- 
>1.6.5.2.143.g8cc62
>

-- 
Live like a child, think like the god.
 

[PATCH 1/4] sysfs: Remove sysfs_get/put_active_two

[Eric W. Biederman]

It turns out that holding an active reference on a directory is
pointless.  The purpose of the active references are to allows us to
block when removing sysfs entries that have custom methods so we don't
remove modules while running modular code and to keep those custom
methods from accessing data structures after the files have been
removed.  Further sysfs_remove_dir remove all elements in the
directory before removing the directory itself, so there is no chance
we will remove a directory with active children.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/sysfs/bin.c   |   50 +++++++++++++++++++++++++-------------------------
 fs/sysfs/dir.c   |   43 ++-----------------------------------------
 fs/sysfs/file.c  |   18 +++++++++---------
 fs/sysfs/sysfs.h |    4 ++--
 4 files changed, 38 insertions(+), 77 deletions(-)

diff --git a/fs/sysfs/bin.c b/fs/sysfs/bin.c
index a0a500a..e9d2935 100644
--- a/fs/sysfs/bin.c
+++ b/fs/sysfs/bin.c
@@ -54,14 +54,14 @@ fill_read(struct dentry *dentry, char *buffer, loff_t off, size_t count)
 	int rc;
 
 	/* need attr_sd for attr, its parent for kobj */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	rc = -EIO;
 	if (attr->read)
 		rc = attr->read(kobj, attr, buffer, off, count);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 
 	return rc;
 }
@@ -125,14 +125,14 @@ flush_write(struct dentry *dentry, char *buffer, loff_t offset, size_t count)
 	int rc;
 
 	/* need attr_sd for attr, its parent for kobj */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	rc = -EIO;
 	if (attr->write)
 		rc = attr->write(kobj, attr, buffer, offset, count);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 
 	return rc;
 }
@@ -184,12 +184,12 @@ static void bin_vma_open(struct vm_area_struct *vma)
 	if (!bb->vm_ops || !bb->vm_ops->open)
 		return;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return;
 
 	bb->vm_ops->open(vma);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 }
 
 static void bin_vma_close(struct vm_area_struct *vma)
@@ -201,12 +201,12 @@ static void bin_vma_close(struct vm_area_struct *vma)
 	if (!bb->vm_ops || !bb->vm_ops->close)
 		return;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return;
 
 	bb->vm_ops->close(vma);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 }
 
 static int bin_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
@@ -219,12 +219,12 @@ static int bin_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	if (!bb->vm_ops || !bb->vm_ops->fault)
 		return VM_FAULT_SIGBUS;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return VM_FAULT_SIGBUS;
 
 	ret = bb->vm_ops->fault(vma, vmf);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return ret;
 }
 
@@ -241,12 +241,12 @@ static int bin_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
 	if (!bb->vm_ops->page_mkwrite)
 		return 0;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return VM_FAULT_SIGBUS;
 
 	ret = bb->vm_ops->page_mkwrite(vma, vmf);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return ret;
 }
 
@@ -261,12 +261,12 @@ static int bin_access(struct vm_area_struct *vma, unsigned long addr,
 	if (!bb->vm_ops || !bb->vm_ops->access)
 		return -EINVAL;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -EINVAL;
 
 	ret = bb->vm_ops->access(vma, addr, buf, len, write);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return ret;
 }
 
@@ -281,12 +281,12 @@ static int bin_set_policy(struct vm_area_struct *vma, struct mempolicy *new)
 	if (!bb->vm_ops || !bb->vm_ops->set_policy)
 		return 0;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -EINVAL;
 
 	ret = bb->vm_ops->set_policy(vma, new);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return ret;
 }
 
@@ -301,12 +301,12 @@ static struct mempolicy *bin_get_policy(struct vm_area_struct *vma,
 	if (!bb->vm_ops || !bb->vm_ops->get_policy)
 		return vma->vm_policy;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return vma->vm_policy;
 
 	pol = bb->vm_ops->get_policy(vma, addr);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return pol;
 }
 
@@ -321,12 +321,12 @@ static int bin_migrate(struct vm_area_struct *vma, const nodemask_t *from,
 	if (!bb->vm_ops || !bb->vm_ops->migrate)
 		return 0;
 
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return 0;
 
 	ret = bb->vm_ops->migrate(vma, from, to, flags);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return ret;
 }
 #endif
@@ -356,7 +356,7 @@ static int mmap(struct file *file, struct vm_area_struct *vma)
 
 	/* need attr_sd for attr, its parent for kobj */
 	rc = -ENODEV;
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		goto out_unlock;
 
 	rc = -EINVAL;
@@ -384,7 +384,7 @@ static int mmap(struct file *file, struct vm_area_struct *vma)
 	bb->vm_ops = vma->vm_ops;
 	vma->vm_ops = &bin_vm_ops;
 out_put:
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 out_unlock:
 	mutex_unlock(&bb->mutex);
 
@@ -399,7 +399,7 @@ static int open(struct inode * inode, struct file * file)
 	int error;
 
 	/* binary file operations requires both @sd and its parent */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	error = -EACCES;
@@ -426,11 +426,11 @@ static int open(struct inode * inode, struct file * file)
 	mutex_unlock(&sysfs_bin_lock);
 
 	/* open succeeded, put active references */
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return 0;
 
  err_out:
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	kfree(bb);
 	return error;
 }
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index 5c4703d..1bdc42f 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -93,7 +93,7 @@ static void sysfs_unlink_sibling(struct sysfs_dirent *sd)
  *	RETURNS:
  *	Pointer to @sd on success, NULL on failure.
  */
-static struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
+struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
 {
 	if (unlikely(!sd))
 		return NULL;
@@ -124,7 +124,7 @@ static struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd)
  *	Put an active reference to @sd.  This function is noop if @sd
  *	is NULL.
  */
-static void sysfs_put_active(struct sysfs_dirent *sd)
+void sysfs_put_active(struct sysfs_dirent *sd)
 {
 	struct completion *cmpl;
 	int v;
@@ -145,45 +145,6 @@ static void sysfs_put_active(struct sysfs_dirent *sd)
 }
 
 /**
- *	sysfs_get_active_two - get active references to sysfs_dirent and parent
- *	@sd: sysfs_dirent of interest
- *
- *	Get active reference to @sd and its parent.  Parent's active
- *	reference is grabbed first.  This function is noop if @sd is
- *	NULL.
- *
- *	RETURNS:
- *	Pointer to @sd on success, NULL on failure.
- */
-struct sysfs_dirent *sysfs_get_active_two(struct sysfs_dirent *sd)
-{
-	if (sd) {
-		if (sd->s_parent && unlikely(!sysfs_get_active(sd->s_parent)))
-			return NULL;
-		if (unlikely(!sysfs_get_active(sd))) {
-			sysfs_put_active(sd->s_parent);
-			return NULL;
-		}
-	}
-	return sd;
-}
-
-/**
- *	sysfs_put_active_two - put active references to sysfs_dirent and parent
- *	@sd: sysfs_dirent of interest
- *
- *	Put active references to @sd and its parent.  This function is
- *	noop if @sd is NULL.
- */
-void sysfs_put_active_two(struct sysfs_dirent *sd)
-{
-	if (sd) {
-		sysfs_put_active(sd);
-		sysfs_put_active(sd->s_parent);
-	}
-}
-
-/**
  *	sysfs_deactivate - deactivate sysfs_dirent
  *	@sd: sysfs_dirent to deactivate
  *
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index dc30d9e..8d6bd65 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -85,13 +85,13 @@ static int fill_read_buffer(struct dentry * dentry, struct sysfs_buffer * buffer
 		return -ENOMEM;
 
 	/* need attr_sd for attr and ops, its parent for kobj */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	buffer->event = atomic_read(&attr_sd->s_attr.open->event);
 	count = ops->show(kobj, attr_sd->s_attr.attr, buffer->page);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 
 	/*
 	 * The code works fine with PAGE_SIZE return but it's likely to
@@ -203,12 +203,12 @@ flush_write_buffer(struct dentry * dentry, struct sysfs_buffer * buffer, size_t
 	int rc;
 
 	/* need attr_sd for attr and ops, its parent for kobj */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	rc = ops->store(kobj, attr_sd->s_attr.attr, buffer->page, count);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 
 	return rc;
 }
@@ -344,7 +344,7 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 		memmove(last_sysfs_file, p, strlen(p) + 1);
 
 	/* need attr_sd for attr and ops, its parent for kobj */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		return -ENODEV;
 
 	/* every kobject with an attribute needs a ktype assigned */
@@ -393,13 +393,13 @@ static int sysfs_open_file(struct inode *inode, struct file *file)
 		goto err_free;
 
 	/* open succeeded, put active references */
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return 0;
 
  err_free:
 	kfree(buffer);
  err_out:
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 	return error;
 }
 
@@ -437,12 +437,12 @@ static unsigned int sysfs_poll(struct file *filp, poll_table *wait)
 	struct sysfs_open_dirent *od = attr_sd->s_attr.open;
 
 	/* need parent for the kobj, grab both */
-	if (!sysfs_get_active_two(attr_sd))
+	if (!sysfs_get_active(attr_sd))
 		goto trigger;
 
 	poll_wait(filp, &od->poll, wait);
 
-	sysfs_put_active_two(attr_sd);
+	sysfs_put_active(attr_sd);
 
 	if (buffer->event != atomic_read(&od->event))
 		goto trigger;
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index cdd9377..bb7723c 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -124,8 +124,8 @@ extern const struct file_operations sysfs_dir_operations;
 extern const struct inode_operations sysfs_dir_inode_operations;
 
 struct dentry *sysfs_get_dentry(struct sysfs_dirent *sd);
-struct sysfs_dirent *sysfs_get_active_two(struct sysfs_dirent *sd);
-void sysfs_put_active_two(struct sysfs_dirent *sd);
+struct sysfs_dirent *sysfs_get_active(struct sysfs_dirent *sd);
+void sysfs_put_active(struct sysfs_dirent *sd);
 void sysfs_addrm_start(struct sysfs_addrm_cxt *acxt,
 		       struct sysfs_dirent *parent_sd);
 int __sysfs_add_one(struct sysfs_addrm_cxt *acxt, struct sysfs_dirent *sd);
-- 
1.6.5.2.143.g8cc62

[PATCH 0/4] Better sysfs lockdep

[Eric W. Biederman]

It turns out that it is pretty simple to rework the lockdep support
to give each sysfs attribute it's own lockdep class, which should
kill the annoying false positives we have been seeing lately.

The one caveat is that it requires dynamically allocated sysfs
attributes to be explicitly initialized.  Luckily it is brainless
trivial whack-a-mole and took maybe 15 minutes to find all of the
dynamically allocates sysfs attributes on my test machine, because
the errors show up when the attributes are registered with sysfs,
and all of the fixes are one-liners.

Hopefully with this we can head back to fixing the real issues.

Eric

Re: [PATCH 0/4] Better sysfs lockdep

[Greg KH]

On Thu, Feb 11, 2010 at 03:17:45PM -0800, Eric W. Biederman wrote:
> 
> It turns out that it is pretty simple to rework the lockdep support
> to give each sysfs attribute it's own lockdep class, which should
> kill the annoying false positives we have been seeing lately.
> 
> The one caveat is that it requires dynamically allocated sysfs
> attributes to be explicitly initialized.  Luckily it is brainless
> trivial whack-a-mole and took maybe 15 minutes to find all of the
> dynamically allocates sysfs attributes on my test machine, because
> the errors show up when the attributes are registered with sysfs,
> and all of the fixes are one-liners.
> 
> Hopefully with this we can head back to fixing the real issues.

Nice job, I'll queue this up for -next

thanks,

greg k-h

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Tejun Heo]

Hello, Eric.

On 02/11/2010 03:25 AM, Eric W. Biederman wrote:
>> Maybe I'm glossly misunderstanding it but wouldn't embedding struct
>> lockdep_map into sysfs_node as in work_struct do the trick?
> 
> In lockdep_init_map there is the following check:
> 
> 	/*
> 	 * Sanity check, the lock-class key must be persistent:
> 	 */
> 	if (!static_obj(key)) {
> 		printk("BUG: key %p not in .data!\n", key);
> 		DEBUG_LOCKS_WARN_ON(1);
> 		return;
> 	}

Right, the lockdep_map is not the class, it's the lock instance.

> It needs playing with but I think we can embed something in struct
> attribute, and simply disallow dynamically allocated instances of
> struct attribute.

But I think something along this line would be the right way to do it,
instead of trying to mark up all the use cases manually.  I'm pretty
sure if we start by giving separate classes to different sysfs types
(by attr or by sysfs_ops) there will be far less special cases which
would need manual markups.

Thanks.

-- 
tejun

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Eric W. Biederman]

Tejun Heo <tj@kernel.org> writes:

> Hello, Eric.
>
> On 02/11/2010 03:25 AM, Eric W. Biederman wrote:
>>> Maybe I'm glossly misunderstanding it but wouldn't embedding struct
>>> lockdep_map into sysfs_node as in work_struct do the trick?
>> 
>> In lockdep_init_map there is the following check:
>> 
>> 	/*
>> 	 * Sanity check, the lock-class key must be persistent:
>> 	 */
>> 	if (!static_obj(key)) {
>> 		printk("BUG: key %p not in .data!\n", key);
>> 		DEBUG_LOCKS_WARN_ON(1);
>> 		return;
>> 	}
>
> Right, the lockdep_map is not the class, it's the lock instance.
>
>> It needs playing with but I think we can embed something in struct
>> attribute, and simply disallow dynamically allocated instances of
>> struct attribute.
>
> But I think something along this line would be the right way to do it,
> instead of trying to mark up all the use cases manually.

Assuming it works I am in complete agreement.

>  I'm pretty
> sure if we start by giving separate classes to different sysfs types
> (by attr or by sysfs_ops) there will be far less special cases which
> would need manual markups.

sysfs_ops are not especially useful because practically everything
uses the sysfs_ops provided by the driver core.

We also need to put sysfs symlinks into a different class or possibly
even remove s_active from them as Neil Brown helpfully pointed out.

Eric

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Eric W. Biederman]

Neil Brown <neilb@suse.de> writes:

> Hi,
>  I've just spent a while sorting out some lockdep complaints triggered
>  by the recent addition of the "s_active" lockdep annotation in sysfs
>   (commit 846f99749ab68bbc7f75c74fec305de675b1a1bf)
>
>  Some of them are genuine and I have submitted a fix for those.
>  Some are, I think, debatable and I get to that is a minute.  I've
>  submitted a fix for them anyway.
>  But some are to my mind clearly bogus and I'm hoping that can be
>  fixed by the change below (or similar).
>  The 'bogus' ones are triggered by writing to a sysfs attribute file
>  for which the handler tries to delete a symlink from sysfs.
>  This appears to be a recursion on s_active as s_active is held while
>  the handler runs and is again needed to effect the delete.  However
>  as the thing being deleted is a symlink, it is very clearly a
>  different object to the thing triggering the delete, so there is no
>  real loop.
>
>  The following patch splits the lockdep context in two - one for
>  symlink and one for everything else.  This removes the apparent loop.
>  (An example report can be seen in
>      http://bugzilla.kernel.org/show_bug.cgi?id=15142).
>
>  The "debatable" dependency loops happen when writing to one attribute
>  causes a different attribute to be deleted.  In my (md) case this can
>  actually cause a deadlock as both the attributes take the same lock
>  while the handler is running.  This is because deleting the attribute
>  will block until the all accesses of that attribute have completed (I
>  think).

You are correct.  Not until the file handles are closed but until all
users of the underyling methods are complete.

>  However it should be possible to delete a name from sysfs while there
>  are still accesses pending (it works for normal files!!).  So if
>  sysfs could be changed to simply unlink the file and leave deletion to
>  happen when the refcount become zero it would certainly make my life
>  a lot easier, and allow the removal of some ugly code from md.c.
>  I don't know sysfs well enough to suggest a patch though.

Thanks for this.  

Separating out symlinks and treating them differently because they can not
cause problems is definitely worth doing.  We never take an active reference
in the symlink code so we can never block waiting for symlinks to be deleted.


We block when deleting files in sysfs (and proc and sysctl).  If we
did not block we could follow pointers into modules that are being
deleted, or those methods that are running could access data
structures that we want to tear down (perhaps there is a lock we want
to kfree).  Blocking in sysfs is to simplify the life of the callers.
Unfortunately for a handful of callers it complicates things.

If you want to compare this to regular files think of what sysfs is
doing as a combined remove and revoke.  The remove is easy.  Revoke
is just plane difficult.

Eric

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Dmitry Torokhov]

On Wednesday 10 February 2010 09:36:32 am Eric W. Biederman wrote:
> Neil Brown <neilb@suse.de> writes:
> > Hi,
> >  I've just spent a while sorting out some lockdep complaints triggered
> >  by the recent addition of the "s_active" lockdep annotation in sysfs
> >   (commit 846f99749ab68bbc7f75c74fec305de675b1a1bf)
> >
> >  Some of them are genuine and I have submitted a fix for those.
> >  Some are, I think, debatable and I get to that is a minute.  I've
> >  submitted a fix for them anyway.
> >  But some are to my mind clearly bogus and I'm hoping that can be
> >  fixed by the change below (or similar).
> >  The 'bogus' ones are triggered by writing to a sysfs attribute file
> >  for which the handler tries to delete a symlink from sysfs.
> >  This appears to be a recursion on s_active as s_active is held while
> >  the handler runs and is again needed to effect the delete.  However
> >  as the thing being deleted is a symlink, it is very clearly a
> >  different object to the thing triggering the delete, so there is no
> >  real loop.
> >
> >  The following patch splits the lockdep context in two - one for
> >  symlink and one for everything else.  This removes the apparent loop.
> >  (An example report can be seen in
> >      http://bugzilla.kernel.org/show_bug.cgi?id=15142).
> >
> >  The "debatable" dependency loops happen when writing to one attribute
> >  causes a different attribute to be deleted.  In my (md) case this can
> >  actually cause a deadlock as both the attributes take the same lock
> >  while the handler is running.  This is because deleting the attribute
> >  will block until the all accesses of that attribute have completed (I
> >  think).
> 
> You are correct.  Not until the file handles are closed but until all
> users of the underyling methods are complete.
> 
> >  However it should be possible to delete a name from sysfs while there
> >  are still accesses pending (it works for normal files!!).  So if
> >  sysfs could be changed to simply unlink the file and leave deletion to
> >  happen when the refcount become zero it would certainly make my life
> >  a lot easier, and allow the removal of some ugly code from md.c.
> >  I don't know sysfs well enough to suggest a patch though.
> 
> Thanks for this.
> 
> Separating out symlinks and treating them differently because they can not
> cause problems is definitely worth doing.  We never take an active
>  reference in the symlink code so we can never block waiting for symlinks
>  to be deleted.
> 
> 
> We block when deleting files in sysfs (and proc and sysctl).  If we
> did not block we could follow pointers into modules that are being
> deleted, or those methods that are running could access data
> structures that we want to tear down (perhaps there is a lock we want
> to kfree).  Blocking in sysfs is to simplify the life of the callers.
> Unfortunately for a handful of callers it complicates things.

Exactly. Before Tejun changed sysfs to provide guarantee that no
show/store methods are still running, nor new references to the
corresponding kobject will be acquired through sysfs after
sysfs_remove_file() returns, you had to jump through million of
hoops at subsystem level to work with lifetime rules and work around
the fact that kobjects could outlive your module.

I was glad to see bunch of ugly code in serio, gameport and input go
and I do not want it coming back ;)

> 
> If you want to compare this to regular files think of what sysfs is
> doing as a combined remove and revoke.  The remove is easy.  Revoke
> is just plane difficult.
> 
> Eric

-- 
Dmitry

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Greg KH]

On Wed, Feb 10, 2010 at 12:09:33PM +1100, Neil Brown wrote:
> 
> Hi,
>  I've just spent a while sorting out some lockdep complaints triggered
>  by the recent addition of the "s_active" lockdep annotation in sysfs
>   (commit 846f99749ab68bbc7f75c74fec305de675b1a1bf)
> 
>  Some of them are genuine and I have submitted a fix for those.
>  Some are, I think, debatable and I get to that is a minute.  I've
>  submitted a fix for them anyway.
>  But some are to my mind clearly bogus and I'm hoping that can be
>  fixed by the change below (or similar).
>  The 'bogus' ones are triggered by writing to a sysfs attribute file
>  for which the handler tries to delete a symlink from sysfs.
>  This appears to be a recursion on s_active as s_active is held while
>  the handler runs and is again needed to effect the delete.  However
>  as the thing being deleted is a symlink, it is very clearly a
>  different object to the thing triggering the delete, so there is no
>  real loop.
> 
>  The following patch splits the lockdep context in two - one for
>  symlink and one for everything else.  This removes the apparent loop.
>  (An example report can be seen in
>      http://bugzilla.kernel.org/show_bug.cgi?id=15142).
> 
>  The "debatable" dependency loops happen when writing to one attribute
>  causes a different attribute to be deleted.  In my (md) case this can
>  actually cause a deadlock as both the attributes take the same lock
>  while the handler is running.  This is because deleting the attribute
>  will block until the all accesses of that attribute have completed (I
>  think).
>  However it should be possible to delete a name from sysfs while there
>  are still accesses pending (it works for normal files!!).  So if
>  sysfs could be changed to simply unlink the file and leave deletion to
>  happen when the refcount become zero it would certainly make my life
>  a lot easier, and allow the removal of some ugly code from md.c.
>  I don't know sysfs well enough to suggest a patch though.
> 
> Thanks,
> NeilBrown
> 
> 
> 
> commit 2e502cfe444b68f6ef6b8b2abe83b6112564095b
> Author: NeilBrown <neilb@suse.de>
> Date:   Wed Feb 10 09:43:45 2010 +1100
> 
>     sysfs: differentiate  between locking links and non-links for sysfs
>     
>     symlinks and non-symlink is sysfs are very different.
>     A symlink can never be locked (active) while an attribute
>     modification routine is running.  So removing symlink from an
>     attribute 'store' routine should be permitted without any lockdep
>     warnings.
>     
>     So split the lockdep context for 's_active' in two, one for symlinks
>     and other for everything else.
>     
>     Signed-off-by: NeilBrown <neilb@suse.de>

Nice patch, I'll queue it up for .34.

thanks,

greg k-h

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Wed, Feb 10, 2010 at 12:09:33PM +1100, Neil Brown wrote:
>> 
>> Hi,
>>  I've just spent a while sorting out some lockdep complaints triggered
>>  by the recent addition of the "s_active" lockdep annotation in sysfs
>>   (commit 846f99749ab68bbc7f75c74fec305de675b1a1bf)
>> 
>>  Some of them are genuine and I have submitted a fix for those.
>>  Some are, I think, debatable and I get to that is a minute.  I've
>>  submitted a fix for them anyway.
>>  But some are to my mind clearly bogus and I'm hoping that can be
>>  fixed by the change below (or similar).
>>  The 'bogus' ones are triggered by writing to a sysfs attribute file
>>  for which the handler tries to delete a symlink from sysfs.
>>  This appears to be a recursion on s_active as s_active is held while
>>  the handler runs and is again needed to effect the delete.  However
>>  as the thing being deleted is a symlink, it is very clearly a
>>  different object to the thing triggering the delete, so there is no
>>  real loop.
>> 
>>  The following patch splits the lockdep context in two - one for
>>  symlink and one for everything else.  This removes the apparent loop.
>>  (An example report can be seen in
>>      http://bugzilla.kernel.org/show_bug.cgi?id=15142).
>> 
>>  The "debatable" dependency loops happen when writing to one attribute
>>  causes a different attribute to be deleted.  In my (md) case this can
>>  actually cause a deadlock as both the attributes take the same lock
>>  while the handler is running.  This is because deleting the attribute
>>  will block until the all accesses of that attribute have completed (I
>>  think).
>>  However it should be possible to delete a name from sysfs while there
>>  are still accesses pending (it works for normal files!!).  So if
>>  sysfs could be changed to simply unlink the file and leave deletion to
>>  happen when the refcount become zero it would certainly make my life
>>  a lot easier, and allow the removal of some ugly code from md.c.
>>  I don't know sysfs well enough to suggest a patch though.
>> 
>> Thanks,
>> NeilBrown
>> 
>> 
>> 
>> commit 2e502cfe444b68f6ef6b8b2abe83b6112564095b
>> Author: NeilBrown <neilb@suse.de>
>> Date:   Wed Feb 10 09:43:45 2010 +1100
>> 
>>     sysfs: differentiate  between locking links and non-links for sysfs
>>     
>>     symlinks and non-symlink is sysfs are very different.
>>     A symlink can never be locked (active) while an attribute
>>     modification routine is running.  So removing symlink from an
>>     attribute 'store' routine should be permitted without any lockdep
>>     warnings.
>>     
>>     So split the lockdep context for 's_active' in two, one for symlinks
>>     and other for everything else.
>>     
>>     Signed-off-by: NeilBrown <neilb@suse.de>
>
> Nice patch, I'll queue it up for .34.

Note the patch does not compile with lockdep disabled.

Eric

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Greg KH]

On Thu, Feb 11, 2010 at 01:42:10PM -0800, Eric W. Biederman wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > On Wed, Feb 10, 2010 at 12:09:33PM +1100, Neil Brown wrote:
> >> 
> >> Hi,
> >>  I've just spent a while sorting out some lockdep complaints triggered
> >>  by the recent addition of the "s_active" lockdep annotation in sysfs
> >>   (commit 846f99749ab68bbc7f75c74fec305de675b1a1bf)
> >> 
> >>  Some of them are genuine and I have submitted a fix for those.
> >>  Some are, I think, debatable and I get to that is a minute.  I've
> >>  submitted a fix for them anyway.
> >>  But some are to my mind clearly bogus and I'm hoping that can be
> >>  fixed by the change below (or similar).
> >>  The 'bogus' ones are triggered by writing to a sysfs attribute file
> >>  for which the handler tries to delete a symlink from sysfs.
> >>  This appears to be a recursion on s_active as s_active is held while
> >>  the handler runs and is again needed to effect the delete.  However
> >>  as the thing being deleted is a symlink, it is very clearly a
> >>  different object to the thing triggering the delete, so there is no
> >>  real loop.
> >> 
> >>  The following patch splits the lockdep context in two - one for
> >>  symlink and one for everything else.  This removes the apparent loop.
> >>  (An example report can be seen in
> >>      http://bugzilla.kernel.org/show_bug.cgi?id=15142).
> >> 
> >>  The "debatable" dependency loops happen when writing to one attribute
> >>  causes a different attribute to be deleted.  In my (md) case this can
> >>  actually cause a deadlock as both the attributes take the same lock
> >>  while the handler is running.  This is because deleting the attribute
> >>  will block until the all accesses of that attribute have completed (I
> >>  think).
> >>  However it should be possible to delete a name from sysfs while there
> >>  are still accesses pending (it works for normal files!!).  So if
> >>  sysfs could be changed to simply unlink the file and leave deletion to
> >>  happen when the refcount become zero it would certainly make my life
> >>  a lot easier, and allow the removal of some ugly code from md.c.
> >>  I don't know sysfs well enough to suggest a patch though.
> >> 
> >> Thanks,
> >> NeilBrown
> >> 
> >> 
> >> 
> >> commit 2e502cfe444b68f6ef6b8b2abe83b6112564095b
> >> Author: NeilBrown <neilb@suse.de>
> >> Date:   Wed Feb 10 09:43:45 2010 +1100
> >> 
> >>     sysfs: differentiate  between locking links and non-links for sysfs
> >>     
> >>     symlinks and non-symlink is sysfs are very different.
> >>     A symlink can never be locked (active) while an attribute
> >>     modification routine is running.  So removing symlink from an
> >>     attribute 'store' routine should be permitted without any lockdep
> >>     warnings.
> >>     
> >>     So split the lockdep context for 's_active' in two, one for symlinks
> >>     and other for everything else.
> >>     
> >>     Signed-off-by: NeilBrown <neilb@suse.de>
> >
> > Nice patch, I'll queue it up for .34.
> 
> Note the patch does not compile with lockdep disabled.

Ugh, why not?

Neil, care to fix this up?

thanks,

greg k-h

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Thu, Feb 11, 2010 at 01:42:10PM -0800, Eric W. Biederman wrote:
>> Greg KH <gregkh@suse.de> writes:
>> 
>> > On Wed, Feb 10, 2010 at 12:09:33PM +1100, Neil Brown wrote:
>> >> 
>> >> Hi,
>> >>  I've just spent a while sorting out some lockdep complaints triggered
>> >>  by the recent addition of the "s_active" lockdep annotation in sysfs
>> >>   (commit 846f99749ab68bbc7f75c74fec305de675b1a1bf)
>> >> 
>> >>  Some of them are genuine and I have submitted a fix for those.
>> >>  Some are, I think, debatable and I get to that is a minute.  I've
>> >>  submitted a fix for them anyway.
>> >>  But some are to my mind clearly bogus and I'm hoping that can be
>> >>  fixed by the change below (or similar).
>> >>  The 'bogus' ones are triggered by writing to a sysfs attribute file
>> >>  for which the handler tries to delete a symlink from sysfs.
>> >>  This appears to be a recursion on s_active as s_active is held while
>> >>  the handler runs and is again needed to effect the delete.  However
>> >>  as the thing being deleted is a symlink, it is very clearly a
>> >>  different object to the thing triggering the delete, so there is no
>> >>  real loop.
>> >> 
>> >>  The following patch splits the lockdep context in two - one for
>> >>  symlink and one for everything else.  This removes the apparent loop.
>> >>  (An example report can be seen in
>> >>      http://bugzilla.kernel.org/show_bug.cgi?id=15142).
>> >> 
>> >>  The "debatable" dependency loops happen when writing to one attribute
>> >>  causes a different attribute to be deleted.  In my (md) case this can
>> >>  actually cause a deadlock as both the attributes take the same lock
>> >>  while the handler is running.  This is because deleting the attribute
>> >>  will block until the all accesses of that attribute have completed (I
>> >>  think).
>> >>  However it should be possible to delete a name from sysfs while there
>> >>  are still accesses pending (it works for normal files!!).  So if
>> >>  sysfs could be changed to simply unlink the file and leave deletion to
>> >>  happen when the refcount become zero it would certainly make my life
>> >>  a lot easier, and allow the removal of some ugly code from md.c.
>> >>  I don't know sysfs well enough to suggest a patch though.
>> >> 
>> >> Thanks,
>> >> NeilBrown
>> >> 
>> >> 
>> >> 
>> >> commit 2e502cfe444b68f6ef6b8b2abe83b6112564095b
>> >> Author: NeilBrown <neilb@suse.de>
>> >> Date:   Wed Feb 10 09:43:45 2010 +1100
>> >> 
>> >>     sysfs: differentiate  between locking links and non-links for sysfs
>> >>     
>> >>     symlinks and non-symlink is sysfs are very different.
>> >>     A symlink can never be locked (active) while an attribute
>> >>     modification routine is running.  So removing symlink from an
>> >>     attribute 'store' routine should be permitted without any lockdep
>> >>     warnings.
>> >>     
>> >>     So split the lockdep context for 's_active' in two, one for symlinks
>> >>     and other for everything else.
>> >>     
>> >>     Signed-off-by: NeilBrown <neilb@suse.de>
>> >
>> > Nice patch, I'll queue it up for .34.
>> 
>> Note the patch does not compile with lockdep disabled.
>
> Ugh, why not?
>
> Neil, care to fix this up?

 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
-#define sysfs_dirent_init_lockdep(sd)				\
+#define sysfs_dirent_init_lockdep(sd, type)			\
 do {								\
 	static struct lock_class_key __key;			\
 								\
-	lockdep_init_map(&sd->dep_map, "s_active", &__key, 0);	\
+	lockdep_init_map(&sd->dep_map, "s_active_" type, &__key, 0);	\
 } while(0)
 #else
 #define sysfs_dirent_init_lockdep(sd) do {} while(0)
                                     ^^^^

Eric

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Greg KH]

On Thu, Feb 11, 2010 at 02:47:57PM -0800, Eric W. Biederman wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > On Thu, Feb 11, 2010 at 01:42:10PM -0800, Eric W. Biederman wrote:
> >> Greg KH <gregkh@suse.de> writes:
> >> 
> >> > On Wed, Feb 10, 2010 at 12:09:33PM +1100, Neil Brown wrote:
> >> >> 
> >> >> Hi,
> >> >>  I've just spent a while sorting out some lockdep complaints triggered
> >> >>  by the recent addition of the "s_active" lockdep annotation in sysfs
> >> >>   (commit 846f99749ab68bbc7f75c74fec305de675b1a1bf)
> >> >> 
> >> >>  Some of them are genuine and I have submitted a fix for those.
> >> >>  Some are, I think, debatable and I get to that is a minute.  I've
> >> >>  submitted a fix for them anyway.
> >> >>  But some are to my mind clearly bogus and I'm hoping that can be
> >> >>  fixed by the change below (or similar).
> >> >>  The 'bogus' ones are triggered by writing to a sysfs attribute file
> >> >>  for which the handler tries to delete a symlink from sysfs.
> >> >>  This appears to be a recursion on s_active as s_active is held while
> >> >>  the handler runs and is again needed to effect the delete.  However
> >> >>  as the thing being deleted is a symlink, it is very clearly a
> >> >>  different object to the thing triggering the delete, so there is no
> >> >>  real loop.
> >> >> 
> >> >>  The following patch splits the lockdep context in two - one for
> >> >>  symlink and one for everything else.  This removes the apparent loop.
> >> >>  (An example report can be seen in
> >> >>      http://bugzilla.kernel.org/show_bug.cgi?id=15142).
> >> >> 
> >> >>  The "debatable" dependency loops happen when writing to one attribute
> >> >>  causes a different attribute to be deleted.  In my (md) case this can
> >> >>  actually cause a deadlock as both the attributes take the same lock
> >> >>  while the handler is running.  This is because deleting the attribute
> >> >>  will block until the all accesses of that attribute have completed (I
> >> >>  think).
> >> >>  However it should be possible to delete a name from sysfs while there
> >> >>  are still accesses pending (it works for normal files!!).  So if
> >> >>  sysfs could be changed to simply unlink the file and leave deletion to
> >> >>  happen when the refcount become zero it would certainly make my life
> >> >>  a lot easier, and allow the removal of some ugly code from md.c.
> >> >>  I don't know sysfs well enough to suggest a patch though.
> >> >> 
> >> >> Thanks,
> >> >> NeilBrown
> >> >> 
> >> >> 
> >> >> 
> >> >> commit 2e502cfe444b68f6ef6b8b2abe83b6112564095b
> >> >> Author: NeilBrown <neilb@suse.de>
> >> >> Date:   Wed Feb 10 09:43:45 2010 +1100
> >> >> 
> >> >>     sysfs: differentiate  between locking links and non-links for sysfs
> >> >>     
> >> >>     symlinks and non-symlink is sysfs are very different.
> >> >>     A symlink can never be locked (active) while an attribute
> >> >>     modification routine is running.  So removing symlink from an
> >> >>     attribute 'store' routine should be permitted without any lockdep
> >> >>     warnings.
> >> >>     
> >> >>     So split the lockdep context for 's_active' in two, one for symlinks
> >> >>     and other for everything else.
> >> >>     
> >> >>     Signed-off-by: NeilBrown <neilb@suse.de>
> >> >
> >> > Nice patch, I'll queue it up for .34.
> >> 
> >> Note the patch does not compile with lockdep disabled.
> >
> > Ugh, why not?
> >
> > Neil, care to fix this up?
> 
>  
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> -#define sysfs_dirent_init_lockdep(sd)				\
> +#define sysfs_dirent_init_lockdep(sd, type)			\
>  do {								\
>  	static struct lock_class_key __key;			\
>  								\
> -	lockdep_init_map(&sd->dep_map, "s_active", &__key, 0);	\
> +	lockdep_init_map(&sd->dep_map, "s_active_" type, &__key, 0);	\
>  } while(0)
>  #else
>  #define sysfs_dirent_init_lockdep(sd) do {} while(0)

Got it, I've fixed this by hand.

thanks,

greg k-h

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Neil Brown]

On Wed, 17 Feb 2010 14:38:48 -0800
Greg KH <greg@kroah.com> wrote:

> On Thu, Feb 11, 2010 at 02:47:57PM -0800, Eric W. Biederman wrote:
> > Greg KH <gregkh@suse.de> writes:
> > 
> > > On Thu, Feb 11, 2010 at 01:42:10PM -0800, Eric W. Biederman wrote:
> > >> Greg KH <gregkh@suse.de> writes:
> > >> 
> > >> > On Wed, Feb 10, 2010 at 12:09:33PM +1100, Neil Brown wrote:
> > >> >> 
> > >> >> Hi,
> > >> >>  I've just spent a while sorting out some lockdep complaints triggered
> > >> >>  by the recent addition of the "s_active" lockdep annotation in sysfs
> > >> >>   (commit 846f99749ab68bbc7f75c74fec305de675b1a1bf)
> > >> >> 
> > >> >>  Some of them are genuine and I have submitted a fix for those.
> > >> >>  Some are, I think, debatable and I get to that is a minute.  I've
> > >> >>  submitted a fix for them anyway.
> > >> >>  But some are to my mind clearly bogus and I'm hoping that can be
> > >> >>  fixed by the change below (or similar).
> > >> >>  The 'bogus' ones are triggered by writing to a sysfs attribute file
> > >> >>  for which the handler tries to delete a symlink from sysfs.
> > >> >>  This appears to be a recursion on s_active as s_active is held while
> > >> >>  the handler runs and is again needed to effect the delete.  However
> > >> >>  as the thing being deleted is a symlink, it is very clearly a
> > >> >>  different object to the thing triggering the delete, so there is no
> > >> >>  real loop.
> > >> >> 
> > >> >>  The following patch splits the lockdep context in two - one for
> > >> >>  symlink and one for everything else.  This removes the apparent loop.
> > >> >>  (An example report can be seen in
> > >> >>      http://bugzilla.kernel.org/show_bug.cgi?id=15142).
> > >> >> 
> > >> >>  The "debatable" dependency loops happen when writing to one attribute
> > >> >>  causes a different attribute to be deleted.  In my (md) case this can
> > >> >>  actually cause a deadlock as both the attributes take the same lock
> > >> >>  while the handler is running.  This is because deleting the attribute
> > >> >>  will block until the all accesses of that attribute have completed (I
> > >> >>  think).
> > >> >>  However it should be possible to delete a name from sysfs while there
> > >> >>  are still accesses pending (it works for normal files!!).  So if
> > >> >>  sysfs could be changed to simply unlink the file and leave deletion to
> > >> >>  happen when the refcount become zero it would certainly make my life
> > >> >>  a lot easier, and allow the removal of some ugly code from md.c.
> > >> >>  I don't know sysfs well enough to suggest a patch though.
> > >> >> 
> > >> >> Thanks,
> > >> >> NeilBrown
> > >> >> 
> > >> >> 
> > >> >> 
> > >> >> commit 2e502cfe444b68f6ef6b8b2abe83b6112564095b
> > >> >> Author: NeilBrown <neilb@suse.de>
> > >> >> Date:   Wed Feb 10 09:43:45 2010 +1100
> > >> >> 
> > >> >>     sysfs: differentiate  between locking links and non-links for sysfs
> > >> >>     
> > >> >>     symlinks and non-symlink is sysfs are very different.
> > >> >>     A symlink can never be locked (active) while an attribute
> > >> >>     modification routine is running.  So removing symlink from an
> > >> >>     attribute 'store' routine should be permitted without any lockdep
> > >> >>     warnings.
> > >> >>     
> > >> >>     So split the lockdep context for 's_active' in two, one for symlinks
> > >> >>     and other for everything else.
> > >> >>     
> > >> >>     Signed-off-by: NeilBrown <neilb@suse.de>
> > >> >
> > >> > Nice patch, I'll queue it up for .34.
> > >> 
> > >> Note the patch does not compile with lockdep disabled.
> > >
> > > Ugh, why not?
> > >
> > > Neil, care to fix this up?
> > 
> >  
> >  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > -#define sysfs_dirent_init_lockdep(sd)				\
> > +#define sysfs_dirent_init_lockdep(sd, type)			\
> >  do {								\
> >  	static struct lock_class_key __key;			\
> >  								\
> > -	lockdep_init_map(&sd->dep_map, "s_active", &__key, 0);	\
> > +	lockdep_init_map(&sd->dep_map, "s_active_" type, &__key, 0);	\
> >  } while(0)
> >  #else
> >  #define sysfs_dirent_init_lockdep(sd) do {} while(0)
> 
> Got it, I've fixed this by hand.
> 
>

Thanks.
I hadn't sent you a fix myself (As requested) as I got the impression from
the following discussion that a different approach would be taken.

I'm happy either way though.

Thanks,
NeilBrown

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Eric W. Biederman]

Neil Brown <neilb@suse.de> writes:

> On Wed, 17 Feb 2010 14:38:48 -0800
> Greg KH <greg@kroah.com> wrote:

>> Got it, I've fixed this by hand.
>> 
>>
>
> Thanks.
> I hadn't sent you a fix myself (As requested) as I got the impression from
> the following discussion that a different approach would be taken.
>
> I'm happy either way though.

I had the same impression.

For simplicity I didn't base my patches on Neils because I would just
have had to undo it when I modified directories and symlinks to not
mess with s_active.

Eric

Re: [PATCH] sysfs: differentiate between locking links and non-links

[Greg KH]

On Wed, Feb 17, 2010 at 05:01:50PM -0800, Eric W. Biederman wrote:
> Neil Brown <neilb@suse.de> writes:
> 
> > On Wed, 17 Feb 2010 14:38:48 -0800
> > Greg KH <greg@kroah.com> wrote:
> 
> >> Got it, I've fixed this by hand.
> >> 
> >>
> >
> > Thanks.
> > I hadn't sent you a fix myself (As requested) as I got the impression from
> > the following discussion that a different approach would be taken.
> >
> > I'm happy either way though.
> 
> I had the same impression.
> 
> For simplicity I didn't base my patches on Neils because I would just
> have had to undo it when I modified directories and symlinks to not
> mess with s_active.

Argh, sorry about that, I'll go drop Neil's patch and queue up Eric's
instead.  I was working my way through the patch queue and hadn't gotten
to Eric's just yet...

thanks,

greg k-h