linux-kernel - [RESEND][RFC][PATCH] leds: fix oops race in led trigger registration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080821071750.GA8046@localhost.localdomain>
Date:	Thu, 21 Aug 2008 09:17:50 +0200
From:	Vegard Nossum <vegard.nossum@...il.com>
To:	Richard Purdie <rpurdie@...ys.net>,
	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Zdenek Kabelac <zdenek.kabelac@...il.com>,
	Henrique de Moraes Holschuh <hmh@....eng.br>,
	linux-kernel@...r.kernel.org
Subject: [RESEND][RFC][PATCH] leds: fix oops race in led trigger
	registration

Hi,

I resubmit this, which was posted a month ago, with no responses from
the maintainer (and no recent fixes in this area either). I have not
tested it myself, but I am reasonably confident that this is an
obviously correct fix. I guess it would not hurt to exercise it as well.

Zdenek: Have recent kernels also been showing this behaviour? Did the
patch fix the problem for you? You said something about a new issue
showing up, do you think that was related to this patch?

Also, just one question (since I'm newbie at this): Is it dangerous to
register the &dev_attr_trigger device file before we put the device on
the list or initialize it completely? If so, the initialization should
probably be moved all to the top before the file is made public.


Vegard


>From ed99f8da46d90fa7ff058e1d8912d3abc7d6b3ca Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@...il.com>
Date: Thu, 21 Aug 2008 08:39:06 +0200
Subject: [PATCH] leds: fix oops race in led trigger registration

Zdenek Kabelac reported:
> I'm getting this oops while using 64bit kernel and running mostly
> 32bit system.
> This one happens sometimes during boot - unpredictible - usually
> once in 15 boots.

> BUG: spinlock bad magic on CPU#1, modprobe/815
>  lock: ffffffffa014f350, .magic: 00000000, .owner: modprobe/815, .owner_cpu: 1
> Pid: 815, comm: modprobe Not tainted 2.6.26 #44
>
> Call Trace:
>  [<ffffffff81060a87>] ? put_lock_stats+0x27/0x30
>  [<ffffffff81186c02>] spin_bug+0xa2/0xf0
>  [<ffffffff81186c71>] _raw_spin_unlock+0x21/0xa0
>  [<ffffffff812fa57f>] _spin_unlock_irqrestore+0x2f/0x90
>  [<ffffffff8117f36d>] __down_write_trylock+0x3d/0x60
>  [<ffffffff812f8c30>] down_write+0x40/0x70
>  [<ffffffff81263c17>] led_trigger_register+0xb7/0x110
>  [<ffffffff81263cae>] led_trigger_register_simple+0x3e/0x80
>  [<ffffffffa009b36e>] :mmc_core:mmc_add_host+0x3e/0x80

I find it likely that the cause of this is the fact that the cdev is
added to the list of leds before init_rwsem() is called. So we fix
this by reordering the initialization code a little. The fact that it
also happens unpredictably is also the sign of a race.

Cc: Zdenek Kabelac <zdenek.kabelac@...il.com>
Cc: Richard Purdie <rpurdie@...ys.net>
Cc: Henrique de Moraes Holschuh <hmh@....eng.br>
Signed-off-by: Vegard Nossum <vegard.nossum@...il.com>
---
 drivers/leds/led-class.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 559a408..44d13ac 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -113,13 +113,6 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 	if (rc)
 		goto err_out;
 
-	/* add to the list of leds */
-	down_write(&leds_list_lock);
-	list_add_tail(&led_cdev->node, &leds_list);
-	up_write(&leds_list_lock);
-
-	led_update_brightness(led_cdev);
-
 #ifdef CONFIG_LEDS_TRIGGERS
 	init_rwsem(&led_cdev->trigger_lock);
 
@@ -130,6 +123,13 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 	led_trigger_set_default(led_cdev);
 #endif
 
+	led_update_brightness(led_cdev);
+
+	/* add to the list of leds */
+	down_write(&leds_list_lock);
+	list_add_tail(&led_cdev->node, &leds_list);
+	up_write(&leds_list_lock);
+
 	printk(KERN_INFO "Registered led device: %s\n",
 			led_cdev->name);
 
@@ -138,7 +138,6 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 #ifdef CONFIG_LEDS_TRIGGERS
 err_out_led_list:
 	device_remove_file(led_cdev->dev, &dev_attr_brightness);
-	list_del(&led_cdev->node);
 #endif
 err_out:
 	device_unregister(led_cdev->dev);
-- 
1.5.5.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/