linux.git/fs/sysfs/bin.c, branch v2.6.28

sysfs: fix deadlock

2008-10-16T16:24:50+00:00

On Thu, Sep 11, 2008 at 10:27:10AM +0200, Ingo Molnar wrote:

> and it's working fine on most boxes. One testbox found this new locking
> scenario:
>
> PM: Adding info for No Bus:vcsa7
> EDAC DEBUG: MC0: i82860_check()
>
> =======================================================
> [ INFO: possible circular locking dependency detected ]
> 2.6.27-rc6-tip #1
> -------------------------------------------------------
> X/4873 is trying to acquire lock:
>  (&bb->mutex){--..}, at: [] mmap+0x40/0xa0
>
> but task is already holding lock:
>  (&mm->mmap_sem){----}, at: [] sys_mmap2+0x8e/0xc0
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&mm->mmap_sem){----}:
>        [] validate_chain+0xa96/0xf50
>        [] __lock_acquire+0x2cb/0x5b0
>        [] lock_acquire+0x89/0xc0
>        [] might_fault+0x6b/0x90
>        [] copy_to_user+0x38/0x60
>        [] read+0xfb/0x170
>        [] vfs_read+0x95/0x110
>        [] sys_pread64+0x63/0x80
>        [] sysenter_do_call+0x12/0x43
>        [] 0xffffffff
>
> -> #0 (&bb->mutex){--..}:
>        [] validate_chain+0x6b7/0xf50
>        [] __lock_acquire+0x2cb/0x5b0
>        [] lock_acquire+0x89/0xc0
>        [] __mutex_lock_common+0xab/0x3c0
>        [] mutex_lock_nested+0x38/0x50
>        [] mmap+0x40/0xa0
>        [] mmap_region+0x14e/0x450
>        [] do_mmap_pgoff+0x2ef/0x310
>        [] sys_mmap2+0xad/0xc0
>        [] sysenter_do_call+0x12/0x43
>        [] 0xffffffff
>
> other info that might help us debug this:
>
> 1 lock held by X/4873:
>  #0:  (&mm->mmap_sem){----}, at: [] sys_mmap2+0x8e/0xc0
>
> stack backtrace:
> Pid: 4873, comm: X Not tainted 2.6.27-rc6-tip #1
>  [] print_circular_bug_tail+0x79/0xc0
>  [] validate_chain+0x6b7/0xf50
>  [] ? trace_hardirqs_off_caller+0x15/0xb0
>  [] __lock_acquire+0x2cb/0x5b0
>  [] lock_acquire+0x89/0xc0
>  [] ? mmap+0x40/0xa0
>  [] __mutex_lock_common+0xab/0x3c0
>  [] ? mmap+0x40/0xa0
>  [] mutex_lock_nested+0x38/0x50
>  [] ? mmap+0x40/0xa0
>  [] mmap+0x40/0xa0
>  [] mmap_region+0x14e/0x450
>  [] ? arch_get_unmapped_area_topdown+0xf8/0x160
>  [] do_mmap_pgoff+0x2ef/0x310
>  [] sys_mmap2+0xad/0xc0
>  [] sysenter_do_call+0x12/0x43
>  [] ? __switch_to+0x130/0x220
>  =======================
> evbug.c: Event. Dev: input3, Type: 20, Code: 0, Value: 500
> warning: `sudo' uses deprecated v2 capabilities in a way that may be insecure.
>
> i've attached the config.
>
> at first sight it looks like a genuine bug in fs/sysfs/bin.c?

Yes, it is a real bug by the looks. bin.c takes bb->mutex under mmap_sem
when it is mmapped, and then does its copy_*_user under bb->mutex too.

Here is a basic fix for the sysfs lor.


From: Nick Piggin 
Signed-off-by: Ingo Molnar 
Signed-off-by: Greg Kroah-Hartman

sysfs: add copyrights

2007-10-12T21:51:12+00:00

Sysfs has gone through considerable amount of reimplementation.  Add
copyrights.  Any objections?  :-)

Signed-off-by: Tejun Heo 
Signed-off-by: Greg Kroah-Hartman

sysfs: make s_elem an anonymous union

2007-10-12T21:51:10+00:00

Make s_elem an anonymous union.  Prefixing with s_elem makes things
needlessly longer without any advantage.

Signed-off-by: Tejun Heo 
Acked-by: Cornelia Huck 
Signed-off-by: Greg Kroah-Hartman

sysfs: make bin attr open get active reference of parent too

2007-10-12T21:51:10+00:00

All bin attr operations require active references of itself and its
parent.  There's no reason to allow open when its parent has been
deactivated and allowing it is inconsistent with regular sysfs file.
Use sysfs_get_active_two() in bin attribute open function.

Signed-off-by: Tejun Heo 
Acked-by: Cornelia Huck 
Signed-off-by: Greg Kroah-Hartman

sysfs: kill unnecessary sysfs_get() in open paths

2007-10-12T21:51:10+00:00

There's no reason to get an extra reference to sysfs_dirent for an
open file.  Open file has a reference to the dentry which in turn has
a reference to sysfs_dirent.  This is fairly obvious as otherwise open
itself won't be able to access the sysfs_dirent.  Kill the extra
sysfs_get() and matching sysfs_put().

Signed-off-by: Tejun Heo 
Acked-by: Cornelia Huck 
Signed-off-by: Greg Kroah-Hartman

sysfs: cleanup semaphore.h

2007-10-12T21:51:03+00:00

Cleanup semaphore.h

Signed-off-by: Dave Young 
Signed-off-by: Greg Kroah-Hartman

sysfs: don't warn on removal of a nonexistent binary file

2007-08-22T21:35:36+00:00

This patch (as960) removes the error message and stack dump logged by
sysfs_remove_bin_file() when someone tries to remove a nonexistent
file.  The warning doesn't seem to be needed, since none of the other
file-, symlink-, or directory-removal routines in sysfs complain in a
comparable way.

Signed-off-by: Alan Stern 
Acked-by: Tejun Heo 
Acked-by: Cornelia Huck 
Signed-off-by: Greg Kroah-Hartman

sysfs: add parameter "struct bin_attribute *" in .read/.write methods for sysfs binary attributes

2007-07-11T23:09:09+00:00

Well, first of all, I don't want to change so many files either.

What I do:
Adding a new parameter "struct bin_attribute *" in the
.read/.write methods for the sysfs binary attributes.

In fact, only the four lines change in fs/sysfs/bin.c and
include/linux/sysfs.h do the real work.
But I have to update all the files that use binary attributes
to make them compatible with the new .read and .write methods.
I'm not sure if I missed any. :(

Why I do this:
For a sysfs attribute, we can get a pointer pointing to the
struct attribute in the .show/.store method,
while we can't do this for the binary attributes.
I don't know why this is different, but this does make it not
so handy to use the binary attributes as the regular ones.
So I think this patch is reasonable. :)

Who benefits from it:
The patch that exposes ACPI tables in sysfs
requires such an improvement.
All the table binary attributes share the same .read method.
Parameter "struct bin_attribute *" is used to get
the table signature and instance number which are used to
distinguish different ACPI table binary attributes.

Without this parameter, we need to offer different .read methods
for different ACPI table binary attributes.
This is impossible as there are various ACPI tables on different
platforms, and we don't know what they are until they are loaded.

Signed-off-by: Zhang Rui 
Signed-off-by: Greg Kroah-Hartman

sysfs: make kobj point to sysfs_dirent instead of dentry

2007-07-11T23:09:08+00:00

As kobj sysfs dentries and inodes are gonna be made reclaimable,
dentry can't be used as naming token for sysfs file/directory, replace
kobj->dentry with kobj->sd.  The only external interface change is
shadow directory handling.  All other changes are contained in kobj
and sysfs.

Signed-off-by: Tejun Heo 
Signed-off-by: Greg Kroah-Hartman

sysfs: kill unnecessary attribute->owner

2007-07-11T23:09:06+00:00

sysfs is now completely out of driver/module lifetime game.  After
deletion, a sysfs node doesn't access anything outside sysfs proper,
so there's no reason to hold onto the attribute owners.  Note that
often the wrong modules were accounted for as owners leading to
accessing removed modules.

This patch kills now unnecessary attribute->owner.  Note that with
this change, userland holding a sysfs node does not prevent the
backing module from being unloaded.

For more info regarding lifetime rule cleanup, please read the
following message.

  http://article.gmane.org/gmane.linux.kernel/510293

(tweaked by Greg to not delete the field just yet, to make it easier to
merge things properly.)

Signed-off-by: Tejun Heo 
Cc: Cornelia Huck 
Cc: Andrew Morton 
Signed-off-by: Greg Kroah-Hartman